<< Back to previous view

[CLJ-415] smarter assert (prints locals) Created: 29/Jul/10  Updated: 06/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: errormsgs

Attachments: Text File clj-415-assert-prints-locals-v1.txt     Text File CLJ-415-v2.patch    
Patch: Code
Approval: Vetted
Waiting On: Rich Hickey

 Description   

Here is an implementation you can paste into a repl. Feedback wanted:

(defn ^{:private true} local-bindings
  "Produces a map of the names of local bindings to their values."
  [env]
  (let [symbols (map key env)]
    (zipmap (map (fn [sym] `(quote ~sym)) symbols) symbols)))

(defmacro assert
  "Evaluates expr and throws an exception if it does not evaluate to
 logical true."
  {:added "1.0"}
  [x]
  (when *assert*
    (let [bindings (local-bindings &env)]
      `(when-not ~x
         (let [sep# (System/getProperty "line.separator")]
           (throw (AssertionError. (apply str "Assert failed: " (pr-str '~x) sep#
                                          (map (fn [[k# v#]] (str "\t" k# " : " v# sep#)) ~bindings)))))))))


 Comments   
Comment by Assembla Importer [ 24/Aug/10 5:41 PM ]

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

Comment by Assembla Importer [ 24/Aug/10 5:41 PM ]

alexdmiller said: A simple example I tried for illustration:

user=> (let [a 1 b 2] (assert (= a b)))
#<CompilerException java.lang.AssertionError: Assert failed: (= a b)
 a : 1
 b : 2
Comment by Assembla Importer [ 24/Aug/10 5:41 PM ]

fogus said: Of course it's weird if you do something like:

(let [x 1 y 2 z 3 a 1 b 2 c 3] (assert (= x y)))
java.lang.AssertionError: Assert failed: (= x y)
 x : 1
 y : 2
 z : 3
 a : 1
 b : 2
 c : 3
 (NO_SOURCE_FILE:0)
</code></pre>

So maybe it could be slightly changed to:
<pre><code>(defmacro assert
  "Evaluates expr and throws an exception if it does not evaluate to logical true."
  {:added "1.0"}
  [x]
  (when *assert*
    (let [bindings (local-bindings &env)]
      `(when-not ~x
         (let [sep#  (System/getProperty "line.separator")
               form# '~x]
           (throw (AssertionError. (apply str "Assert failed: " (pr-str form#) sep#
                                          (map (fn [[k# v#]] 
                                                 (when (some #{k#} form#) 
                                                   (str "\t" k# " : " v# sep#))) 
                                               ~bindings)))))))))
</code></pre>

So that. now it's just:
<pre><code>(let [x 1 y 2 z 3 a 1 b 2 c 3] (assert (= x y)))
java.lang.AssertionError: Assert failed: (= x y)
 x : 1
 y : 2
 (NO_SOURCE_FILE:0)

:f

Comment by Assembla Importer [ 24/Aug/10 5:41 PM ]

fogus said: Hmmm, but that fails entirely for: (let [x 1 y 2 z 3 a 1 b 2 c 3] (assert (= [x y] [a c]))). So maybe it's better just to print all of the locals unless you really want to get complicated.
:f

Comment by Assembla Importer [ 24/Aug/10 5:41 PM ]

jawolfe said: See also some comments in:

http://groups.google.com/group/clojure-dev/browse_frm/thread/68d49cd7eb4a4899/9afc6be4d3f8ae27?lnk=gst&q=assert#9afc6be4d3f8ae27

Plus one more suggestion to add to the mix: in addition to / instead of printing the locals, how about saving them somewhere. For example, the var assert-bindings could be bound to the map of locals. This way you don't run afoul of infinite/very large sequences, and allow the user to do more detailed interrogation of the bad values (especially useful when some of the locals print opaquely).

Comment by Assembla Importer [ 24/Aug/10 5:41 PM ]

stuart.sierra said: Another approach, which I wil willingly donate:
http://github.com/stuartsierra/lazytest/blob/master/src/main/clojure/lazytest/expect.clj

Comment by Jeff Weiss [ 15/Dec/10 1:33 PM ]

There's one more tweak to fogus's last comment, which I'm actually using. You need to flatten the quoted form before you can use 'some' to check whether the local was used in the form:

(defmacro assert
  "Evaluates expr and throws an exception if it does not evaluate to logical true."
  {:added "1.0"}
  [x]
  (when *assert*
    (let [bindings (local-bindings &env)]
      `(when-not ~x
         (let [sep#  (System/getProperty "line.separator")
               form# '~x]
           (throw (AssertionError. (apply str "Assert failed: " (pr-str form#) sep#
                                          (map (fn [[k# v#]] 
                                                 (when (some #{k#} (flatten form#)) 
                                                   (str "\t" k# " : " v# sep#))) 
                                               ~bindings)))))))))
Comment by Stuart Halloway [ 04/Jan/11 8:31 PM ]

I am holding off on this until we have more solidity around http://dev.clojure.org/display/design/Error+Handling. (Considering, for instance, having all exceptions thrown from Clojure provide access to locals.)

When my pipe dream fades I will come back and screen this before the next release.

Comment by Stuart Halloway [ 28/Jan/11 1:14 PM ]

Why try to guess what someone wants to do with the locals (or any other context, for that matter) when you can specify a callback (see below). This would have been useful last week when I had an assertion that failed only on the CI box, where no debugger is available.

Rich, at the risk of beating a dead horse, I still think this is a good idea. Debuggers are not always available, and this is an example of where a Lisp is intrinsically capable of providing better information than can be had in other environments. If you want a patch for the code below please mark waiting on me, otherwise please decline this ticket so I stop looking at it.

(def ^:dynamic *assert-handler* nil)

(defn ^{:private true} local-bindings
  "Produces a map of the names of local bindings to their values."
  [env]
  (let [symbols (map key env)]
    (zipmap (map (fn [sym] `(quote ~sym)) symbols) symbols)))

(defmacro assert
  [x]
  (when *assert*
    (let [bindings (local-bindings &env)]
      `(when-not ~x
         (let [sep#  (System/getProperty "line.separator")
               form# '~x]
           (if *assert-handler*
             (*assert-handler* form# ~bindings)
             (throw (AssertionError. (apply str "Assert failed: " (pr-str form#) sep#
                                            (map (fn [[k# v#]] 
                                                   (when (some #{k#} (flatten form#)) 
                                                     (str "\t" k# " : " v# sep#))) 
                                                 ~bindings))))))))))
Comment by Jeff Weiss [ 27/May/11 8:16 AM ]

A slight improvement I made in my own version of this code: flatten does not affect set literals. So if you do (assert (some #{x} [a b c d])) the value of x will not be printed. Here's a modified flatten that does the job:

(defn symbols [sexp]
  "Returns just the symbols from the expression, including those
   inside literals (sets, maps, lists, vectors)."
  (distinct (filter symbol? (tree-seq coll? seq sexp))))
Comment by Andy Fingerhut [ 18/Nov/12 1:06 AM ]

Attaching git format patch clj-415-assert-prints-locals-v1.txt of Stuart Halloway's version of this idea. I'm not advocating it over the other variations, just getting a file attached to the JIRA ticket.

Comment by Michael Blume [ 05/Jul/15 7:53 PM ]

Previous patch was incompatible with CLJ-1005, which moves zipmap later in clojure.core. Rewrote to use into.

Comment by Michael Blume [ 06/Jul/15 3:30 PM ]

Both patches are somehow incompatible with CLJ-1224. When building my compojure-api project I get

Exception in thread "main" java.lang.UnsupportedOperationException: Can't type hint a primitive local, compiling:(schema/core.clj:680:27)
	at clojure.lang.Compiler.analyze(Compiler.java:6569)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$MapExpr.parse(Compiler.java:3050)
	at clojure.lang.Compiler.analyze(Compiler.java:6558)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$InvokeExpr.parse(Compiler.java:3791)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6751)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$NewExpr$Parser.parse(Compiler.java:2614)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$ThrowExpr$Parser.parse(Compiler.java:2409)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5889)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2785)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5889)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:6205)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2777)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2785)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2785)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5891)
	at clojure.lang.Compiler$FnMethod.parse(Compiler.java:5322)
	at clojure.lang.Compiler$FnExpr.parse(Compiler.java:3925)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6747)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5891)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:6205)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2777)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5891)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:6205)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6737)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5891)
	at clojure.lang.Compiler$NewInstanceMethod.parse(Compiler.java:8165)
	at clojure.lang.Compiler$NewInstanceExpr.build(Compiler.java:7672)
	at clojure.lang.Compiler$NewInstanceExpr$DeftypeParser.parse(Compiler.java:7549)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5889)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:6205)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6749)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.analyze(Compiler.java:6511)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5891)
	at clojure.lang.Compiler$FnMethod.parse(Compiler.java:5322)
	at clojure.lang.Compiler$FnExpr.parse(Compiler.java:3925)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6747)
	at clojure.lang.Compiler.analyze(Compiler.java:6550)
	at clojure.lang.Compiler.eval(Compiler.java:6805)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5679)
	at clojure.core$load_lib$fn__5409.invoke(core.clj:5719)
	at clojure.core$load_lib.doInvoke(core.clj:5718)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$load_libs.doInvoke(core.clj:5757)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$require.doInvoke(core.clj:5840)
	at clojure.lang.RestFn.invoke(RestFn.java:436)
	at plumbing.fnk.schema$eval12507$loading__5352__auto____12508.invoke(schema.clj:1)
	at plumbing.fnk.schema$eval12507.invoke(schema.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6797)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5679)
	at clojure.core$load_lib$fn__5409.invoke(core.clj:5719)
	at clojure.core$load_lib.doInvoke(core.clj:5718)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$load_libs.doInvoke(core.clj:5757)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$require.doInvoke(core.clj:5840)
	at clojure.lang.RestFn.invoke(RestFn.java:457)
	at plumbing.core$eval12244$loading__5352__auto____12245.invoke(core.clj:1)
	at plumbing.core$eval12244.invoke(core.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6797)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5679)
	at clojure.core$load_lib$fn__5409.invoke(core.clj:5719)
	at clojure.core$load_lib.doInvoke(core.clj:5718)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$load_libs.doInvoke(core.clj:5757)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$require.doInvoke(core.clj:5840)
	at clojure.lang.RestFn.invoke(RestFn.java:930)
	at compojure.api.meta$eval11960$loading__5352__auto____11961.invoke(meta.clj:1)
	at compojure.api.meta$eval11960.invoke(meta.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6797)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5679)
	at clojure.core$load_lib$fn__5409.invoke(core.clj:5719)
	at clojure.core$load_lib.doInvoke(core.clj:5718)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$load_libs.doInvoke(core.clj:5757)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$require.doInvoke(core.clj:5840)
	at clojure.lang.RestFn.invoke(RestFn.java:703)
	at compojure.api.core$eval11954$loading__5352__auto____11955.invoke(core.clj:1)
	at compojure.api.core$eval11954.invoke(core.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6797)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5679)
	at clojure.core$load_lib$fn__5409.invoke(core.clj:5719)
	at clojure.core$load_lib.doInvoke(core.clj:5718)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$load_libs.doInvoke(core.clj:5757)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$require.doInvoke(core.clj:5840)
	at clojure.lang.RestFn.invoke(RestFn.java:457)
	at compojure.api.sweet$eval11948$loading__5352__auto____11949.invoke(sweet.clj:1)
	at compojure.api.sweet$eval11948.invoke(sweet.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6797)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5679)
	at clojure.core$load_lib$fn__5409.invoke(core.clj:5719)
	at clojure.core$load_lib.doInvoke(core.clj:5718)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$load_libs.doInvoke(core.clj:5757)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$require.doInvoke(core.clj:5840)
	at clojure.lang.RestFn.invoke(RestFn.java:703)
	at com.climate.scouting.resources$eval10202$loading__5352__auto____10203.invoke(resources.clj:1)
	at com.climate.scouting.resources$eval10202.invoke(resources.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6797)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5679)
	at clojure.core$load_lib$fn__5409.invoke(core.clj:5719)
	at clojure.core$load_lib.doInvoke(core.clj:5718)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$load_libs.doInvoke(core.clj:5761)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$require.doInvoke(core.clj:5840)
	at clojure.lang.RestFn.invoke(RestFn.java:703)
	at com.climate.scouting.service$eval8256$loading__5352__auto____8257.invoke(service.clj:1)
	at com.climate.scouting.service$eval8256.invoke(service.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6797)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5679)
	at clojure.core$load_lib$fn__5409.invoke(core.clj:5719)
	at clojure.core$load_lib.doInvoke(core.clj:5718)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$load_libs.doInvoke(core.clj:5757)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:632)
	at clojure.core$require.doInvoke(core.clj:5840)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at com.climate.scouting.dev_server$eval8250$loading__5352__auto____8251.invoke(dev_server.clj:1)
	at com.climate.scouting.dev_server$eval8250.invoke(dev_server.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6797)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.RT.loadResourceScript(RT.java:371)
	at clojure.lang.RT.loadResourceScript(RT.java:362)
	at clojure.lang.RT.load(RT.java:446)
	at clojure.lang.RT.load(RT.java:412)
	at clojure.core$load$fn__5460.invoke(core.clj:5874)
	at clojure.core$load.doInvoke(core.clj:5873)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at user$eval58$fn__69.invoke(form-init9085313321330645488.clj:1)
	at user$eval58.invoke(form-init9085313321330645488.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6808)
	at clojure.lang.Compiler.eval(Compiler.java:6798)
	at clojure.lang.Compiler.load(Compiler.java:7253)
	at clojure.lang.Compiler.loadFile(Compiler.java:7191)
	at clojure.main$load_script.invoke(main.clj:275)
	at clojure.main$init_opt.invoke(main.clj:280)
	at clojure.main$initialize.invoke(main.clj:308)
	at clojure.main$null_opt.invoke(main.clj:343)
	at clojure.main$main.doInvoke(main.clj:421)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at clojure.lang.Var.invoke(Var.java:383)
	at clojure.lang.AFn.applyToHelper(AFn.java:156)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: java.lang.UnsupportedOperationException: Can't type hint a primitive local
	at clojure.lang.Compiler$LocalBindingExpr.<init>(Compiler.java:5792)
	at clojure.lang.Compiler.analyzeSymbol(Compiler.java:6929)
	at clojure.lang.Compiler.analyze(Compiler.java:6532)
	... 299 more
Failed.
Comment by Andy Fingerhut [ 06/Jul/15 4:02 PM ]

Michael, are you finding these incompatibilities between patches because you want to run a modified version of Clojure with all of these patches? Understood, if so.

If you are looking for pairs of patches that are incompatible with each other, I'd recommend a different hobby They get applied at the rate of about 9 per month, on average, so there should be plenty of time to resolve inconsistencies between them later.





[CLJ-1218] mapcat is too eager Created: 16/Jun/13  Updated: 06/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: lazy


 Description   

The following expression prints 1234 and returns 1:

(first (mapcat #(do (print %) [%]) '(1 2 3 4 5 6 7)))

The reason is that (apply concat args) is not maximally lazy in its arguments, and indeed will realize the first four before returning the first item. This in turn is essentially unavoidable for a variadic concat.

This could either be fixed just in mapcat, or by adding a new function (to clojure.core?) that is a non-variadic equivalent to concat, and reimplementing mapcat with it:

(defn join
  "Lazily concatenates a sequence-of-sequences into a flat sequence."
  [s]
  (lazy-seq (when-let [[x & xs] (seq s)] (concat x (join xs)))))


 Comments   
Comment by Gary Fredericks [ 17/Jun/13 7:54 AM ]

I realized that concat could actually be made lazier without changing its semantics, if it had a single [& args] clause that was then implemented similarly to join above.

Comment by John Jacobsen [ 27/Jul/13 8:08 AM ]

I lost several hours understanding this issue last month [1, 2] before seeing this ticket in Jira today... +1.

[1] http://eigenhombre.com/2013/07/13/updating-the-genome-decoder-resulting-consequences/

[2] http://clojurian.blogspot.com/2012/11/beware-of-mapcat.html

Comment by Gary Fredericks [ 05/Feb/14 1:35 PM ]

Updated join code to be actually valid.

Comment by Ghadi Shayban [ 21/May/15 8:32 PM ]

The version of join in the description is not maximally lazy either, and will realize two of the underlying collections. Reason: destructuring the seq results in a call to 'nth' for 'x' and 'nthnext' for 'xs'. nthnext is not maximally lazy.

(defn join
  "Lazily concatenates a sequence-of-sequences into a flat sequence."
  [s]
  (lazy-seq
   (when-let [s (seq s)] 
     (concat (first s) (join (rest s))))))




[CLJ-428] subseq, rsubseq enhancements to support priority maps? Created: 20/Aug/10  Updated: 05/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Stuart Halloway Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None

Attachments: Text File clj-428-code-v5.patch     Text File clj-428-tests-v5.patch    
Patch: Code and Test

 Description   

See dev thread at http://groups.google.com/group/clojure-dev/browse_thread/thread/fdb000cae4f66a95.

Excerpted from Mark Engelberg's message of July 14, 2010 in that discussion thread:

It will only be possible to properly implement subseq on priority maps with a patch to the core. subseq achieves its polymorphic behavior (over sorted maps and sorted sets) by delegating much of its behavior to the Java methods seqFrom, seq, entryKey, and comparator. So in theory, you should be able to extend subseq to work for any new collection by customizing those methods.

However, the current implementation of subseq assumes that the values you are sorting on are unique, which is a reasonable assumption for the built-in sorted maps which sort on unique keys, but it is then impossible to extend subseq behavior to other types of collections. To give library designers the power to hook into subseq, this assumption needs to be removed.

Approaches:

1. A simple way is to allow for dropping multiple equal values when subseq massages the results of seqFrom into a strictly greater than sequence.

2. A better way is for subseq to delegate more of its logic to seqFrom (by adding an extra flag to the method call as to whether you
want greater than or equal or strictly greater than). This will allow for the best efficiency, and provide the greatest possibilities for future extensions.

Note: subseq currently returns () instead of nil in some situations. If the rest of this idea dies we might still want to fix that.

Patch clj-428-code-v5.patch implements approach #2 above. It preserves the current behavior of returning () instead of nil.

Patch: clj-428-code-v5.patch tests in clj-428-tests-v5.patch



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

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

Comment by Andy Fingerhut [ 28/Apr/13 2:14 AM ]

Patch clj-428-change-Sorted-seqFrom-to-take-inclusive-patch-v1.txt dated Apr 28 2013 was written by Mark Engelberg in July 2010, and was attached to a message he sent to the dev thread linked in the description. The approach he takes is described by him in that thread, copied here:

----------------------------------------
Meanwhile, to initiate discussion on how to modify subseq, I've attached a proposed patch. This patch works by modifying the seqFrom method of the Sorted interface to take an additional "inclusive" parameter (i.e., <= and >= are inclusive, < and > are not).

In this patch, I do not address one issue I raised before, which is whether subseq implies by its name that it should return a seq rather than a sequence (in other words nil rather than ()). If seq behavior is desired, it would be necessary to wrap a call to seq around the calls to take-while. But for now, I'm just making the behavior match the current behavior.

Although I think this is the cleanest way to address the extensibility issue with subseq, the change to seqFrom will break anyone who currently is overriding that method. I didn't see any such classes in clojure.contrib, so I don't think it's an issue, but if this is a concern, my other idea is to fix the problem entirely within subseq by changing the calls to next into calls to drop-while. I have coded this to confirm that it works, and can provide that alternative patch if desired.
----------------------------------------

I can also supply a patch that uses drop-while in clojure.core/subseq and rsubseq if such an approach is preferred to the one in this patch.

Comment by Andy Fingerhut [ 28/Apr/13 12:12 PM ]

clj-428-change-Sorted-seqFrom-to-take-inclusive-patch-v2.txt dated Apr 28 2013 is same as clj-428-change-Sorted-seqFrom-to-take-inclusive-patch-v1.txt (soon to be deleted), except it adds tests for subseq and rsubseq, and corrects a bug in that patch. The approach is the same as described above for that patch.

Comment by Andy Fingerhut [ 01/May/13 2:44 AM ]

Patch clj-428-change-Sorted-seqFrom-to-take-inclusive-patch-v3.txt dated May 1 2013 is the same as clj-428-change-Sorted-seqFrom-to-take-inclusive-patch-v1.txt, still with the bug fix mentioned for -v2, but with some unnecessary changes removed from the patch. The comments for -v1.txt on the approach still apply.

Comment by Andy Fingerhut [ 14/Feb/14 12:20 PM ]

Patch clj-428-v4.diff is identical to clj-428-change-Sorted-seqFrom-to-take-inclusive-patch-v3.txt described in an earlier comment, except it updates some diff context lines so that it applies cleanly to the latest Clojure master as of today.

Comment by Andy Fingerhut [ 05/Jul/15 6:08 PM ]

Files clj-428-code-v5.patch and clj-428-tests-v5.patch are identical to the previous -v4 attachment, except they apply cleanly to the latest Clojure master. The code and tests are in separate files since they have different authors, and it will be easier to update such patches in the future if they are kept separate.





[CLJ-1671] Clojure socket server Created: 09/Mar/15  Updated: 04/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Unresolved Votes: 0
Labels: repl

Attachments: Text File clj-1671-2.patch     Text File clj-1671-3.patch     Text File clj-1671-4.patch     Text File clj-1671-5.patch    
Patch: Code and Test
Approval: Incomplete

 Description   

Programs often want to provide REPLs to users in contexts when a) network communication is desired, b) capturing stdio is difficult, or c) when more than one REPL session is desired. In addition, tools that want to support REPLs and simultaneous conversations with the host are difficult with a single stdio REPL as currently provided by Clojure.

Tooling and users often need to enable a REPL on a program without changing the program, e.g. without asking author or program to include code to start a REPL host of some sort. Thus a solution must be externally and declaratively configured (no user code changes). A REPL is just a special case of a socket service. Rather than provide a socket server REPL, provide a built-in socket server that composes with the existing repl function.

For design background, see: http://dev.clojure.org/display/design/Socket+Server+REPL

Start a socket server by supplying an extra system property:

java -cp clojure.jar:app.jar my.app -Dclojure.server.repl="{:address \"127.0.0.1\" :\port 5555 :accept clojure.repl/repl}"

where options are:

  • address = host or address, defaults to loopback
  • port = port, required
  • accept = namespaced function to invoke on socket accept, required
  • args = sequential collection of args to pass to accept
  • bind-err = defaults to true, binds err to out stream
  • server-daemon = defaults to true, socket server thread doesn't block exit
  • client-daemon = defaults to true, socket client threads don't block exit

If you want to test a repl client with the a repl server, telnet works:

$ telnet 127.0.0.1 5555
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
user=> (+ 1 1)
2
user=> (/ 1 0)
#error {:cause "Divide by zero",
 :via
 [{:type java.lang.ArithmeticException,
   :message "Divide by zero",
   :at [clojure.lang.Numbers divide "Numbers.java" 158]}],
 :trace
 [[clojure.lang.Numbers divide "Numbers.java" 158]  
  [clojure.lang.Numbers divide "Numbers.java" 3808]
  [user1$eval1 invoke "NO_SOURCE_FILE" 1]
  [clojure.lang.Compiler eval "Compiler.java" 6784]
  [clojure.lang.Compiler eval "Compiler.java" 6747]
  [clojure.core$eval invoke "core.clj" 3078]
  [clojure.main$repl$read_eval_print__8287$fn__8290 invoke "main.clj" 265]
  [clojure.main$repl$read_eval_print__8287 invoke "main.clj" 265]
  [clojure.main$repl$fn__8296 invoke "main.clj" 283]
  [clojure.main$repl doInvoke "main.clj" 283]
  [clojure.lang.RestFn invoke "RestFn.java" 619]
  [clojure.main$socket_repl_server$fn__8342$fn__8344 invoke "main.clj" 450]
  [clojure.lang.AFn run "AFn.java" 22]
  [java.lang.Thread run "Thread.java" 724]]}
user1=> (println "hello")
hello
nil

Patch: clj-1671-5.patch (wip - not yet complete)



 Comments   
Comment by Timothy Baldridge [ 09/Mar/15 5:50 PM ]

Could we perhaps keep this as a contrib library? This ticket simply states "The goal is to provide a simple streaming socket repl as part of Clojure." What is the rationale for the "part of Clojure" bit?

Comment by Alex Miller [ 09/Mar/15 7:33 PM ]

We want this to be available as a Clojure.main option. It's all additive - why wouldn't you want it in the box?

Comment by Timothy Baldridge [ 09/Mar/15 10:19 PM ]

It never has really been too clear to me why some features are included in core, while others are kept in contrib. I understand that some are simply for historical reasons, but aside from that there doesn't seem to be too much of a philosophy behind it.

However it should be noted that since patches to clojure are much more guarded it's sometimes nice to have certain features in contrib, that way they can evolve with more rapidity than the one release a year that clojure has been going through.

But aside from those issues, I've found that breaking functionality into modules forces the core of a system to become more configurable. Perhaps I would like to use this repl socket feature, but pipe the data over a different communication protocol, or through a different serializer. If this feature were to be coded as a contrib library it would expose extension points that others could use to add additional functionality.

So I guess, all that to say, I'd prefer a tool I can compose rather than a pre-built solution.

Comment by Rich Hickey [ 10/Mar/15 6:25 AM ]

Please move discussions on the merits of the idea to the dev list. Comments should be about the work of resolving the ticket, approach taken by the patch, quality/perf issues etc.

Comment by Colin Jones [ 11/Mar/15 1:33 PM ]

I see that context (a) of the rationale is that network communication is desired, which sounds to me like users of this feature may want to communicate across hosts (whether in VMs or otherwise). Is that the case?

If so, it seems like specifying the address to bind to (e.g. "0.0.0.0", "::", "127.0.0.1", etc.) may become important as well as the existing port option. This way, someone who wants to communicate across hosts (or conversely, lock down access to local-only) can make that decision.

Comment by Alex Miller [ 11/Mar/15 2:07 PM ]

Colin - agreed. There are many ways to potentially customize what's in there so we need to figure out what's worth doing, both in the function and via the command line.

I think address is clearly worth having via the function and possibly in the command line too.

Comment by Kevin Downey [ 11/Mar/15 5:49 PM ]

I find the exception printing behavior really odd. for a machine you want an exception as data, but you also want some indication of if the data is an error or not, for a human you wanted a pretty printed stacktrace. making the socket repl default to printing errors this way seems to optimize for neither.

Comment by Rich Hickey [ 12/Mar/15 12:29 PM ]

Did you miss the #error tag? That indicates the data is an error. It is likely we will pprint the error data, making it not bad for both purposes

Comment by Alex Miller [ 13/Mar/15 11:29 AM ]

New -4 patch changes:

  • clojure.core/throwable-as-map now public and named clojure.core/Throwable->map
  • catch and ignore SocketException without printing in socket server repl (for client disconnect)
  • functions to print as message and as data are now: clojure.main/err-print and clojure.main/err->map. All defaults and docs updated.
Comment by David Nolen [ 18/Mar/15 12:44 PM ]

Is there any reason to not allow supplying :eval in addition to :use-prompt? In the case of projects like ClojureCLR + Unity eval generally must happen on the main thread. With :eval as something which can be configured, REPL sessions can queue forms to be eval'ed with the needed context (current ns etc.) to the main thread.

Comment by Kevin Downey [ 20/Mar/15 2:12 PM ]

I did see the #error tag, but throwables print with that tag regardless of if they are actually thrown or if they are just the value returned from a function. Admittedly returning exceptions as values is not something generally done, but the jvm does distinguish between a return value and a thrown exception. Having a repl that doesn't distinguish between the two strikes me as an odd design. The repl you get from clojure.main currently prints the message from a thrown uncaught throwable, and on master prints with #error if you have a throwable value, so it distinguishes between an uncaught thrown throwable and a throwable value. That obviously isn't great for tooling because you don't get a good data representation in the uncaught case.

It looks like the most recent patch does pretty print uncaught throwables, which is helpful for humans to distinguish between a returned value and an uncaught throwable.

Comment by Kevin Downey [ 25/Mar/15 1:10 PM ]

alex: saying this is all additive, when it has driven changes to how things are printed, using the global print-method, rings false to me

Comment by Sam Ritchie [ 25/Mar/15 1:15 PM ]

This seems like a pretty big last minute addition for 1.7. What's the rationale for adding it here vs deferring to 1.8, or trying it out as a contrib first?

Comment by Alex Miller [ 25/Mar/15 2:13 PM ]

Kevin: changing the fallthrough printing for things that are unreadable to be readable should be useful regardless of the socket repl. It shouldn't be a change for existing programs (unless they're relying on the toString of objects without print formats).

Sam: Rich wants it in the box as a substrate for tools.

Comment by Alex Miller [ 26/Mar/15 10:03 AM ]

Marking incomplete, pending at least the repl exit question.

Comment by Laurent Petit [ 29/Apr/15 2:18 PM ]

Hello, I intend to work on this, if it appears it still has a good probability of being included in clojure 1.7.
There hasn't been much visible activity on it lately.
What is the current status of the pending question, and do you think it will still make it in 1.7?

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

This has been pushed to 1.8 and is on my plate. The direction has diverged quite a bit from the original description and we don't expect to modify clojure.main as is done in the prior patches. So, I would recommend not working on it as described here.

Comment by Laurent Petit [ 01/May/15 7:24 AM ]

OK thanks for the update.

Is the discussion about the new design / goal (you say the direction has diverged) available somewhere so that I can keep in touch with what the Hammock Time is producing? Because on my own hammock time I'm doing some mental projections for CCW support of this, based on what is publicly available here -

Also, as soon as you have something available for testing please don't hesitate to ping me, I'll see what I can do to help depending on my schedule. Cheers.

Comment by Alex Miller [ 01/May/15 8:44 AM ]

Some design work is here - http://dev.clojure.org/display/design/Socket+Server+REPL.

Comment by Laurent Petit [ 05/May/15 11:41 AM ]

Thanks for the link. It seems that the design is totally revamped indeed. Better to wait then.

Comment by Andy Fingerhut [ 04/Jul/15 1:21 PM ]

Alex, just a note that the Java method getLoopbackAddress [1] appears to have been added with Java 1.7, so your patches that use that method do not compile with Java 1.6. If the plan was for the next release of Clojure to drop support for Java 1.6 anyway, then no worries.

[1] http://docs.oracle.com/javase/7/docs/api/java/net/InetAddress.html#getLoopbackAddress%28%29





[CLJ-1680] quot and rem handle doubles badly Created: 24/Mar/15  Updated: 04/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.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.





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

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

Type: Defect Priority: Minor
Reporter: Eric Normand Assignee: Unassigned
Resolution: Unresolved Votes: 0
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-1647] infinite loop in 'partition' and 'partition-all' when 'step' or 'n' is not positive Created: 20/Jan/15  Updated: 03/Jul/15

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

Type: Defect Priority: Minor
Reporter: Kevin Woram Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: checkargs, newbie

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

 Description   

If you pass a non-positive value of 'n' or 'step' to partition, you get an infinite loop. Here are a few examples:

(partition 0 [1 2 3])
(partition 1 -1 [1 2 3])

To fix this, I recommend adding 'assert-args' to the appropriate places in partition and partition-all:

(assert-args
(pos? n) "n must be positive"
(pos? step) "step must be positive" )



 Comments   
Comment by Alex Miller [ 03/Feb/15 5:34 PM ]

Also see CLJ-764

Comment by Alex Miller [ 29/Apr/15 12:02 PM ]

Needs a perf check when done.

Comment by Kevin Woram [ 16/May/15 1:58 PM ]

patch file to fix clj-1647

Comment by Kevin Woram [ 16/May/15 2:19 PM ]

Since 'n' and 'step' remain unchanged from the initial function call through all of the recursive self-calls, I only need to verify that they are positive once, on the initial call.

I therefore created functions 'internal-partition' and 'internal-partition-all' whose implementations are identical to the current versions of 'partition' and 'partition-all'.

I then added preconditions that 'step' and 'n' must be positive to the 'partition' and 'partition-all' functions, and made them call 'internal-partition' and 'internal-partition-all' respectively to do the work.

Comment by Alex Miller [ 17/May/15 8:14 AM ]

There are a lot of unrelated whitespace changes in this patch - can you supply a smaller patch with only the change at issue? Also needs tests.

Comment by Kevin Woram [ 17/May/15 2:05 PM ]

I will supply a patch file without the whitespace changes.

I know there are some existing functionality tests for 'partition' and 'partition-all' in test_clojure\sequences.clj and test_clojure\transducers.clj. I don't think I need to add more functionality tests, but I think I should add:

1. Tests that verify that non-positive 'step' and 'n' parameters are rejected.
2. Tests that show that 'partition' and 'partition-all' performance has not degraded significantly.

Could you give me some guidance on how to develop and add these tests?

Comment by Alex Miller [ 17/May/15 3:31 PM ]

You should add #1 to the patch. For #2, you can just do the timings before/after (criterium is a good tool for this) and put the results in the description.

Comment by Kevin Woram [ 22/May/15 4:04 PM ]

I have coded up the tests for #1 and taken some 'before' timings for #2 using criterium.

I have been stumped by a problem for hours now and I need to get some help. I made my changes to 'partition' and 'partition-all' in core.clj and then did 'mvn package' to build the jar. I executed 'target>java -cp clojure-1.7.0-master-SNAPSHOT.jar clojure.main' to test out my patched version of clojure interactively. The (source ...) function shows that my source changes for both 'partition' and 'partition-all' are in place. My change to 'partition-all' seems to be working but my change to 'partition' is not. As far as I can tell, they should both throw an AssertionError with the input parameters I am providing.

Any help would be greatly appreciated.

user=> (source partition)
(defn partition
"Returns a lazy sequence of lists of n items each, at offsets step
apart. If step is not supplied, defaults to n, i.e. the partitions
do not overlap. If a pad collection is supplied, use its elements as
necessary to complete last partition upto n items. In case there are
not enough padding elements, return a partition with less than n items."
{:added "1.0"
:static true}
([n coll]
{:pre [(pos? n)]}
(partition n n coll))
([n step coll]
{:pre [(pos? n) (pos? step)]}
(internal-partition n step coll))
([n step pad coll]
{:pre [(pos? n) (pos? step)]}
(internal-partition n step pad coll)))
nil
user=> (partition -1 [1 2 3])
()
user=> (source partition-all)
(defn partition-all
"Returns a lazy sequence of lists like partition, but may include
partitions with fewer than n items at the end. Returns a stateful
transducer when no collection is provided."
{:added "1.2"
:static true}
([^long n]
(internal-partition-all n))
([n coll]
(partition-all n n coll))
([n step coll]
{:pre [(pos? n) (pos? step)]}
(internal-partition-all n step coll)))
nil
user=> (partition-all -1 [1 2 3])
AssertionError Assert failed: (pos? n) clojure.core/partition-all (core.clj:6993)

Comment by Alex Miller [ 22/May/15 4:47 PM ]

Did you mvn clean? Or rm target?

Comment by Kevin Woram [ 24/May/15 11:46 PM ]

Yes, I did mvn clean and verified that clojure-1.7.0-master-SNAPSHOT.jar had the expected date-time stamp before doing the interactive test. I even went so far as to retrace my steps on my Macbook on the theory that maybe there was a Windows-specific build problem.

My change to partition-all works as expected but my change to partition does not. However, if I copy the result of the call to (source partition) and execute it (replacing clojure.core/partition with user/partition), user/partition works as expected. I don't understand why my change to clojure.core/partition isn't taking effect.

Comment by Andy Fingerhut [ 25/May/15 1:27 AM ]

Kevin, I do not know the history of your Clojure source tree, but if you ever ran 'ant' in it, that creates jar files in the root directory, whereas 'mvn package' creates them in the target directory. It wasn't clear from your longer comment above whether the 'java -cp ...' command you ran pointed at the one in the target directory. That may not be the cause of the issue you are seeing, but I don't yet have any guesses what else it could be.





[CLJ-1449] Add clojure.string functions for portability to ClojureScript Created: 19/Jun/14  Updated: 03/Jul/15

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: Nola Stowe
Resolution: Unresolved Votes: 25
Labels: string

Attachments: Text File clj-1449-more-v1.patch    
Patch: Code
Approval: Triaged

 Description   

It would be useful if a few common functions from Java's String were available as Clojure functions for the purposes of increasing portability to other Clojure platforms like ClojureScript.

The functions below also cover the vast majority of cases where Clojure users currently drop into Java interop for String calls - this tends to be an issue for discoverability and learning. While the goal of this ticket is increased portability, improving that is a nice secondary benefit.

Proposed clojure.string fn java.lang.String method
index-of indexOf
last-index-of lastIndexOf
starts-with? startsWith
ends-with? endsWith
includes? contains

Patch: Patch clj-1449-more-v1.patch is a good start, but needs the following additional work:

1) Update function names per the description here.
2) Per the instructions at the top of the clojure.string ns, functions should take the broader CharSequence interface instead of String. Similar to existing clojure.string functions, you will need to provide a CharSequence implementation while also calling into the String functions when you actually have a String.
3) Consider return type hints - I'm not sure they're necessary here, but I would examine the bytecode in typical calling situations to see if it would be helpful.
4) The new functions need new tests.
5) Check performance implications of the new versions vs the old with a good tool (like criterium). It would be good to know what the difference in perf actually is - some hit is worth it for a portable target.



 Comments   
Comment by Alex Miller [ 19/Jun/14 12:53 PM ]

Re substring, there is a clojure.core/subs for this (predates the string ns I believe).

clojure.core/subs
([s start] [s start end])
Returns the substring of s beginning at start inclusive, and ending
at end (defaults to length of string), exclusive.

Comment by Jozef Wagner [ 20/Jun/14 3:21 AM ]

As strings are collection of characters, you can use Clojure's sequence facilities to achieve such functionality:

user=> (= (first "asdf") \a)
true
user=> (= (last "asdf") \a)
false
Comment by Alex Miller [ 20/Jun/14 8:33 AM ]

Jozef, String.startsWith() checks for a prefix string, not just a prefix char.

Comment by Bozhidar Batsov [ 20/Jun/14 9:42 AM ]

Re substring, I know about subs, but it seems very odd that it's not in the string ns. After all most people will likely look for string-related functionality in clojure.string. I think it'd be best if `subs` was added to clojure.string and clojure.core/subs was deprecated.

Comment by Pierre Masci [ 01/Aug/14 5:27 AM ]

Hi, I was thinking the same about starts-with and .ends-with, as well as (.indexOf s "c") and (.lastIndexOf "c").

I read the whole Java String API recently, and these 4 functions seem to be the only ones that don't have an equivalent in Clojure.
It would be nice to have them.

Andy Fingerhut who maintains the Clojure Cheatsheet told me: "I maintain the cheatsheet, and I put .indexOf and .lastIndexOf on there since they are probably the most common thing I saw asked about that is in the Java API but not the Clojure API, for strings."
Which shows that there is a demand.

Because Clojure is being hosted on several platforms, and might be hosted on more in the future, I think these functions should be part of the de-facto ecosystem.

Comment by Andy Fingerhut [ 30/Aug/14 3:39 PM ]

Updating summary line and description to add contains? as well. I can back this off if it changes your mind about triaging it, Alex.

Comment by Andy Fingerhut [ 30/Aug/14 3:40 PM ]

Patch clj-1449-basic-v1.patch dated Aug 30 2014 adds starts-with? ends-with? contains? functions to clojure.string.

Patch clj-1449-more-v1.patch is the same, except it also replaces several Java method calls with calls to these Clojure functions.

Comment by Andy Fingerhut [ 05/Sep/14 1:02 PM ]

Patch clj-1449-basic-v1.patch dated Sep 5 2014 is identical to the patch I added recently called clj-1149-basic-v1.patch. It is simply renamed without the typo'd ticket number in the file name.

Comment by Yehonathan Sharvit [ 02/Dec/14 3:09 PM ]

What about an implementation that works also in cljs?

Comment by Bozhidar Batsov [ 02/Dec/14 3:11 PM ]

Once this is added to Clojure it will be implemented in ClojureScript as well.

Comment by Yehonathan Sharvit [ 02/Dec/14 3:22 PM ]

Great! Any idea when it will be added to Clojure?
Also, will it be automatically added to Clojurescript or someone will have to write a particular code for it.
The suggested patch relies on Java so I am curious to understand who is going to port the patch to cljs.

Comment by Bozhidar Batsov [ 02/Dec/14 3:27 PM ]

No idea when/if this will get merged. Upvote the ticket to improve the odds of this happening sooner.
Someone on the ClojureScript team will have to implement this in terms of JavaScript.

Comment by Alex Miller [ 02/Dec/14 4:01 PM ]

Some things that would be helpful:

1) It would be better to combine the two patches into a single patch - I think changing current uses into new users is a good thing to include. Also, please keep track of the "current" patch in the description.
2) Patch needs tests.
3) Per the instructions at the top of the clojure.string ns (and the rest of the functions), the majority of these functions are implemented to take the broader CharSequence interface. Similar to those implementations, you will need to provide a CharSequence implementation while also calling into the String functions when you actually have a String.
4) Consider return type hints - I'm not sure they're necessary here, but I would examine bytecode for typical calling situations to see if it would be helpful.
5) Check performance implications of the new versions vs the old with a good tool (like criterium). You've put an additional var invocation and (soon) type check in the calling path for these functions. I think providing a portable target is worth a small cost, but it would be good to know what the cost actually is.

I don't expect we will look at this until after 1.7 is released.

Comment by Andy Fingerhut [ 02/Dec/14 8:05 PM ]

Alex, all your comments make sense.

If you think a ready-and-waiting patch that does those things would improve the odds of the ticket being vetted by Rich, please let us know.

My guess is that his decision will be based upon the description, not any proposed patches. If that is your belief also, I'll wait until he makes that decision before working on a patch. Of course, any other contributor is welcome to work on one if they like.

Comment by Alex Miller [ 02/Dec/14 8:40 PM ]

Well nothing is certain of course, but I keep a special report of things I've "screened" prior to vetting that makes possible moving something straight from Triaged all the way through into Screened/Ok when Rich is able to look at them. This is a good candidate if things were in pristine condition.

That said, I don't know whether Rich will approve it or not, so it's up to you. I think the argument for portability is a strong one and complements the feature expression.

Comment by Alex Miller [ 14/May/15 8:55 AM ]

I'd love to have a really high-quality patch on this ticket to consider for 1.8 that took into account my comments above.

Also, it occurred to me that I don't think this should be called "contains?", overlapping the core function with a different meaning (contains value vs contains key). Maybe "includes?"?

Comment by Andy Fingerhut [ 14/May/15 9:14 AM ]

clojure.string already has 2 name conflicts with clojure.core, for which most people probably do something like (require '[clojure.string :as str]) to avoid that:

user=> (use 'clojure.string)
WARNING: reverse already refers to: #'clojure.core/reverse in namespace: user, being replaced by: #'clojure.string/reverse
WARNING: replace already refers to: #'clojure.core/replace in namespace: user, being replaced by: #'clojure.string/replace
nil
Comment by Alex Miller [ 14/May/15 10:05 AM ]

I'm not concerned about overlapping the name. I'm concerned about overlapping it and meaning something different, particularly vs one of the most confusing functions in core already. clojure.core/contains? is better than linear time key search. clojure.string/whatever will be a linear time subsequence match.

Comment by Alex Miller [ 14/May/15 10:18 AM ]

Ruby uses "include?" for this.

Comment by Devin Walters [ 19/May/15 4:56 PM ]

I agree with Alex's comment about the overlap. Personally, I prefer the way "includes?" reads over "include?", but IMO either one is better than adding to the "contains?" confusion.

Comment by Bozhidar Batsov [ 20/May/15 12:10 AM ]

I'm fine with `includes?`. Ruby is famous for the bad English used in its core library.

Comment by Andrew Rosa [ 17/Jun/15 12:29 PM ]

Andy Fingerhut, since you are the author of the original patch do you desire to continue the work on it? If you prefer (or even need) to focus on different stuff, I would like to tackle this issue.

Comment by Andy Fingerhut [ 17/Jun/15 1:08 PM ]

Andrew Rosa, I am perfectly happy if someone else wants to work on a revised patch for this. If you are interested, go for it! And from Alex's comments, I believe my initial patch covers such a small fraction of what he wants, do not worry about keeping my name associated with any patch you submit – if yours is accepted, my patch will not have helped you in getting there.

Comment by Nola Stowe [ 03/Jul/15 9:01 AM ]

Andrew Rosa, I am interested on working on this. Are you currently working it?

Comment by Andrew Rosa [ 03/Jul/15 12:08 PM ]

Thanks Andy Fingerhut, didn't saw the answer here. I'm happy that Nola Stowe could take this one.





[CLJ-1774] Field access on typed record does not preserve type Created: 02/Jul/15  Updated: 03/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Michael Blume Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: defrecord, reflection, typehints


 Description   
(ns field-test.core
  (:import [java.util UUID]))

(defrecord UUIDWrapper [^UUID uuid])

(defn unwrap [^UUIDWrapper w]
  (.-uuid w)) ; <- No reflection

(defn get-lower-bits [^UUIDWrapper w]
  (-> w .-uuid .getLeastSignificantBits)) ; <- Reflection :(

The compiler seems to have all the information it needs, but lein check prints

Reflection warning, field_test/core.clj:10:3 - reference to field getLeastSignificantBits on java.lang.Object can't be resolved.

(test case also at https://github.com/MichaelBlume/field-test)



 Comments   
Comment by Alex Miller [ 02/Jul/15 4:31 PM ]

afaik, that ^UUID type hint on the record field doesn't do anything. The record field will be of type Object (only ^double and ^long affect the actual field type).

Perhaps more importantly, it is bad form to use Java interop to retrieve the field values of a record. Keyword access for that is preferred.

Comment by Nicola Mometto [ 03/Jul/15 4:48 AM ]

The same issue applies for deftypes where keyword access is not an option

Comment by Alex Miller [ 03/Jul/15 12:17 PM ]

Per http://clojure.org/datatypes: "You should always program to protocols or interfaces -
datatypes cannot expose methods not in their protocols or interfaces"

Along those lines, usually for deftypes, I gen an interface with the proper types if necessary, then have the deftype implement the interface to expose the field.

Also per http://clojure.org/datatypes:

"note that currently a type hint of a non-primitive type will not be used to constrain the field type nor the constructor arg, but will be used to optimize its use in the class methods" - that is, inside a method implemented on the record/type, referring to the field should have the right hint. So in the example above, if unwrap was an interface or protocol implementation method on the record, and you referred to the field, you should expect the hint to be utilized in that scenario.

So, my contention would be that all of the behavior described in this ticket should be expected based on the design, which is why I've reclassified this as an enhancement, not a defect.





[CLJ-1060] 'list*' returns not a list Created: 03/Sep/12  Updated: 02/Jul/15

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

Type: Defect Priority: Trivial
Reporter: Andrei Zhlobich Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: ft

Attachments: File list-star-docstring.diff     File list-star-fix.diff    
Patch: Code
Approval: Triaged

 Description   

Function 'list*' returns sequence, but not a list.
It is a bit confusing.

user=> (list? (list* 1 '(2 3)))
false

Approach: Change the doc string to say that it returns a seq, not a list.

Patch: list-star-docstring.diff



 Comments   
Comment by Stuart Halloway [ 17/Sep/12 6:52 AM ]

should the docstring for list* change to say it returns a seq?

Comment by Timothy Baldridge [ 27/Nov/12 11:58 AM ]

Is there a reason why we can't have list* actually return a list? The cost of creating a list vs a cons is negligible.

Comment by Marek Srank [ 04/Jan/13 2:02 PM ]

The question is what to do with the one-argument case of list*, because in cases like: (list* {:a 1 :b 2}) it doesn't return IPersistentList as well. I propose just applying 'list'.

I added patch 'list-star-fix.diff' (dated 04/Jan/2013) with Cons implementing IPersistentList and doing (apply list args) in one-argument case of list*. To be able to use 'apply' in list* I had to declare it before the definition of list* in the source code. The apply itself also uses list*, but luckily not the one-argument version of list*, so it should be safe... The patch also contains simple unit tests.

Discussion is also here: https://groups.google.com/forum/#!topic/clojure/co8lcKymfi8

Comment by Michał Marczyk [ 04/Jan/13 4:11 PM ]

(apply list args) would make (list* (range)) hang, where currently it returns a partially-realized lazy seq. Also, even for non-lazy seqs – possibly lists themselves – it would always build a new list from scratch, right?

Also, if I'm reading the patch correctly, it would make 2+-ary list* overloads and cons return lists – that is, IPersistentList instances – always (Conses would now be lists), but repeatedly calling next on such a list might eventually produce a non-list. The only way around that would be to make all seqs into IPersistentLists – that doesn't seem desirable at first glance...?

On a side note, for the case where the final "args" argument is in fact a list, we already have a "prepend many items" function, namely conj. list* currently acts as the vararg variant of cons (in line with Lisp tradition); I'm actually quite happy with that.

Comment by Michał Marczyk [ 04/Jan/13 4:19 PM ]

I'm in favour of the "list" -> "seq" tweak to the docstring though, assuming impl remains unchanged.

Comment by Marek Srank [ 04/Jan/13 6:13 PM ]

Yep, these are all valid points, thanks! I see this as a question whether the list* function is a list constructor or not. If yes (and it would be possible to implement it in a satisfactory way), it should probably return a list.

We could avoid building a new list by sth like:

(if (list? args)
  args
  (apply list args))

(btw, 'vec' also creates a new vector even when the argument itself is a vector)

The contract of next seems to be to return a seq, not a list - its docstring reads: "Returns a seq of the items after the first. Calls seq on its argument. If there are no more items, returns nil."

Btw, in some Lisp/Scheme impls I checked, cons seems to be a list as well. E.g. in CLisp (and similar in Guile and Racket):

> (listp (cons 2 '()))
T
> (listp (list* 1 2 '()))
T
Comment by Steve Miner [ 26/Feb/15 3:11 PM ]

I bump into this every once in a while and it bothers my pedantic side.

I think it's too late to change the implementation of `list*`. There's a risk of breaking existing code (dealing with lazy-seqs, etc.) It would be good to change the doc string to say it returns a seq, not a list.

But the real issue is the name of the function implies that it will return a list. You could deprecate `list*` (but keep it forever for backwards compatibility.) A better name for the same implementation might be `seq*`.

Comment by Andy Fingerhut [ 02/Jul/15 3:13 PM ]

To Andy Sheldon, author of patch list-star-docstring.diff: Clojure only accepts patches written by those who have signed a contributor agreement. If you were interested in doing that, details on how are here: http://clojure.org/contributing

Comment by Andy Sheldon [ 02/Jul/15 8:34 PM ]

@AndyFingerhut - Thanks for the reminder. Signed, sealed, delivered.





[CLJ-1769] Docstrings for *' and +' refer to * and + instead of *' and +' Created: 28/Jun/15  Updated: 02/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: Trivial
Reporter: Mark Simpson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, ft

Attachments: Text File docstringfix.patch    
Patch: Code
Approval: Triaged

 Description   

The docstrings for *' and +' refer to the behavior of * and + if they are passed no arguments. The docstrings should refer to the behavior of *' and +' instead.



 Comments   
Comment by Andy Fingerhut [ 02/Jul/15 3:49 PM ]

Mark, your patch "patch.txt" is not in the expected format for a patch, and please use a file name ending with ".diff" or ".patch", for the convenience of patch reviewers. See this link for instructions on creating a patch in the expected format: http://dev.clojure.org/display/community/Developing+Patches

Comment by Mark Simpson [ 02/Jul/15 4:36 PM ]

Sorry about that. Hopefully I got things right this time.





[CLJ-1566] Documentation for clojure.core/require does not document :rename Created: 16/Oct/14  Updated: 02/Jul/15

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

Type: Defect Priority: Minor
Reporter: James Laver Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File refer.patch    
Patch: Code

 Description   

By contrast, clojure.core/use does mention :rename.

I attach a patch



 Comments   
Comment by Andy Fingerhut [ 16/Oct/14 1:33 PM ]

James, your patch removes any mention of the :all keyword, and that keyword is not mentioned in the doc string for clojure.core/refer.

I haven't checked whether refer can take :all as an argument, but clojure.core/require definitely can.

Comment by James Laver [ 16/Oct/14 1:39 PM ]

Ah, you're quite right. Fixed now. See updated patch in a sec.

Comment by Andy Fingerhut [ 16/Oct/14 8:16 PM ]

For sake of reduced confusion, it would be better if you could either name your patches differently, or delete obsolete ones with identical names as later ones. JIRA allows multiple patches to have the same names, without replacing the earlier ones.

Comment by James Laver [ 17/Oct/14 12:44 AM ]

Okay, that's done. The JIRA interface is a bit tedious in places.

Comment by Bozhidar Batsov [ 19/Oct/14 1:34 AM ]

Seems to me the sentence should end with a dot.

Comment by James Laver [ 19/Oct/14 4:36 AM ]

Added a dot.

Comment by Andy Fingerhut [ 02/Jul/15 3:56 PM ]

James, I do not know whether this change is of interest to the Clojure core team, but see this link for instructions on creating patches in the expected format (yours is not in that format): http://dev.clojure.org/display/community/Developing+Patches





[CLJ-1626] ns macro: compare ns name during macroexpansion. Created: 23/Dec/14  Updated: 02/Jul/15

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

Type: Enhancement Priority: Trivial
Reporter: Petr Gladkikh Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: File compare-ns-name-at-macroexpansion.diff    

 Description   

Macroexpansion of 'ns' produces 'if' form that is executed at runtime. However comparison can be done during macroexpansion phase producing clearer resulting form in most cases.

Patch for suggested change is in attachment.



 Comments   
Comment by Andy Fingerhut [ 02/Jul/15 3:53 PM ]

Petr, I do not know whether this change is of interest to the Clojure core team or not. I do know that it is not in the expected format for a patch. See this link for instructions on creating a patch in the expected format: http://dev.clojure.org/display/community/Developing+Patches

Same comment applies to your patch for CLJ-1628





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

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

Type: Enhancement Priority: Major
Reporter: Abe Fettig Assignee: Unassigned
Resolution: Unresolved Votes: 3
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!





[CLJ-1773] Support for REPL commands for tooling Created: 01/Jul/15  Updated: 01/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Unresolved Votes: 0
Labels: repl

Approval: Incomplete

 Description   

Per http://dev.clojure.org/display/design/Socket+Server+REPL, want to enhance repl to support "commands" useful for nested repls or for parallel tooling repls communicating over sockets (CLJ-1671).

Commands are defined as keywords in the "repl" namespace. The REPL will trap these after reading but before evaluation. Currently this is a closed set, but perhaps it could be open - the server socket repl could then install new ones if desired when repl is invoked.

Commands:

  • :repl/quit - same as Ctrl-D but works in terminal environments where that's not feasible. Allows for backing out of a nested REPL.
  • :repl/push - push the current repl "state" (tbd what that is, but at least ns) to a stateful map in the runtime. Returns coordinates that can be used to retrieve it elsewhere
  • :repl/pull <coords> - retrieves the repl state defined by the coordinates

In the tooling scenario, it is expected that there are two repls - the client repl and the tooling repl. The tooling can send :repl/push over the client repl before startup and retrieve the coordinates (which don't change). Then the tooling repl can call :repl/pull at any time to retrieve the state of the client repl.






[CLJ-1772] Spelling mistake in clojure.test/use-fixtures Created: 01/Jul/15  Updated: 01/Jul/15

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

Type: Defect Priority: Trivial
Reporter: Daniel Compton Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, ft

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

 Description   

Part of the docstring for `use-fixtures` is:

individually, while:once wraps the whole run in a single function.

this should be

individually, while :once wraps the whole run in a single function.

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 01/Jul/15 12:42 AM ]

If you can get me a patch, happy to pre-screen for next release.

Comment by Daniel Compton [ 01/Jul/15 12:43 AM ]

2015-07-01 17:43:02





[CLJ-1761] clojure.core/run! does not always return nil Created: 17/Jun/15  Updated: 30/Jun/15

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

Type: Defect Priority: Major
Reporter: Jonas Enlund Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File clj-1761.patch     Text File clj-1761-with-tests.patch    
Patch: Code and Test
Approval: Screened

 Description   

According to the documentation clojure.core/run! should return nil. This is not the case as seen by the following examples:

user=> (run! identity [1])
1
user=> (run! reduced (range))
0

Approach: return 'nil'

Patch: clj-1761-with-tests.patch

Screened by: Alex Miller






[CLJ-1063] Missing dissoc-in Created: 07/Sep/12  Updated: 30/Jun/15

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

Type: Enhancement Priority: Major
Reporter: Gunnar Völkel Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: None

Attachments: File 001-dissoc-in.diff     Text File clj-1063-add-dissoc-in-patch-v2.txt    
Patch: Code and Test
Approval: Triaged

 Description   

There is no clojure.core/dissoc-in although there is an assoc-in.
It is correct that dissoc-in can be build with update-in and dissoc but this is an argument against assoc-in as well.
When a shortcut for assoc-in is provided, there should also be one for dissoc-in for consistency reasons.
Implementation is analogical to assoc-in.



 Comments   
Comment by Andy Fingerhut [ 13/Sep/12 2:17 PM ]

Patch clj-1063-add-dissoc-in-patch-v2.txt dated Sep 13 2012 supersedes 001-dissoc-in.diff dated Sep 7 2012. It fixes a typo (missing final " in doc string), and adds a test case for the new function.

Comment by Nicola Mometto [ 13/Sep/12 2:27 PM ]

Thanks for the fix Andy

Comment by Andy Fingerhut [ 14/Sep/12 8:24 PM ]

This proposed dissoc-in should be compared with the one in clojure.core.incubator which I just happened across. I see they look different, but haven't examined to see if there are any behavior differences.

https://github.com/clojure/core.incubator/blob/master/src/main/clojure/clojure/core/incubator.clj

Comment by Nicola Mometto [ 15/Sep/12 6:43 AM ]

dissoc-in in clojure.core.incubator recursively removes empty maps

user=> (clojure.core.incubator/dissoc-in {:a {:b {:c 1}}} [:a :b :c])
{}

while the one in this patch doesn't (as I would expect)

user=> (dissoc-in {:a {:b {:c 1}}} [:a :b :c])
{:a {:b {}}}

Comment by Stuart Halloway [ 17/Sep/12 7:04 AM ]

Please do this work in the incubator.

Comment by Andrew Rosa [ 30/Jun/15 9:09 AM ]

Keeping the empty paths is really the most expected thing to do? I say, since both assoc-in/update-in create paths along the way this behavior looks to be the real "dual".

What kind of bad things could happen that nil punning does not deal well with it? Those issues would be too much obscure for dissoc-in user's point of view?





[CLJ-1771] Support for multiple key(s)-value pairs in assoc-in Created: 29/Jun/15  Updated: 30/Jun/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: 0
Labels: None
Environment:

All


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





Generated at Mon Jul 06 19:47:01 CDT 2015 using JIRA 4.4#649-r158309.