<< Back to previous view

[CLJ-1404] clojure.core/vals returns nil on an empty map instead of an empty sequence Created: 14/Apr/14  Updated: 14/Apr/14  Resolved: 14/Apr/14

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

Type: Defect Priority: Trivial
Reporter: Satshabad Khalsa Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

Is this a bug? maybe I just don't understand. The documentation says: Returns a sequence of the map's values. Is nil a sequence?

This caused an unexpected nil to propagate through a bunch of list processing stuff.



 Comments   
Comment by Alex Miller [ 14/Apr/14 10:54 PM ]

An empty sequence is represented by nil, so this is consistent. For example: (seq (range 0)) => nil





[CLJ-1397] exception testing broken over map Created: 01/Apr/14  Updated: 01/Apr/14  Resolved: 01/Apr/14

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

Type: Defect Priority: Major
Reporter: MG Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: test
Environment:

Linux ... 3.2.0-56-generic-pae #86-Ubuntu SMP ... i686 i686 i386 GNU/Linux



 Description   

Expected: Tests pass
Actual: Two tests fail
To reproduce, run the following test file:

(ns pe.test-test
(:require [clojure.test :refer :all]))
(defn throwexc [m] (throw (Exception. m)))
(defn throwass [m] (assert false m))
(defn nestexc [] (throwexc "exc"))
(defn nestass [] (throwass "ass"))
(defn nestmapexc [] (map throwexc '("a" "b" "c")))
(defn nestmapass [] (map throwass '("a" "b" "c")))
(deftest exceptions-and-assertions-test
(testing "throwing"
(is (thrown? Exception (throwexc "exc")))
(is (thrown? AssertionError (throwass "ass"))))
(testing "nesting"
(is (thrown? Exception (nestexc)))
(is (thrown? AssertionError (nestass))))
(testing "nesting over map"
(is (thrown? Exception (nestmapexc)))
(is (thrown? AssertionError (nestmapass)))))



 Comments   
Comment by MG [ 01/Apr/14 7:25 AM ]

Clarification: The two assertions in "nesting over map" fails, the other four assertions succeed.

Comment by Nicola Mometto [ 01/Apr/14 7:30 AM ]

map is lazy, the exception is never thrown because the sequence is never realized.
either wrap the map in a dorun or use a doseq instead of a map.
map should not be used for side-effecting





[CLJ-1396] Bad link Created: 01/Apr/14  Updated: 01/Apr/14  Resolved: 01/Apr/14

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

Type: Defect Priority: Trivial
Reporter: MG Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

http://richhickey.github.io/clojure/



 Description   

http://richhickey.github.io/clojure/
The issue tracker link points to Assembla.



 Comments   
Comment by Nicola Mometto [ 01/Apr/14 7:31 AM ]

That site is deprecated and so is the repo that's hosting it.
This is the updated one http://clojure.github.io/clojure/

Comment by Alex Miller [ 01/Apr/14 7:45 AM ]

Nicola, I agree with your assessment here but only screeners should be closing tickets, thanks.

Comment by Nicola Mometto [ 01/Apr/14 7:50 AM ]

Alex, sorry, I didn't know this, I will refrain from doing so in the future.





[CLJ-1395] get should deref delay, refs atoms etc Created: 01/Apr/14  Updated: 01/Apr/14  Resolved: 01/Apr/14

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

Type: Enhancement Priority: Minor
Reporter: Gerrit Jansen van Vuuren Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

(get (delay {:a 1}) :a)
;; nil

(get {:a 1} :a)
;; 1

The above situation can happen easily and there is no way to refactor or check that all code doing gets is in fact doing deref before doing get. Given that everything is dynamic typing, changing a single value from a map to delay or ref can make large parts of the code fail silently on nil for gets.

get would be more consistent (with what is expected of it) if it was to check for refs, atoms and delays and do a deref.



 Comments   
Comment by Alex Miller [ 01/Apr/14 7:43 AM ]

I think this goes beyond what get should do. In regards to silent failure, that is covered by CLJ-1107 which would cause this to throw an exception instead.





[CLJ-1393] clojure.string/trim doesn't trim null character Created: 28/Mar/14  Updated: 28/Mar/14  Resolved: 28/Mar/14

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

Type: Defect Priority: Minor
Reporter: Ryan Fowler Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: string


 Description   

CLJ-935 changed clojure.string/trim to not trim all the characters less than or equal to \u0020 as Java does.

I noticed this because base64 uses null characters to pad the end of encoding blocks.

Clojure 1.6.0's trim leaves the null character in:
user=> (.length (clojure.string/trim "\u0000"))
1

java.lang.String's trim takes it out:
user=> (.length (.trim "\u0000"))
0

Here are the first 21 unicode characters and what Character/isWhitespace says about them.

(dotimes [n 0x20] (printf "
u%04x - %b\n" n (Character/isWhitespace n)))
\u0000 - false
\u0001 - false
\u0002 - false
\u0003 - false
\u0004 - false
\u0005 - false
\u0006 - false
\u0007 - false
\u0008 - false
\u0009 - true
\u000a - true
\u000b - true
\u000c - true
\u000d - true
\u000e - false
\u000f - false
\u0010 - false
\u0011 - false
\u0012 - false
\u0013 - false
\u0014 - false
\u0015 - false
\u0016 - false
\u0017 - false
\u0018 - false
\u0019 - false
\u001a - false
\u001b - false
\u001c - true
\u001d - true
\u001e - true
\u001f - true



 Comments   
Comment by Alex Miller [ 28/Mar/14 12:27 PM ]

The choice was made in CLJ-935 to consistently define whitespace as Character.isWhitespace() across trim, triml, and trimr. There are many possible ways to define "space" (at least two as we see here). If your trimming needs differ from the standard library, then you'll probably need to define your own functions to trim your data. You can still use Java interop to call String.trim() directly if that happens to match your needs.

Comment by Ryan Fowler [ 28/Mar/14 1:03 PM ]

Indeed, it's an easy workaround to use Java interop once you figure out what your problem is.

It's just unintuitive that the character generally used for string termination isn't trimmed by clojure.string/trim.





[CLJ-1392] AOT vs "var already exists warning" results in NPE Created: 26/Mar/14  Updated: 26/Mar/14  Resolved: 26/Mar/14

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

Type: Defect Priority: Major
Reporter: Aaron Cohen Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: aot, compiler


 Description   

I just saw this thread on the cascalog list: https://groups.google.com/forum/#!topic/cascalog-user/Pe5QIpmU0vA

Apparently the "WARNING: some? already refers to: #'clojure.core/some? in namespace: jackknife.seq, being replaced by: #'jackknife.seq/some?" results in a NullPointerException when trying to AOT a namespace that results in the message being output.

Reproducer:

1) lein new aotFail
2) Edit project.clj
;add as appropriate
:aot :all
:dependencies [[org.clojure/clojure "1.6.0"]
[cascalog "2.0.0"]]
2) Add "(:use cascalog.api)" to the ns block of src/aotFail/core.clj
3) lein compile

Output:

Compiling aotFail.core
WARNING: some? already refers to: #'clojure.core/some? in namespace: jackknife.seq, being replaced by: #'jackknife.seq/some?
Exception in thread "main" java.lang.NullPointerException, compiling:(api.clj:1:1)
at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3558)
at clojure.lang.Compiler.compile1(Compiler.java:7226)
at clojure.lang.Compiler.compile1(Compiler.java:7216)
at clojure.lang.Compiler.compile(Compiler.java:7292)
at clojure.lang.RT.compile(RT.java:398)
at clojure.lang.RT.load(RT.java:438)
at clojure.lang.RT.load(RT.java:411)
at clojure.core$load$fn__5066.invoke(core.clj:5641)
at clojure.core$load.doInvoke(core.clj:5640)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at clojure.core$load_one.invoke(core.clj:5446)
at clojure.core$load_lib$fn__5015.invoke(core.clj:5486)
at clojure.core$load_lib.doInvoke(core.clj:5485)
at clojure.lang.RestFn.applyTo(RestFn.java:142)
at clojure.core$apply.invoke(core.clj:626)
at clojure.core$load_libs.doInvoke(core.clj:5528)
at clojure.lang.RestFn.applyTo(RestFn.java:137)
at clojure.core$apply.invoke(core.clj:628)
at clojure.core$use.doInvoke(core.clj:5618)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at cascalog2.core$loading_4958auto_.invoke(core.clj:1)
at clojure.lang.AFn.applyToHelper(AFn.java:152)
at clojure.lang.AFn.applyTo(AFn.java:144)
at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3553)
at clojure.lang.Compiler.compile1(Compiler.java:7226)
at clojure.lang.Compiler.compile1(Compiler.java:7216)
at clojure.lang.Compiler.compile(Compiler.java:7292)
at clojure.lang.RT.compile(RT.java:398)
at clojure.lang.RT.load(RT.java:438)
at clojure.lang.RT.load(RT.java:411)
at clojure.core$load$fn__5066.invoke(core.clj:5641)
at clojure.core$load.doInvoke(core.clj:5640)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at clojure.core$load_one.invoke(core.clj:5446)
at clojure.core$compile$fn__5071.invoke(core.clj:5652)
at clojure.core$compile.invoke(core.clj:5651)
at user$eval19.invoke(form-init2092370125048380878.clj:1)
at clojure.lang.Compiler.eval(Compiler.java:6703)
at clojure.lang.Compiler.eval(Compiler.java:6693)
at clojure.lang.Compiler.load(Compiler.java:7130)
at clojure.lang.Compiler.loadFile(Compiler.java:7086)
at clojure.main$load_script.invoke(main.clj:274)
at clojure.main$init_opt.invoke(main.clj:279)
at clojure.main$initialize.invoke(main.clj:307)
at clojure.main$null_opt.invoke(main.clj:342)
at clojure.main$main.doInvoke(main.clj:420)
at clojure.lang.RestFn.invoke(RestFn.java:421)
at clojure.lang.Var.invoke(Var.java:383)
at clojure.lang.AFn.applyToHelper(AFn.java:156)
at clojure.lang.Var.applyTo(Var.java:700)
at clojure.main.main(main.java:37)
Caused by: java.lang.NullPointerException
at clojure.lang.Compiler$ObjExpr.emitVar(Compiler.java:4944)
at clojure.lang.Compiler$DefExpr.emit(Compiler.java:437)
at clojure.lang.Compiler.compile1(Compiler.java:7225)
at clojure.lang.Compiler.compile(Compiler.java:7292)
at clojure.lang.RT.compile(RT.java:398)
at clojure.lang.RT.load(RT.java:438)
at clojure.lang.RT.load(RT.java:411)
at clojure.core$load$fn__5066.invoke(core.clj:5641)
at clojure.core$load.doInvoke(core.clj:5640)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at clojure.core$load_one.invoke(core.clj:5446)
at clojure.core$load_lib$fn__5015.invoke(core.clj:5486)
at clojure.core$load_lib.doInvoke(core.clj:5485)
at clojure.lang.RestFn.applyTo(RestFn.java:142)
at clojure.core$apply.invoke(core.clj:626)
at clojure.core$load_libs.doInvoke(core.clj:5524)
at clojure.lang.RestFn.applyTo(RestFn.java:137)
at clojure.core$apply.invoke(core.clj:628)
at clojure.core$use.doInvoke(core.clj:5618)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at cascalog.api$loading_4958auto_.invoke(api.clj:1)
at clojure.lang.AFn.applyToHelper(AFn.java:152)
at clojure.lang.AFn.applyTo(AFn.java:144)
at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3553)



 Comments   
Comment by Nicola Mometto [ 26/Mar/14 12:42 PM ]

Duplicate of http://dev.clojure.org/jira/browse/CLJ-1241





[CLJ-1387] reduce-kv on hash map ignores reduced objects in large maps Created: 18/Mar/14  Updated: 22/Mar/14  Resolved: 22/Mar/14

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

Type: Defect Priority: Major
Reporter: Jozef Wagner Assignee: Jozef Wagner
Resolution: Completed Votes: 0
Labels: None
Environment:

JRE 7


Attachments: File clj-1387.diff     File clj-1387-v2.diff     File clj-1387-v3.diff    
Patch: Code and Test
Approval: Ok

 Description   

Larger hash maps have nested INodes. As kvreduce implementations in INodes dereference reduced objects, parent INodes continue to reduce.

user=> (defn test-reduce-kv [m] (reduce-kv (fn [_ k v] (when (== 1 k) (reduced :foo))) nil m))
#'user/test-reduce-kv
user=> (test-reduce-kv (zipmap (range 3) (range 3)))
:foo
user=> (test-reduce-kv (zipmap (range 300) (range 300)))
nil

Dereferencing reduced objects should happen only PersistentHashMap/kvreduce - intermediate nodes should pass the Reduced object along.

Patch: clj-1387-v3.diff
Screened-by:



 Comments   
Comment by Alex Miller [ 18/Mar/14 5:11 PM ]

I updated the patch to use a generative test that will try many combinations of map size and the reduced index to bail out on. This test failed before applying the source patch and passes with it.

Comment by Rich Hickey [ 21/Mar/14 7:33 AM ]

if(root != null){ - return root.kvreduce(f,init); + init = root.kvreduce(f,init); + if(RT.isReduced(init)) + return ((IDeref)init).deref(); }

Turns code that always had a return into code that sometimes does.

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

Added new version of patch that retains the return flow and doesn't fall through.





[CLJ-1382] Vector sort order should be specified sufficiently to embrace sort-by-juxt Created: 13/Mar/14  Updated: 15/Mar/14  Resolved: 15/Mar/14

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

Type: Enhancement Priority: Minor
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: data-structures, documentation, idiom


 Description   

Vectors of equal length sort in a way that seems natural – by
comparing their 0th elements, then their 1st, etc., until something
is different:

user> (def vv '(["c" 9] ["a" 100] ["a" 33] ["b" 8]))
#'user/vv
user> (sort vv)
(["a" 33] ["a" 100] ["b" 8] ["c" 9])

This property enables a blisteringly wonderful idiom for sorting
records by multiple keys:

user> (def mm (->> vv (map (fn [[p q]] {:p p :q q}))))
#'user/mm
user> (sort-by (juxt :p :q) mm)
({:p "a", :q 33} {:p "a", :q 100} {:p "b", :q 8} {:p "c", :q 9})

The sort-by-juxt idiom was described on briancarper.net[1], where it
was refined for Clojure 1.1 by Malcolm Sparks. Andy Fingerhut has
also written about it, thoroughly.[2]

Such lore gives it the odor of respectability, but the sort-by-juxt
idiom takes liberties beyond the documented behavior ("contract") of
vectors. APersistentVector indulges the idiom, but the clojure.org
Data Structures page does not say how vectors should compare.

The vector specification should be bolstered with enough traits of
vectors' sorting behavior to make the sort-by-juxt idiom safe to use
wherever Clojure might be implemented.

[1] http://briancarper.net/blog/488/sort-a-clojure-map-by-two-or-more-keys

[2] https://github.com/jafingerhut/thalia/blob/master/doc/other-topics/comparators.md



 Comments   
Comment by Alex Miller [ 13/Mar/14 9:52 PM ]

I don't understand what this ticket is asking for.

Comment by Andy Fingerhut [ 13/Mar/14 10:32 PM ]

It sounds like he is asking that the doc of clojure.core/compare say that vectors of equal length are compared in lexicographic order.

Comment by Phill Wolf [ 15/Mar/14 1:07 PM ]

"(sort-by (juxt" relies on a feature of vectors that the Clojure documentation does not guarantee. But using juxt in this way is part of the cultural fabric and helps make concise programs that "read like a definition" of the problem, to quote Halloway in "Programming Clojure". Therefore, let's document the sort order of equal-length vectors. I looked for this information on the Data Structures page, which has a section on vectors.

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

I added a line to the Vectors section on the Data Structures (http://clojure.org/data_structures) page: "Vectors are compared first by length, then each element is compared in order."





[CLJ-1377] java.lang.IllegalArgumentException: More than one matching method found on calling ExecutorService.submit from let instead of using Var. Created: 11/Mar/14  Updated: 12/Mar/14  Resolved: 11/Mar/14

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

Type: Defect Priority: Major
Reporter: Roy Varghese Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

Example from Joy Of Clojure doesn't work when Executor service pool is used in (let )

https://groups.google.com/forum/#!msg/clojure/asEXM2uHyxw/aK4rrKOKs1YJ



 Comments   
Comment by Alex Miller [ 11/Mar/14 3:47 PM ]

In the let case, the pool will be tagged with the proper type so the ambiguity is detected.

In the def case, the pool will be seen as an object and the compiler is just deferring to reflection at runtime to figure it out. If you turn on warn-on-reflection, you'll see a reflection warning in this case. Reflection is just picking the first one that matches in that case. If you type hinted the def case, you'd see the same error.

I don't think there is a bug here.

Comment by Roy Varghese [ 11/Mar/14 5:08 PM ]

Actually, I did turn on warn-on-reflection, but didn't see any messages.

I think this creates an element of surprise and uncertainty.

(def A (let [x (java.util.concurrent.Executors/newFixedThreadPool 5)] x))

How would (.submit A ...) behave?

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

When I ran the prior example, I saw a reflection warning.

In your new example, I would expect x to be typed as a ThreadExecutorPool, A to be typed as an Object, and .submit to get called reflectively (successfully) and produce a reflection warning. It's effectively no different than the def example in the post.

What are you suggesting is the bug and what should happen? Reflector could throw an error when it finds multiple matches but that would certainly break code that is currently working like the example you gave, so I doubt we would consider such a change at this point. There really is an ambiguity here, so I think a type hint is required to make it work unambiguously in either case.

Comment by Roy Varghese [ 12/Mar/14 10:49 AM ]

The bug is that type hints are required in one case, and not required in the other case because of implementation details, unless its documented in the language that (def) and (let) hold different types of Vars.

I the example above, I would expect A and x to refer to the same object, and thus contain the type information. Or not. Without reading the implementation, either case seems possible.

Agree, its a breaking change, but that's a different consideration.

Comment by Roy Varghese [ 12/Mar/14 10:50 AM ]

BTW..trying to fix with hints is what led to CLJ-1378.

Comment by Alex Miller [ 12/Mar/14 11:39 AM ]

x is locally bound to an object (there is no Var here) - the type information is on the local binding, inferred from the type of the expression. In the compiled form, the let expression does know the return type of the evaluated let (if there were multiple branches, there might be many possible return types). The def creates a Var that holds the result of evaluating the let. At compilation time, the type of the result of the let is unknown so is assumed to be Object.

So, x is a local binding and A is a Var. Even if they both refer to the same object, they do so in different contexts using different information. The type hinting and binding behavior in let is described on the special forms page http://clojure.org/special_forms. Remember too that Clojure is a dynamic language - while the Var A might hold a ThreadPoolExecutor instance now, it might hold something else later.

CLJ-1378 is useful - that's phrased more as a problem we can solve and there's a reasonable patch there.

Comment by Alex Miller [ 12/Mar/14 11:55 AM ]

"there is no Var here" == "there is no Var at this point"
"the let expression does know the return type" should have been "does NOT know"





[CLJ-1374] Make PersistentQueue implement List Created: 09/Mar/14  Updated: 31/Mar/14  Resolved: 31/Mar/14

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

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

Attachments: File clj-1374-1.diff    
Patch: Code and Test

 Description   

Most ordered Clojure collections like lists, vectors, and lazy seqs implement java.util.List, and thus .equals can be true between values of those types and other collections implementing java.util.List, like java.util.Vector and java.util.ArrayList.

Clojure PersistentQueue seems to be the odd man out here, in that it implements Collection but not List, and thus while it can be .equals to Clojure lists, vectors, and lazy seqs, it cannot be .equals to other collections implementing java.util.List.

user=> (instance? java.util.List '())
true
user=> (instance? java.util.List (lazy-seq))
true
user=> (instance? java.util.List [])
true
user=> (instance? java.util.List (vector-of :long))
true
user=> (instance? java.util.List clojure.lang.PersistentQueue/EMPTY)
false

user=> (= '() (java.util.ArrayList.))
true
user=> (= (lazy-seq) (java.util.ArrayList.))
true
user=> (= [] (java.util.ArrayList.))
true
user=> (= (vector-of :long) (java.util.ArrayList.))
true
user=> (= clojure.lang.PersistentQueue/EMPTY (java.util.ArrayList.))
false


 Comments   
Comment by Andy Fingerhut [ 09/Mar/14 6:29 PM ]

Patch clj-1374-1.diff is written assuming that CLJ-1372 patch clj-1372-2.diff or very similar has been committed, because of the tests modified, not because of the change of PersistentQueue to implement the java.util.List interface. I can update this patch as desired if that change does not go in.

Comment by Andy Fingerhut [ 09/Mar/14 6:33 PM ]

Ugh. The subject is definitely a duplicate of CLJ-1059, which I should have checked for before creating this ticket. I will compare patches to see how the approaches compare. Mine is probably a poor substitute for that one, but the tests I add may still be useful to keep in a patch for CLJ-1059.

Comment by Andy Fingerhut [ 31/Mar/14 5:29 PM ]

Problem description is a duplicate of CLJ-1059. Even the patch (independently developed) is nearly the same as the patch with a name beginning with "001" attached to CLJ-1059.





[CLJ-1370] ((println)) should throw CompilerException instead of NPE? Created: 06/Mar/14  Updated: 06/Mar/14  Resolved: 06/Mar/14

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

Type: Defect Priority: Minor
Reporter: The Alchemist Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: bug


 Description   

How to Reproduce

=> ((println))

NullPointerException   hello.core/eval4117 (NO_SOURCE_FILE:1)

What's Wrong?

I might be completely wrong, in which case don't hesitate to close this defect, but I wish this would throw a CompilerException with an IllegalArgumentException, just like ((nil)):

=> ((nil))
CompilerException java.lang.IllegalArgumentException: Can't call nil, compiling:(NO_SOURCE_PATH:1:2)


 Comments   
Comment by Alex Miller [ 06/Mar/14 12:37 PM ]

There is no compilation error here. The error occurs during evaluation.

user> (defn x [] ((println)))  ;; compiles just fine
#'user/x
user> (x)   ;; fails in evaluation
NullPointerException   user/x (NO_SOURCE_FILE:1)

The error is thrown when trying to evaluate (nil) where NullPointerException is a perfectly valid error.





[CLJ-1369] CLJ-738 is marked Closed is not implemented Created: 04/Mar/14  Updated: 04/Mar/14  Resolved: 04/Mar/14

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

Type: Defect Priority: Minor
Reporter: David Welte Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

OS X, Java 6



 Description   

CLJ-738 is marked Closed but the attached patch is has not been applied and both Clojure 1.5.1 and 1.6.0-beta2 exhibit the bad behavior listed in CLJ-738. The issue that CLJ-738 is that (<= (Double. Double/NaN) 1) evaluates to true while (<= Double/NaN 1) evaluates to false. See CLJ-738 for many details.



 Comments   
Comment by Alex Miller [ 04/Mar/14 3:20 PM ]

Thanks for letting us know about this - I concur that 738 was incorrectly closed without being applied and I have resurrected that ticket. I am closing this one. In the future, feel free to just comment on a ticket directly, or better (for a closed ticket), comment on one of the mailing lists.





[CLJ-1365] New collection hash functions are too slow Created: 20/Feb/14  Updated: 11/Mar/14  Resolved: 11/Mar/14

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: collections

Attachments: Text File clj-1365-v1.patch     Text File clj-1365-v2.patch     Zip Archive testclj1365.zip    
Patch: Code
Approval: Ok

 Description   

As reported ( https://groups.google.com/d/msg/clojure-dev/t6LAmVe-RLM/ekLTKxYfU5UJ ) by Mark Engelberg, the new collection hashing functions are slower than invoking the Murmur3 functions directly. See the attached zip for performance tests.

Approach: Made mix-collection-hash, hash-ordered-coll, and hash-unordered-coll use primitive type hints to avoid the bulk of the time.

Patch: clj-1365-v2.patch

Screened by:



 Comments   
Comment by Alex Miller [ 20/Feb/14 11:26 AM ]

Added to 1.6 release.

Comment by Alex Miller [ 20/Feb/14 12:40 PM ]

Made hash functions inline for performance.

Comment by Rich Hickey [ 20/Feb/14 7:55 PM ]

Reported where?

This looks like bad benchmarking.

(dotimes [_ 10] (let [x 1 y 1] (time (dotimes [n 1000000000] (clojure.lang.Murmur3/mixCollHash x y)))))

and

(dotimes [_ 10] (let [x 1 y 1] (time (dotimes [n 1000000000] #_(clojure.lang.Murmur3/mixCollHash x y)))))

take the same time on my machine.

I'd need to see tests where the return was definitely used, it seems this is just more easily ignored by hotspot when not used.

We probably only need to hint count and the return for decent results.

Comment by Alex Miller [ 20/Feb/14 8:55 PM ]

It was reported by Mark Engelberg in his Instaparse rework - he observed these calls taking noticeably longer and overall times 10-20% down. I will ask him to chime in here.

Comment by Rich Hickey [ 04/Mar/14 8:44 AM ]

Could someone please test hinting hint count and the return? I'd hate for the answer to anyone's perf issues be inlining.

Comment by Alex Miller [ 04/Mar/14 9:06 AM ]

I will provide some more data for consideration of the options.

Comment by Alex Miller [ 04/Mar/14 11:07 AM ]

Test project for different variants

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

Attached a test project with different variants for testing and better benchmarking. To run:

unzip testclj1365.zip
cd clj1365
lein uberjar
java -server -cp target/clj1365-0.1.0-SNAPSHOT-standalone.jar clj1365.core

Results:

mix-collection-hash original
"Elapsed time: 57.777 msecs"
"Elapsed time: 18.034 msecs"
"Elapsed time: 20.591 msecs"
"Elapsed time: 25.179 msecs"
"Elapsed time: 21.781 msecs"
mix-collection-hash hints
"Elapsed time: 14.983 msecs"
"Elapsed time: 8.871 msecs"
"Elapsed time: 8.793 msecs"
"Elapsed time: 8.92 msecs"
"Elapsed time: 8.873 msecs"
mix-collection-hash inline
"Elapsed time: 10.04 msecs"
"Elapsed time: 7.117 msecs"
"Elapsed time: 7.306 msecs"
"Elapsed time: 7.324 msecs"
"Elapsed time: 7.175 msecs"
Murmur3/mixCollHash
"Elapsed time: 9.522 msecs"
"Elapsed time: 7.288 msecs"
"Elapsed time: 7.397 msecs"
"Elapsed time: 7.364 msecs"
"Elapsed time: 7.345 msecs"

From these results, I infer that the unhinted version is slower (21 ms) than a static call (7 ms). Inlining gives you same perf as static. Hinting inputs and return gives almost the same perf (9 ms).





[CLJ-1363] Field access via .- in reflective case does not work Created: 18/Feb/14  Updated: 28/Feb/14  Resolved: 28/Feb/14

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: interop

Attachments: Text File clj-1363-v1.patch     Text File clj-1363-v2.patch     Text File clj-1363-v3.patch    
Patch: Code and Test
Approval: Ok

 Description   

The (.-foo instance) syntax will properly resolve to either a field or a no-arg method if the type of instance is known. However, in the reflective case, it will only resolve to a method. This behavior should match the non-reflective case. The method case always be forced by using (. foo (method)).

user> (definterface I (a []))
user.I
user> (deftype T [a] I (a [_] "method"))
user.T
user> (def t (->T "field"))
#'user/t
user=> (. ^T t a)  ;; as expected (prefer method)
"method"
user=> (. ^T t -a) ;; as expected (prefer field)
"field"
user> (. t a)      ;; as expected (prefer method)
"method"
user> (. t -a)     ;; WRONG - should return "field"
"method"

Approach: This case falls into Reflector.invokeNoArgInstanceMember() (this is the only place this method is used). InstanceFieldExpr now takes another flag (requireField) which will be set to true if "-field" and false if "field". InstanceFieldExpr will invoke (or emit) a call to Reflector.invokeNoArgInstanceMember() which now takes the same flag. If the flag is set to true, it first looks only for a field, otherwise it looks for a method and falls back to field which throws an error if necessary. I added a new invokeNoArgInstanceMember() with an arity to match the old arity - existing bytecode compiled on older Clojure versions will be trying to call this arity.

Patch: clj-1363-v3.patch

Screened by:



 Comments   
Comment by Rich Hickey [ 20/Feb/14 7:24 PM ]

You can't change the semantics of invokeNoArgInstanceMember - they are correct when not using '-'. We need to feed the info that '-' was used through InstanceFieldExpr and make field-first conditional on that.

Comment by Alex Miller [ 21/Feb/14 5:42 AM ]

Updated with new patch to thread this case through InstanceFieldExpr.

Comment by Andy Fingerhut [ 28/Feb/14 6:02 AM ]

A patch for this ticket has been committed as part of Clojure 1.6.0-beta2: https://github.com/clojure/clojure/commit/5fda6cb262d1807566ecadd3af9aaafb58ee5544

It appears this ticket could be closed now.





[CLJ-1359] Fix changelog typos for 1.6 Created: 18/Feb/14  Updated: 25/Feb/14  Resolved: 25/Feb/14

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

Type: Enhancement Priority: Minor
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: None

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

 Description   

Some reported problems in the 1.6 changelog:

1) two different issues are both called CLJ-935
2) two issues that are probably different are both called CLJ-1328
3) "Make range consistently return () with a step of 0." This is slightly incorrect. Range now consistently returns an infinite sequence of start with a 0 step.

Patch: clj-1359.patch - updated for these issues, may want to hold this and update for any post-beta1 changes too.






[CLJ-1356] clojure.org/agents calls out deprecated funcs Created: 17/Feb/14  Updated: 17/Feb/14  Resolved: 17/Feb/14

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

Type: Enhancement Priority: Trivial
Reporter: Ryan Macy Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: agents, documentation, website


 Description   

""If any exceptions are thrown by an action function, no nested dispatches will occur, and the exception will be cached in the Agent itself. When an Agent has errors cached, any subsequent interactions will immediately throw an exception, until the agent's errors are cleared. Agent errors can be examined with agent-errors and cleared with clear-agent-errors.""

While it is true and those functions will do what it describes, they are listed as deprecated in the docs. Should we update this paragraph to reflect usage of `agent-error` and `restart-agent` instead?



 Comments   
Comment by Ryan Macy [ 17/Feb/14 11:38 AM ]

I hope I put this in the right place!

Comment by Alex Miller [ 17/Feb/14 12:32 PM ]

Yep, thanks!

Comment by Alex Miller [ 17/Feb/14 12:40 PM ]

Fixed.





[CLJ-1355] Restore symbol and keyword hashCode to avoid breaking compiled case expressions Created: 17/Feb/14  Updated: 27/Feb/14  Resolved: 27/Feb/14

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

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

clojure 1.6.0-beta1


Attachments: File clj-1355-cached.diff     Text File clj-1355-v2.patch    
Patch: Code
Approval: Ok

 Description   

case expressions compiled in Clojure 1.5 are broken if run with Clojure 1.6 where hashCode behavior has diverged from hasheq. In particular, Symbol and Keyword fall into this category.

Approach: Cache both hashCode (with 1.5 calculation) and hasheq (new 1.6 calculation) in Symbol and just hasheq in Keyword. In 1.5, these were the same and case expressions compiled with 1.5 will store the old hash calculation. In 1.6, the hashCode of an expression will be used for comparison.

I tested this by AOT compiling a project in clojure 1.5.1 with this function:

(defn check [v]
  (case v
    :k "keyword match"
    'k "symbol match"
    "k" "string match"
    "no match"))

I verified that (check :k) and (check 'v) incorrectly returned "no match" on Clojure 1.6.0-beta1. I then verified that they returned "keyword match" and "symbol match" respectively on Clojure 1.6.0-master with this patch applied.

Patch: clj-1355-v2.patch



 Comments   
Comment by Alex Miller [ 17/Feb/14 9:38 AM ]

Add patch that caches a new hash field for both Symbol and Keyword that retains Clojure 1.5 computations.

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

There is a concern here that we are adding a new int field to every Symbol and Keyword (and keyword holds a symbol, so it's really 2 for each keyword).

Comment by Rich Hickey [ 20/Feb/14 7:27 PM ]

I don't think we need to cache in keyword, it's just an add

Comment by Alex Miller [ 20/Feb/14 9:24 PM ]

Updated patch to only cache hashCode in symbol and compute in Keyword.





[CLJ-1354] Make the class APersistentVector.SubVector public Created: 17/Feb/14  Updated: 25/Feb/14  Resolved: 25/Feb/14

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

Type: Enhancement Priority: Major
Reporter: Michał Marczyk Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-CLJ-1354-make-APersistentVector.SubVector-public.patch    
Patch: Code
Approval: Ok

 Description   

The patch marks APersistentVector.SubVector public so that it can be used as a type hint for reflection-free access to subvec internals. I missed this in CLJ-1150.

core.rrb-vector needs access to the internals of the built-in vector types in order to support their efficient concatenation and (true, RRB-style) slicing.

Patch: 0001-CLJ-1354-make-APersistentVector.SubVector-public.patch

Screened by: Alex Miller



 Comments   
Comment by Michał Marczyk [ 17/Feb/14 7:30 AM ]

This is the exact spot where I'm trying to get at SubVector internals in core.rrb-vector:

https://github.com/clojure/core.rrb-vector/blob/core.rrb-vector-0.0.10/src/main/clojure/clojure/core/rrb_vector/rrbt.clj#L976

With 1.6.0-alpha3, {{(fv/catvec (subvec [0 1 2 3] 1 2) [:foo])}} results in IllegalAccessError tried to access class clojure.lang.APersistentVector$SubVector from class clojure.core.rrb_vector.rrbt$eval2476$fn__2477 clojure.core.rrb-vector.rrbt/eval2476/fn--2477 (rrbt.clj:978). With this patch applied, it works as expected, returning [1 :foo].





[CLJ-1353] Prevent test app from appearing in Mac OS X dock Created: 16/Feb/14  Updated: 27/Feb/14  Resolved: 27/Feb/14

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

Type: Enhancement Priority: Trivial
Reporter: Steve Miner Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None
Environment:

Mac OS X


Attachments: Text File CLJ-1353-no-mac-dock.patch     Text File CLJ-1353-v2.patch     Text File clj-1353-v3.patch     Text File clj-1353-v4.patch    
Patch: Code
Approval: Ok

 Description   

During a local ant build of Clojure (tested with master after release of 1.6.0-beta1), the script/run_test.clj is executed. As a side-effect on the Mac, the Java coffee cup app icon is placed in the Dock, and the test app becomes the active application on the desktop. This is slightly annoying.

Even with this property set, activation of awt causes focus to switch temporarily then switch back (at least on Mac).

Solution: Set the following properties during the build:

java.awt.headless=true

Patch: clj-1353-v4.patch



 Comments   
Comment by Steve Miner [ 16/Feb/14 1:39 PM ]

CLJ-1353-no-mac-dock.patch adds a line to script/run_tests.clj to set the apple.awt.UIElement property. This prevents the test app from appearing in the Dock on Mac OS X.

Comment by Steve Miner [ 16/Feb/14 2:18 PM ]

CLJ-1349 might rearrange the affected source, which would force an update to this patch. Still just a one-liner so maybe it could be added to the patch for CLJ-1349.

Comment by Alex Miller [ 16/Feb/14 5:20 PM ]

I also find this highly annoying.

Comment by Andy Fingerhut [ 17/Feb/14 11:21 AM ]

Patch CLJ-1353-v2.patch is identical to Steve Miner's CLJ-1353-no-mac-dock.patch, except it adds another line to build.xml to set the property there, too. At least on my Mac systems, an icon appears in the dock during compilation, not only during testing, and this added line prevents that. Keeps Steve as the patch author.

Comment by Andy Fingerhut [ 17/Feb/14 11:22 AM ]

I tested CLJ-1353-v2.patch on a Linux system, too, and at least the messages that appear on the console during the execution of "ant" are identical with and without this patch, so no extra warnings appear due to these extra properties being set that are likely ignored by the JVM there.

Comment by Steve Miner [ 17/Feb/14 1:45 PM ]

Adding the sysproperty setting to build.xml sounds like a good idea. Thanks.

Comment by Alex Miller [ 18/Feb/14 1:42 PM ]

I found that even with this property, I still see focus change, then come back, during the build due to the activation of awt. Adding the java.awt.headless=true property made that stop. I updated the patch in both locations and now on Mac focus is never stolen during the build.

FYI: If you see the Java "Allow incoming network connections?" dialog on Mac during the tests in response to creating the Sockets in test/clojure/test_clojure/java/io.clj (test-socket-iofactory), this procedure makes that stop:

http://techblog.willshouse.com/2012/10/17/how-to-allow-java-in-the-firewall-on-os-x-mountain-lion/

Beware tracking down the correct version of Java (for example the 1.6 version) instead of the easier to find 1.7 version - the permissions are separate for each version.

Comment by Andy Fingerhut [ 24/Feb/14 2:35 PM ]

In my testing, the addition of the java.awt.headless=true properties in both build.xml and src/script/run_tests.clj was sufficient to avoid the additional icon appearing, and also avoiding any change of focus. Setting apple.awt.UIElement=true appears to be unnecessary (but harmless).

Comment by Steve Miner [ 24/Feb/14 3:28 PM ]

Yes, it seems that java.awt.headless=true is a better, more general solution for the build process. I think apple.awt.UIElement would be appropriate if you actually needed AWT for user interaction but didn't want the dock icon.

Comment by Alex Miller [ 25/Feb/14 11:33 AM ]

Added v4 patch that only sets java.awt.headless=true and drops the apple property.





[CLJ-1352] clojure.test/test-vars runs :each fixtures for vars without :test metadata Created: 14/Feb/14  Updated: 25/Feb/14  Resolved: 25/Feb/14

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

Type: Defect Priority: Major
Reporter: Toby Crawley Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File tcrawley-fixtures-with-non-test-vars-2014-02-14.diff    
Patch: Code and Test
Approval: Ok

 Description   

The patch for CLJ-866 introduced a bug with :each fixtures and non-test vars: the fixtures are invoked for every var, not just ones with :test metadata.

Patch: tcrawley-fixtures-with-non-test-vars-2014-02-14.diff

Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 24/Feb/14 2:37 PM ]

The patch for this ticket has been committed: https://github.com/clojure/clojure/commit/919a7100ddf327d73bc2d50d9ee1411d4a0e8921

but the ticket has not yet been closed.

Comment by Alex Miller [ 24/Feb/14 3:09 PM ]

yeah, I noticed that too. I was going to mention it to Stu the next time we talked.





[CLJ-1350] (/ 1 3) returns Ratio 31/3 Created: 14/Feb/14  Updated: 14/Feb/14  Resolved: 14/Feb/14

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

Type: Defect Priority: Minor
Reporter: Justin Hanekom Assignee: Unassigned
Resolution: Not Reproducible Votes: 0
Labels: None
Environment:

openSuSE 13.1



 Description   

(/ 1 3) incorrectly returns the Ratio 31/3. Other numbers, such as (/ 1 4), work as expected. This could be worked around by using Java interop, but I don't think / it is functioning correctly in this case.



 Comments   
Comment by Justin Hanekom [ 14/Feb/14 1:25 AM ]

$ lein version
Leiningen 2.3.4 on Java 1.7.0_51 OpenJDK 64-Bit Server VM

Comment by Nicola Mometto [ 14/Feb/14 5:35 AM ]

I cannot reproduce this on clojure 1.5.1 or 1.6.0-master-SNAPSHOT

Comment by Alex Miller [ 14/Feb/14 8:14 AM ]

I also could not reproduce on 1.5 or 1.6. Please provide more information on your Clojure environment ({\*clojure-version\*} and also verify that you're not seeing printing obscuring your repl output or something.

user=> *clojure-version*
{:major 1, :minor 5, :incremental 1, :qualifier nil}
user=> (def x (/ 1 3))
#'user/x
user=> (numerator x)
1
user=> (denominator x)
3
user=> x
1/3
Comment by Justin Hanekom [ 14/Feb/14 12:01 PM ]

Today I'm unable to reproduce this behavior, although yesterday I could!? I'm so embarrassed :-*>

Thanks for closing.





[CLJ-1348] Add functions for external collection hashing Created: 10/Feb/14  Updated: 14/Feb/14  Resolved: 14/Feb/14

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-1348-1.patch     Text File clj-1348-2.patch     Text File clj-1348-3.patch    
Patch: Code
Approval: Ok

 Description   

External collections wishing to implement hasheq appropriately must follow the advice at http://clojure.org/data_structures#hash. To simplify the implementation (and avoid unwanted dependencies on the internal Murmur3 class), add two new functions hash-ordered-coll and hash-unordered-coll that provide a proper collection hasheq over entire collections.

Patch: clj-1348-3.patch (fixes [k v])



 Comments   
Comment by Alex Miller [ 10/Feb/14 9:27 AM ]

Added patch. Will need to be refreshed once other patches go in.

Comment by Alex Miller [ 10/Feb/14 4:02 PM ]

oops

Comment by Rich Hickey [ 12/Feb/14 10:53 AM ]

[k,v] => [k v]

Comment by Alex Miller [ 12/Feb/14 11:46 AM ]

New patch fixing [k v].





[CLJ-1345] Add 1.6 beta changelog updates Created: 07/Feb/14  Updated: 14/Feb/14  Resolved: 14/Feb/14

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-1345-2.patch     Text File clj-1345.patch    
Patch: Code
Approval: Ok

 Description   

Update changelog for 1.6 beta.

Patch: clj-1345-2.patch



 Comments   
Comment by Alex Miller [ 10/Feb/14 4:02 PM ]

oops

Comment by Alex Miller [ 12/Feb/14 12:47 PM ]

Updated patch to fix if-some and when-some definitions.





[CLJ-1344] defrecord still uses old hashing algorithm Created: 07/Feb/14  Updated: 14/Feb/14  Resolved: 14/Feb/14

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

Type: Defect Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: defrecord

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

 Description   

defrecord implements hasheq by calling clojure.lang.APersistentMap/mapHasheq. mapHasheq uses the old map hash calculation instead of the new one. At least one external collection (data.avl) also calls this function. It should be updated to match the new hasheq calculations.

I considered changing defrecord to call Murmur3 directly, but this would create a case where the generated class does not work with older Clojure runtimes so I left it at calling mapHasheq instead.

Patch: clj-1344-1.patch



 Comments   
Comment by Alex Miller [ 07/Feb/14 1:33 PM ]

Attached patch to make mapHasheq use new hash map calculation.

Comment by Alex Miller [ 10/Feb/14 4:01 PM ]

oops





[CLJ-1343] Add some?, when-some, if-some for (not (nil? x)) conditions Created: 07/Feb/14  Updated: 15/Feb/14  Resolved: 14/Feb/14

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-1343-1.patch     Text File clj-1343-2.patch     Text File clj-1343-3.patch     Text File clj-1343-4.patch    
Patch: Code and Test
Approval: Ok

 Description   

Sometimes it is useful to have a form for non-nil conditions (as opposed to the existing logical true conditions).
Three additions to support this case:

  • some? - same as (not (nil? x))
  • if-some - like if-let, but checks (some? test) instead of test
  • when-some - like when-let, but checks (some? test) instead of test

Patch: clj-1343-4.patch



 Comments   
Comment by Alex Miller [ 10/Feb/14 4:02 PM ]

oops

Comment by Tassilo Horn [ 11/Feb/14 2:32 AM ]

At least to me, the name `some?` doesn't convey the same information as "not nil", so I'd rather prefer a more explicit name like `non-nil?`.

Also, I'm not convinced of the benefit of something like `(when-some x ...)` compared to `(when-not (nil? x) ...)`. A little shorter and one pair of parens less, but IMHO not as clear.

Comment by Jozef Wagner [ 11/Feb/14 2:59 AM ]

In my opinion, some? should be defined as (not (empty? coll)), and used as in "are there 'some' items in this collection?". This will also play nicely with some, which also takes collection as an argument.

Comment by Tassilo Horn [ 12/Feb/14 1:02 AM ]

Jozef, for that purpose, you'd use `seq`. Actually, the definition of `empty?` is `(not (seq coll))`, so your suggestion would boil down to `some?` being `(not (not (seq coll)))`.

Comment by Rich Hickey [ 12/Feb/14 10:56 AM ]

if-some and when-some are supposed to be like if-let and when-let respectively. Changelog will need updating as well

Comment by Alex Miller [ 12/Feb/14 12:38 PM ]

Updated patch to make if-some and when-some similar to if-let and when-let.

Comment by Alex Miller [ 14/Feb/14 10:01 AM ]

New patch that does not use "some?" in if-some and when-some.

Comment by Alex Miller [ 14/Feb/14 10:39 AM ]

New patch that adjusts when-some impl.

Comment by Kyle Kingsbury [ 15/Feb/14 1:04 PM ]

I'd like to echo Jozef Wagner's and Steve Losh's confusion here.

```
user=> (some odd? [1 2 3])
true
user=> (some? odd? [1 2 3])

ArityException Wrong number of args (2) passed to: user$some-QMARK- clojure.lang.AFn.throwArity (AFn.java:437)
```

I might expect (some?) to behave like (some), except returning a boolean instead of a logically true value, but this is clearly not the case. In no other case in the stdlib can I think of two functions which differ only by punctuation yet have completely different semantics.

```
user=> (some? [])
true
```

Given (some)'s association with sequences, I might interpret (some?) to mean "are there some elements here?"; but that's definitely wrong. Given we have (not=), (not-any?), (not-empty), and (not-every?), can we please name this function (not-nil?)? It's only three characters, but makes the interpretation unambiguously clear.

```
user=> (def x nil)
#'user/x
user=> (def y nil)
#'user/y
user=> (some? [x y])
true
user=> (when-some [x y] :i-expect-true)
nil
```

The fact that (when-some) and (if-some) behave like let bindings is, erm, quite surprising to me. The other binding conditionals have -let in their name; perhaps it would be appropriate to use -let here as well?

For that matter, is this use case all that common? I think I reach specifically for a nil? test fewer than 1 in 20 conditionals--in those cases, why not just write

```
(when-let [x (not-nil? y)]
...)
```

instead of

```
(when-some [x y]
...)
```

I'm just not convinced that this pattern is common enough to warrant the confusion of (when-some) having nothing to do with (when (some ...)), haha. What do y'all think? Have I missed some symmetry between (some?) and (some) that helps this all make sense?

Comment by Alex Miller [ 15/Feb/14 4:36 PM ]

Summarizing comments here, mailing list, Twitter, etc:

  • some uses a truthy comparison. some->, some->> use a not nil comparison. This difference existed in 1.5 some?/if-some/when-some follow the latter. This split is unfortunate, but existed before this addition.
  • not-nil?, non-nil?, nnil?, exists?, and all other alternatives I've seen mentioned were considered as options before the existing names were chosen by Rich. Many people have expressed negative feedback about the name choices and I will channel that to Rich for consideration, but ultimately the choice is his.
  • if-some and when-some are likely more useful than some?. In particular, it is commonly needed when reading from core.async channels where nil is a special value (but false is not).
(go
  (if-some [v (<! c)]
    ...))




[CLJ-1339] Empty primitive vectors throw NPE on .equals with non-vector sequential types Created: 04/Feb/14  Updated: 14/Feb/14  Resolved: 14/Feb/14

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

Type: Defect Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJ-1339.patch    
Patch: Code and Test
Approval: Ok

 Description   

Primitive vectors have several equality cases. In the case where the compared value is not a vector or random access collection but is a sequential or list, an empty primitive vector will throw an NPE:

user> (.equals (vector-of :long) [])   ;; ok
true
user> (.equals (vector-of :long) '())  ;; broken
NullPointerException   clojure.core.Vec (gvec.clj:135)

Cause: In this case of the primitive vector equals() method, seq is called on itself, then .equals() is invoked on the result. seq will return null for an empty primitive vector, causing an NPE.

Solution: Check for this condition and compare with (nil? (seq o)) on the other object.

Patch: CLJ-1339.patch






[CLJ-1338] New Murmur3 class is not public Created: 04/Feb/14  Updated: 07/Feb/14  Resolved: 07/Feb/14

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

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

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

 Description   

The new Murmur3 class added for hashing is not public, which is problematic for code that needs to call it in several other tickets. To separate out this overlapping change, I have provided it here by itself.






[CLJ-1336] Allow external collections to use standard collection hashing Created: 31/Jan/14  Updated: 14/Feb/14  Resolved: 14/Feb/14

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-1336-1.patch     Text File clj-1336-2.patch     Text File clj-1336-3.patch     Text File clj-1336-4.patch    
Patch: Code and Test
Approval: Ok

 Description   

With the change in new hashing algorithms in 1.6, we need to provide a public hook for collections implemented outside of core to participate in the same hash mixing behavior as core collections.

Patch: clj-1336-4.patch
Depends on: CLJ-1338, CLJ-1339, CLJ-1335, CLJ-1331



 Comments   
Comment by Alex Miller [ 04/Feb/14 10:42 AM ]

Updated patch for a couple issues. However, in testing the use of this I discovered that the hash-basis must be an int and the basis accumulation must be based on int-accumulation with int-overflow, so it is not possible to do this in pure Clojure so this function is not currently useful.

I think the best solution would be to provide functions that encapsulate the ordered and unordered algorithms (Murmur3/hashOrdered and Murmur3/hashUnordered basically) such that external collections can implement hasheq correctly and with good performance.

Comment by Alex Miller [ 04/Feb/14 2:45 PM ]

Add new patch that makes Murmur3 class public so it will work for users of mix-collection-hash. Also adds generative tests for comparing the external collection hashing algorithm with hashes produced by internal ordered and unordered collections. These tests currently fail due to CLJ-1335 (empty list and empty lazy seq return wrong hash code).





[CLJ-1335] PersistentList$EmptyList and empty LazySeq still returns old value for hasheq Created: 30/Jan/14  Updated: 14/Feb/14  Resolved: 14/Feb/14

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

Type: Defect Priority: Blocker
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File clj-1335-v1.diff     Text File clj-1335-v2.patch    
Patch: Code and Test
Approval: Ok

 Description   

After late Jan 2014 changes to hash functions, PersistentList$EmptyList and (lazy-seq) were left behind:

user=> (= '() (lazy-seq) [])
true
user=> (map hash ['() (lazy-seq) []])
(1 1 -2017569654)
user=> (map class ['() (lazy-seq) []])
(clojure.lang.PersistentList$EmptyList clojure.lang.LazySeq clojure.lang.PersistentVector)

PersistentQueue/EMPTY was updated, so should not need any change.

Solution: Update LazySeq.hasheq() and make EmptyList implement IHashEq. EmptyList now creates a static constant for the hash value of an empty ordered collection and returns the constant for hasheq. An alternative would be to have Murmur3 have this constant instead.

Patch: clj-1335-v2.patch
Depends on: CLJ-1338, CLJ-1339, CLJ-1331 (must be applied first)

Patch:



 Comments   
Comment by Andy Fingerhut [ 30/Jan/14 6:33 PM ]

Patch clj-1335-v1.diff adds tests that assume the patch clj-1331-v1.diff on ticket CLJ-1331 have already been committed. If it is desired to combine these into one patch, or commit this one without that one, I can eliminate that dependency.

Makes PersistentList$EmptyList implement IHashEq interface with a straightforward implementation of hasheq(), comments out empty LazySeq special case check that caused it to return old hash value, and fixes a NullPointerException for primitive vectors discovered by the new tests added.





[CLJ-1334] Improve performance of the bean function with caching Created: 30/Jan/14  Updated: 31/Jan/14  Resolved: 31/Jan/14

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

Type: Enhancement Priority: Minor
Reporter: Ron Pressler Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: performance


 Description   

The bean function is a very useful Java interop feature that provides a read-only view of a Java Bean as a Clojure map.
As it stands, the function performs introspection on the bean's class whenever the function is called. We can, however, cache the mapping from keywords to getters using JDK 7's handy ClassValue. The proposed function will look like this:

(def ^:private ^java.lang.ClassValue bean-class-value
(proxy [java.lang.ClassValue]
[]
(computeValue [c]
(reduce1 (fn [m ^java.beans.PropertyDescriptor pd]
(let [name (. pd (getName))
method (. pd (getReadMethod))
type (.getPropertyType pd)]
(if (and method (zero? (alength (. method (getParameterTypes)))))
(assoc m (keyword name) (fn [x] (clojure.lang.Reflector/prepRet type (. method (invoke x nil)))))
m)))
{}
(seq (.. java.beans.Introspector
(getBeanInfo c)
(getPropertyDescriptors)))))))

(defn bean
"Takes a Java object and returns a read-only implementation of the
map abstraction based upon its JavaBean properties."
{:added "1.0"}
[^Object x]
(let [c (. x (getClass))
pmap (.get bean-class-value c)
v (fn [k] ((pmap k) x))
snapshot (fn []
(reduce1 (fn [m e]
(assoc m (key e) ((val e) x)))
{} (seq pmap)))]
(proxy [clojure.lang.APersistentMap]
[]
(containsKey [k] (contains? pmap k))
(entryAt [k] (when (contains? pmap k) (new clojure.lang.MapEntry k (v k))))
(valAt ([k] (when (contains? pmap k) (v k)))
([k default] (if (contains? pmap k) (v k) default)))
(cons [m] (conj (snapshot) m))
(count [] (count pmap))
(assoc [k v] (assoc (snapshot) k v))
(without [k] (dissoc (snapshot) k))
(seq [] ((fn thisfn [plseq]
(lazy-seq
(when-let [pseq (seq plseq)]
(cons (new clojure.lang.MapEntry (first pseq) (v (first pseq)))
(thisfn (rest pseq)))))) (keys pmap))))))



 Comments   
Comment by Jozef Wagner [ 30/Jan/14 9:25 AM ]

associated discussion

Comment by Alex Miller [ 31/Jan/14 8:45 AM ]

Curious if anyone is using this inside production code? I use it at the REPL when exploring Java stuff sometimes but have never used it inside my actual code.

Comment by Stuart Halloway [ 31/Jan/14 12:28 PM ]

This is a cool idea, but doesn't need to be in core. How about https://github.com/clojure/java.data ?

Comment by Ron Pressler [ 31/Jan/14 12:48 PM ]

This might be good for java.data, and I'm certainly not saying it should be in the core except for the fact that it already is. This is a proposal for a performance improvement of a core function.





[CLJ-1331] Primitive vectors should use new hash Created: 29/Jan/14  Updated: 14/Feb/14  Resolved: 14/Feb/14

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

Type: Enhancement Priority: Major
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File clj-1331-v1.diff     Text File clj-1331-v2.patch    
Patch: Code and Test
Approval: Ok

 Description   

Primitive vectors created via vector-of still use Java hashCode for hasheq.

Solution: Make primitive vectors implement IHashEq and call Murmur3.hashOrdered().

Patch: clj-1331-v2.patch
Depends on: CLJ-1338 (must be applied first)



 Comments   
Comment by Andy Fingerhut [ 29/Jan/14 6:03 PM ]

Patch clj-1331-v1.diff is one way to change primitive vectors to use Murmur3 hash.





[CLJ-1328] Make some Clojure tests independent of hash function used Created: 20/Jan/14  Updated: 07/Feb/14  Resolved: 07/Feb/14

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

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

Attachments: File clj-1328-v3.diff     File clj-1328-v4.diff    
Patch: Code and Test
Approval: Ok

 Description   

The most interesting failures with the new hash function are probably the 3 deftest's in multimethods.clj that define the same multimethod name 'simple', and thus whether they pass or fail depends upon the order that they are executed. They are currently executed in an order that allows them to pass. Found this while testing murmurHash3 changes to Clojure, which caused the deftest's to execute in a different order and fail.

Simplest way to eliminate this dependency on order is to make the multimethod names unique in each test, so none of them depends upon state left behind by the others.



 Comments   
Comment by Andy Fingerhut [ 20/Jan/14 1:18 PM ]

Patch clj-1328-v1.diff makes all defmulti names unique in multimethods.clj, so that no deftest result depends upon state left behind by another.

Comment by Andy Fingerhut [ 29/Jan/14 8:11 PM ]

Updates some, but not all, tests that were recently modified or disabled due to change in hash function.

Comment by Andy Fingerhut [ 29/Jan/14 10:52 PM ]

Updates one more test than the previous patch.

Comment by Andy Fingerhut [ 31/Jan/14 3:43 PM ]

clj-1328-v4.diff is identical to clj-1328-v3.diff, except it adds a comment explaining why the case hash collision tests don't need to change much, and it puts in a couple of missing (is ...) around some equality tests.





[CLJ-1320] min-key assumes numbers, not comparables. Created: 09/Jan/14  Updated: 09/Jan/14  Resolved: 09/Jan/14

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

Type: Enhancement Priority: Major
Reporter: Pierre-Yves Ritschard Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None

Attachments: File min-key.diff    
Patch: Code

 Description   

The min-key function assumes the key-fn will yield a number and thus uses the '<' operator to compare results.
There are cases where one might want to use min-key with comparables instead.

While (first (sort-by key-fn seq)) could also be used, it feels more natural for min-key to use comparables.



 Comments   
Comment by Pierre-Yves Ritschard [ 09/Jan/14 3:18 PM ]

As discussed on the .L, since compare is slower it makes more sense to keep min-key as-is.





[CLJ-1318] Support destructuring maps with namespaced keywords Created: 06/Jan/14  Updated: 23/Feb/14  Resolved: 31/Jan/14

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File clj-1318-1.diff     File clj-1318-2.diff     File clj-1318-3.diff     File clj-1318-4.diff     File clj-1318-5.diff     File clj-1318-6.diff    
Patch: Code and Test
Approval: Ok

 Description   

Current :keys destructuring expects symbols and creates local bindings based on those symbols. This works fine with maps that use non-namespaced keyword keys. This enhancement is to add support for destructuring maps with namespaced keyword keys.

;; typical key destructuring for keyword keys without namespaces
(let [{:keys [a b]} {:a 1 :b 2}] (+ a b))

;; WANT some way to destructure map with namespaced keys
(let [{:keys [????]} {:x/a 1 :y/b 2}] (+ a b))

Approach: Allow keywords (with or without namespaces) in :keys destructuring. Destructure to bindings with the name of the keyword (namespace is ignored).

;; this now works
(let [{:keys [x/a y/b]} {:x/a 1 :y/b 2}] (+ a b))

;; add support for putting keywords into :keys as well to support ::keywords
(let [{:keys [:x/a :y/b]} {:x/a 1 :y/b 2}] (+ a b))
(let [{:keys [::a]} {:user/a 1}] a)

;; syms will also now support namespaced symbols
(let [{:syms [x/a y/b]} {'x/a 1 'y/b 2}] (+ a b))

Patch: clj-1318-6.diff

Screened by: Stuart Sierra. See comments, below.

Doc TODO: Will need to update http://clojure.org/special_forms#binding-forms with new binding form.



 Comments   
Comment by Nicola Mometto [ 06/Jan/14 11:58 AM ]

Why {:keys [:a/b]} and not {:keys [a/b}}?
Also, this should probably be extended to :syms for consistency

Comment by Alex Miller [ 06/Jan/14 12:28 PM ]

Good questions both. For the first question, we want to make locally namespaced keywords (::foo) work and there is no way to say that as a symbol.

I am waiting to hear back from Rich whether support for namespaced :syms is desirable. I think the change to support it is identical to the change to support namespaced keywords as symbols. I'm going to proactively update the patch to support that too.

Comment by Alex Miller [ 06/Jan/14 12:50 PM ]

Added new patch - now supports namespaced symbols or keywords in :keys and namespaced symbols in :syms.

Comment by Rich Hickey [ 06/Jan/14 1:00 PM ]

Should (also) support symbols for names, e.g. {:keys [a/b]}, only limitation is you can't get ns alias resolution. :syms support makes sense, but may seem weird to provide keywords for local names (where it doesn't as much for keywords), but would allow reaching aliases. My preference is no keyword names support for :syms, i.e. {:syms [a/b]} ok, {:syms [:a/b]} not.

Comment by Nicola Mometto [ 06/Jan/14 1:10 PM ]

To me {:syms [:a/b]} doesn't feel any more weird than writing {:keys [:a/b]}.
If this is going to be added, I think it should be consistent for :keys and :syms.
I understand that :syms is rarely used and this should not be an issue realistically, but I would expect everything that works for :keys to work for :syms too and adding only half a feature to :syms might cause unnecessary confusion.

Comment by Nicola Mometto [ 07/Jan/14 2:16 PM ]

With this patch this will now work:

user=> (let [:a/b 1] b)
1

I don't think this is desiderable.

Comment by Alex Miller [ 07/Jan/14 3:52 PM ]

Right, that is a consequence of allowing keywords in the :keys. At a glance this seems hard to address without significant changes unless we catch it prior to processing. Will consider.

Comment by Alex Miller [ 07/Jan/14 4:40 PM ]

Added new patch variant that catches keywords as let binding keys and throws an Exception.

Comment by Alex Miller [ 10/Jan/14 2:24 PM ]

Added one test in -4 showing example of auto-resolved keywords in :keys.

Comment by Stuart Sierra [ 10/Jan/14 3:00 PM ]

Screened. A few comments:

1. The examples in the tests use {:keys (a b)} with lists instead of {:keys [a b]} with vectors. Both forms are accepted both before and after the patch, but the docs at Clojure - special_forms only show vectors.

2. I would like this to work, but it would add some complexity:

(ns com.example.myproject.foo)

  (def data
    {::a 1 ::b 2})

  ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
  (ns com.example.myproject.bar
    (:require [com.example.myproject.foo :as foo]))

  ;; I would like this to work:
  (let [{:keys [foo/a foo/b]} foo/data]
    [a b])
  ;;=> [nil nil]

  ;; This is good enough, however:
  (let [{:keys [::foo/a ::foo/b]} foo/data]
    [a b])
  ;;=> [1 2]

3. This doesn't produce an error, which is logically consistent but perhaps not desirable:

(let [{:a ::foo/a} foo/data]
    [a])
Comment by Rich Hickey [ 24/Jan/14 10:11 AM ]

please change the tests to use vectors

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

Added new -5 diff that uses vectors instead of lists in :keys tests.

Comment by Alex Miller [ 24/Jan/14 11:07 AM ]

And also fixing :syms [] in -6 diff.

Comment by Alex Miller [ 24/Jan/14 11:08 AM ]

Changed examples in description to use [].

Comment by Fogus [ 07/Feb/14 2:23 PM ]

A potential point of confusion here is illustrated by the following:

(let [m {:x/a 1, :y/b 2, :x/b 3000}
        {:keys [x/a y/b x/b]} m]
  (+ a b))

//=> 3

To get the answer 3001 one needs to remove the conflicting binding :y/b. Maybe this is not a big deal, but expect questions for the next 100 years.

Comment by David Nolen [ 23/Feb/14 5:01 PM ]

Ported to ClojureScript with CLJS-745





[CLJ-1312] clojure.string/split on empty string includes empty string in results Created: 21/Dec/13  Updated: 21/Dec/13  Resolved: 21/Dec/13

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

Type: Defect Priority: Minor
Reporter: Russell Dunphy Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: string


 Description   

Splitting a string using clojure.string/split with an empty regex includes the empty string in the results - is this expected behaviour?

Example:

Unable to find source-code formatter for language: clojure. Available languages are: javascript, sql, xhtml, actionscript, none, html, xml, java
user=> (clojure.string/split "abc" #"")
["" "a" "b" "c"]


 Comments   
Comment by Alex Miller [ 21/Dec/13 8:05 AM ]

Yes, I think so. This is a case where Clojure defers to the host (Java) for behavior. I think the way to interpret this is that the empty pattern matches all strings. Split checks left to right whether there is a next chunk of string that matches the pattern. The empty pattern matches at the beginning to a string of length 0. Something like that.





[CLJ-1310] some-> behaves differently than -> when used with macros Created: 19/Dec/13  Updated: 19/Dec/13  Resolved: 19/Dec/13

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

Type: Defect Priority: Minor
Reporter: Chris Perkins Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

I stumbled across a behavior of some-> when used with macros, and I'm wondering whether it's expected.

I started with something like this:

(some-> value
foo
bar
quux)

Then I realized that quux might throw an exception, which I want to ignore, so had the possibly misguided idea to do this:

(defmacro catch->nil [& body]
`(try ~@body (catch Exception _# nil)))

(some-> value
foo
bar
quux
catch->nil)

My mental model of some-> is that it should end up wrapping the whole expression in a try-catch, as > does, but that does not happen. some> expands into a `let` instead of a deeply-nested form, so the exception is not caught.

Certainly easy to work around, now that I know about it, but I thought perhaps this was not intended.



 Comments   
Comment by Chris Perkins [ 19/Dec/13 7:34 PM ]

After a little reflection, I realize that my mental model of some-> as "thread-first plus magic fairy-dust" is fatally flawed. I withdraw my objections. Please disregard.

Comment by Alex Miller [ 19/Dec/13 7:50 PM ]

withdrawn by submitter





[CLJ-1307] Type hint remains unqualified, resulting in errors Created: 15/Dec/13  Updated: 15/Dec/13  Resolved: 15/Dec/13

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None


 Description   

This snippet (even though unsensical) demonstrates the issue:

user=> (ns foo (:import clojure.lang.RT))
nil
foo=> (defn x ^RT [])
#'foo/x
foo=> (ns bar (:use foo))
nil
bar=> (.hashCode (x))
CompilerException java.lang.IllegalArgumentException: Unable to resolve classname: RT, compiling:(NO_SOURCE_PATH:4:1)

This is because (:tag (meta #'x)) returns the Symbol 'RT, it should either be the Symbol 'clojure.lang.RT or the Class clojure.lang.RT



 Comments   
Comment by Nicola Mometto [ 15/Dec/13 8:43 PM ]

Duplicate: http://dev.clojure.org/jira/browse/CLJ-1232





[CLJ-1306] Cannot reduce over short[] arrays Created: 14/Dec/13  Updated: 14/Dec/13  Resolved: 14/Dec/13

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

Type: Defect Priority: Major
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: bug

Attachments: File clj-1306.diff    

 Description   

Reducing over a short array currently causes an error:

(reduce + (seq (short-array 10)))
=> ClassCastException [S cannot be cast to [Ljava.lang.Object; clojure.core.protocols/fn--6037 (protocols.clj:126)

This appears to occur because ArraySeq is assumed by protocols.clj to contain an Object[] array in the ".array" field, when in fact it is a short[] array.

Proposed solution to to create ArraySeq_short (analogous to the other primitive types ArraySeq_long etc.) to handle short arrays.



 Comments   
Comment by Mike Anderson [ 14/Dec/13 7:37 AM ]

Fix for CLJ-1306

Comment by Alex Miller [ 14/Dec/13 8:17 AM ]

This was also discovered in CLJ-1200 and the patch for that issue includes ArraySeq_short as you propose. It is expected that CLJ-1200 will be included in 1.6.

Comment by Mike Anderson [ 14/Dec/13 9:29 AM ]

OK, thanks Alex!

Just to be sure: Can you confirm that a fix will definitely go into 1.6? This is a defect, and as such should have a higher priority than CLJ-1200 (which appears to be presented as an performance enhancement patch).

My patch also includes a regression test which I think could helpfully be included in the test suite.

Comment by Alex Miller [ 14/Dec/13 11:24 AM ]

Yes, I fully expect CLJ-1200 to be included and talked to Rich about it as recently as yesterday. I could split things out of that patch and pull both of these in separately. That would be objectively better but definitely more work to do all the ticket, patch, and screening work so I'd rather not. If you want to attach just the regression test to 1200, I think we could include it that way as 1200 hasn't been screened yet. Stuart Sierra is planning to screen it in the next few days.





[CLJ-1304] Fixed minor typos in documentation and code comments Created: 09/Dec/13  Updated: 04/Feb/14  Resolved: 04/Feb/14

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

Type: Defect Priority: Trivial
Reporter: Vipul A M Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docstring

Attachments: File clj-1304-v2.diff     File doc-comment-typos.diff    
Patch: Code
Approval: Screened

 Description   

Fixed minor typos in documentation and code comments across multiple files.



 Comments   
Comment by Andy Fingerhut [ 11/Jan/14 2:53 PM ]

Patch clj-1304-v2.diff dated Jan 11, 2014 is identical to Vipul's patch doc-comment-typos.diff dated Dec 9, 2013, except it applies cleanly to latest master. The only changes are that it removes the part of the patch for files in the ASM library, which was updated in a recent commit to Clojure master.

Comment by Alex Miller [ 04/Feb/14 9:21 PM ]

reopen so that I can set the fix version which didn't get set.

Comment by Alex Miller [ 04/Feb/14 9:22 PM ]

re-close now that fix version is set





[CLJ-1303] Remove (apparently) vestigial forward-defs of unquote and unquote-splicing Created: 05/Dec/13  Updated: 01/Feb/14  Resolved: 31/Jan/14

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

Type: Enhancement Priority: Trivial
Reporter: David Rupp Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: enhancement

Attachments: File generalize-unquote.diff    
Patch: Code

 Description   

clojure/core.clj contains forward defs of unquote and unquote-splicing that seem no longer to be necessary. The pull request at https://github.com/clojure/clojure/pull/45 removes this dead code (also attaching a git diff file). Existing tests pass; no new test code necessary.



 Comments   
Comment by Alex Miller [ 05/Dec/13 9:23 PM ]

FYI for future reference, Clojure doesn't accept pull requests. Thanks for the report though!

Comment by David Rupp [ 05/Dec/13 9:31 PM ]

I noticed. That's why I created the JIRA.

Comment by Andy Fingerhut [ 07/Dec/13 10:25 AM ]

David, I do not have any comment on whether this patch will be accepted or not based on the changes it makes, but patches do need to be in a particular format, including the author's name. See instructions for how to create a patch in this format here: http://dev.clojure.org/display/community/Developing+Patches

Comment by David Rupp [ 07/Dec/13 11:26 AM ]

Submitting properly-formatted patch.

Comment by David Rupp [ 09/Dec/13 7:49 AM ]

Replaced references to clojure.core/unquote and .../unquote-splicing,
which are unbound.

The UNQUOTE and UNQUOTE-SPLICING Symbols in LispReader don't really
refer to anything in clojure.core any longer. They're created and
elided by the reader when it encounters their respective (reader)
macro chars.

Comment by Stuart Halloway [ 31/Jan/14 3:26 PM ]

The patch attached here is poorly-suited for screening. It does more than what it says, (e.g. deleting the def of META) without explaining why.

It also removes things that are commented out. Pretty clear that the BDFL likes having those things stick around.

Comment by David Rupp [ 01/Feb/14 6:57 PM ]

META is not used anywhere. I will explain better next time.

Also, DEREF_BANG has been commented out since 2007 (commit 139ddd146f2a272b7ddda397f54b501ff499c643). Figured it was pretty safe to get rid of at this point. My bad.





[CLJ-1302] keys and vals consistency not mentioned in docstring Created: 04/Dec/13  Updated: 14/Feb/14  Resolved: 14/Feb/14

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

Type: Enhancement Priority: Minor
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: docstring

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

 Description   

(keys m) and (vals m) appear to return stuff in a consistent order, so (= m (zipmap (keys m) (vals m))). This consistency is a useful property. The API docs should state whether it is part of the functions' contract.

Patch: clj-1302-2.patch



 Comments   
Comment by Gary Fredericks [ 11/Dec/13 7:18 PM ]

One thing to keep in mind is that the functions can be used on arbitrary instances of java.util.Map, which, aside from being mutable, could hypothetically (though not realistically) generate their entry sets nondeterministically.

I don't know what any of this means about what the docstring should say. It could claim the consistency for clojure's collections at least.

Comment by Andy Fingerhut [ 11/Dec/13 7:44 PM ]

The ticket creator might already realize this, but note that (= m (zipmap (keys m) (vals m))) is guaranteed for Clojure maps, where m is the same identical map, at least by the current implementation. I am not addressing the question whether it is part of the contract, but I think it would be good to make this explicit if it is part of the contract.

The following is not guaranteed for Clojure maps: (= m1 m2) implies that (= (keys m1) (keys m2)).

The set of keys/vals will be equal, but the order of keys/vals could be different for two otherwise equal maps m1, m2.

Comment by Steve Miner [ 27/Dec/13 11:10 AM ]

I think you can depend on a slightly stronger contract: The order of the results from `keys` and `vals` follows the order of the results from `seq`. As with any pure function, `seq` returns consistent results across multiple calls with the same (identical?) map. The order may be arbitrary for a non-sorted map, but it should be consistent.

Some time ago, I looked for this guarantee in the documentation, but I couldn't find it explicitly stated. However, after looking at the implementation, I think it's safe to depend on this invariant.

Comment by Stuart Halloway [ 31/Jan/14 12:48 PM ]

The absence of this property in the docs is correct. You should not rely on this.

Comment by Nicola Mometto [ 31/Jan/14 7:43 PM ]

I have to say this surprises me, I was relying on this undocumented behaviour expecting it to be implicit.

I did a quick search in github and the number of (zipmap (keys m) (do-something (vals m))) is significant, even some experienced clojure developers seem to have given this property for granted (https://groups.google.com/forum/?fromgroups#!topic/clojure/s1sFVF7dAVs).

Could we at least explicitely document the absence of this property in the docs in order to avoid further confusion?

Comment by Peter Taoussanis [ 01/Feb/14 3:21 AM ]

Big surprise here too. Could someone (Stu?) possibly motivate a little why this couldn't/shouldn't be a contractual property? It seems like it has utility. Perhaps more importantly, it seems to be an intuitively reasonable assumption. That's subjective, sure, but I feel like I've seen this pattern come up quite widely.

Anecdotally, am quite sure I've made use of the assumption before (i.e. that `(keys m)` and `(vals m)` will return sequences as per pair order).

Would need to review code to see how frequently I've made the error.

To clarify: not disagreeing, just want to understand the thought that's gone in.

> Could we at least explicitely document the absence of this property in the docs in order to avoid further confusion?

That'd be a big help I think. I'd generally take an

Comment by Peter Taoussanis [ 01/Feb/14 3:58 AM ]

End of comment got mangled somehow.

Was just going to point out that I'm a big fan of how cautious + deliberate Clojure's design tends to be. Being hesitant to pick up needless or half-baked contractual obligations, etc. is a huge strength IMO.

Comment by Rich Hickey [ 01/Feb/14 9:36 AM ]

keys order == vals order == seq order

Comment by Alex Miller [ 05/Feb/14 11:25 AM ]

Tweaked doc.





[CLJ-1301] case expression fails to match a BigDecimal Created: 23/Nov/13  Updated: 26/Jan/14  Resolved: 26/Jan/14

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

Type: Defect Priority: Blocker
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: Compiler

Attachments: Text File case-alt.patch     File clj-1301-1.diff    
Patch: Code and Test
Approval: Incomplete

 Description   

In 1.5.1 (anywhere before the CLJ-1118 patch), this is the behavior on BigDecimal case matching:

user=> (defn t [v] (case v 1 "Long" 1.0M "BigDecimal" "none"))
#'user/t
user=> (map t [1 1.0M 1.00M])
("Long" "BigDecimal" "none")

In 1.6 the behavior (post CLJ-1118 patch) has changed:

user=> (defn t [v] (case v 1 "Long" 1.0M "BigDecimal" "none"))
#'user/t
user=> (map t [1 1.0M 1.00M])
("Long" "none" "none")

In 1.6 after CLJ-1118, I expect to see: ("Long" "BigDecimal" "BigDecimal") as they now have the same hash and hasheq.

Cause: The case constants are hashed in the clojure.core/case macro using clojure.core/hash which calls clojure.lang.util/hasheq(). In Compiler.emitExprForHashes(), a call to clojure.lang.Util/hash(). In Clojure 1.5 these hash values are the same (hash of 1.0M == hasheq of 1.0M == 311). In Clojure 1.6, they are different (hash of 1.0M = 311, hasheq of 1.0M = 31).

In any cases where Java's hashCode and Clojure's hasheq return different values, the case statement can fail to do the correct thing.

Approach: Change Compiler.java to use clojure.lang.Util hasheq() to match the case macro use of clojure.core/hash (which calls clojure.lang.Util.hasheq()).

Patch: clj-1301-1.diff

Screened by:



 Comments   
Comment by Andy Fingerhut [ 23/Nov/13 5:00 PM ]

Patch clj-1301-1.diff modifies Compiler.java so that case* statements use hasheq on the test expression value, rather than Java's hashCode. It also adds a test case that currently fails with latest Clojure master, but passes with the patch.

Comment by Andy Fingerhut [ 23/Nov/13 5:01 PM ]

This bug is also the root cause for the recent failures of tests for the test.generative library.

Comment by Alex Miller [ 10/Dec/13 3:22 PM ]

Putting in 1.6 release per Rich.

Comment by Alex Miller [ 13/Dec/13 3:36 PM ]

Andy, I talked to Rich and the conclusion was that we should make the opposite change here such that the case macro should route to the Java hashcode version clojure.lang.util.hash() and the Compiler should be left as is. Can you update the patch?

Comment by Alex Miller [ 13/Dec/13 3:38 PM ]

And in case you were wondering, the reason is that the Java hashcode is generally faster (case is all about speed) and there are easy opportunities for you to properly cast your expression and/or case constants (where as the situations with collections where boxing is difficult to fix generically, that is not true).

Comment by Andy Fingerhut [ 13/Dec/13 5:14 PM ]

Alex, unless I am missing something, changing case to use Java's hashCode() would also require changing its current equality comparison from Clojure = (aka equiv()) to something consistent with hashCode(), which I think must be Java's equals().

Such a change would mean that all of the things that are = but not equals() will not match each other in a case statement, e.g. a case value of (Integer. 5) will not match a (Long. 5) value to compare against in a case branch.

Is that really what is desired here? I almost hesitate to create such a patch, for fear it might be committed

Comment by Alex Miller [ 17/Dec/13 12:06 PM ]

Based on discussion comments, move back to Incomplete until we resolve.

Comment by Alex Miller [ 16/Jan/14 9:37 AM ]

Added better example demonstrating the problem (the specific problem exposed by CLJ-1118).

Comment by Alex Miller [ 16/Jan/14 11:50 AM ]

Simplified examples.

Comment by Alex Miller [ 16/Jan/14 12:29 PM ]

Re Andy's comments above, I walked down that path a bit and built such a patch, however we currently have tests in clojure.test-clojure.control:

(testing "test number equivalence"
    (is (= :1 (case 1N 1 :1 :else))))

which clearly seems to expect Clojure equiv() behavior over Java equals() behavior in case constant matching. So either that is a bad test or this is not a viable approach (it also suggests we could break existing code with this change).

Comment by Andy Fingerhut [ 16/Jan/14 12:55 PM ]

One could consider having the default behavior of case to use hasheq and clojure.core/= everywhere, but add a 'fast' option to use hashCode and Java equals.

Comment by Alex Miller [ 24/Jan/14 9:46 AM ]

Alternative patch in the direction of using hashcode/equals instead of hasheq/equiv. Note that this test causes some test failures. This is not yet a candidate patch - further work needs to be done in evaluating this path.





[CLJ-1299] Binding conveyance seems to be broken as of 1.6.0-alpha2 Created: 21/Nov/13  Updated: 02/Dec/13  Resolved: 02/Dec/13

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

Type: Defect Priority: Blocker
Reporter: Peter Taoussanis Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None
Environment:

Clojure 1.6.0-alpha2
java version "1.7.0_04"
Java(TM) SE Runtime Environment (build 1.7.0_04-b21)
Java HotSpot(TM) 64-Bit Server VM (build 23.0-b21, mixed mode)


Approval: Vetted

 Description   

With Clojure 1.5:

(def ^:dynamic *num* 1)
(binding [*num* 2] (future (dotimes [_ 10] (println *num*))))

Behaves as expected: "2" prints 10 times. With Clojure 1.6.0-alpha2 the same form will print "1"s off the main thread.

This seems to be an interaction between loop/recur and the binding conveyance: the num binding does convey without the loop:

(def ^:dynamic *num* 1)
(binding [*num* 2] (future (println *num*))) ; Prints "2" even with 1.6.0-alpha2


 Comments   
Comment by Alex Miller [ 21/Nov/13 9:02 AM ]

I can confirm that the patch for this ticket is the one that introduced this behavior in 1.6:
http://dev.clojure.org/jira/browse/CLJ-1125

Comment by Alex Miller [ 02/Dec/13 9:57 PM ]

Marking as a dup now handled by the new patch in CLJ-1125 (and test included there).





[CLJ-1294] (vec (range)) causes REPL to hang (infinite loop?) Created: 11/Nov/13  Updated: 11/Nov/13  Resolved: 11/Nov/13

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

Type: Defect Priority: Minor
Reporter: Rene Semmelrath Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

LightTable and CoutnerClockWise
Windows7



 Description   

I just typed

(vec (range))

in LightTable and it stop working while CPU went to 100%
the same behavior is in Eclipse/Couterclockwise REPL



 Comments   
Comment by Andy Fingerhut [ 11/Nov/13 5:39 PM ]

Rene, this is expected to cause an infinite loop. (range) returns a lazy infinite sequence of integers. vec tries to cause the entire infinite sequence to be realized so it can put all elements into a vector. This is not a bug. You asked for an infinite loop, and you got one

Comment by Alex Miller [ 11/Nov/13 6:10 PM ]

(range) will yield an infinite sequence. vec will walk the sequence, conj'ing each item onto the vector ... which will do bad things.

While Clojure works well with infinite sequences (an abstraction), the collections like vector support only a finite number of elements.





[CLJ-1288] aset-* and aget: on multi-dimensional arrays (e.g. double[][]) these fn reflect (and, thus, perf. poorly) even with type hints. Created: 01/Nov/13  Updated: 01/Nov/13  Resolved: 01/Nov/13

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

Type: Defect Priority: Minor
Reporter: Michael O. Church Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: arrays, performance
Environment:

Clojure 1.5.1.

Dependencies: criterium



 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
nil

A (n ugly) 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
nil



 Comments   
Comment by Alex Miller [ 01/Nov/13 1:09 PM ]

Dupe of CLJ-1289.





[CLJ-1287] Change select-keys to return same type of map as it is given Created: 01/Nov/13  Updated: 14/Feb/14  Resolved: 14/Feb/14

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

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

Attachments: File clj-1287-1.diff     File clj-1287-3.diff    
Patch: Code and Test

 Description   

select-keys always returns a map built up from {}. This will sometimes not be the same type of map as the one given as the first arg, e.g. if the first arg is a sorted-map, the returned map will not be. There are situations where it would be nice if it preserved the kind of map it was given, e.g. the developer wishes to preserve the sorting order of the keys.

Discussion thread on Clojure group: https://groups.google.com/forum/#!topic/clojure/l_V1N1nRF-c



 Comments   
Comment by Andy Fingerhut [ 01/Nov/13 9:36 AM ]

clj-1287-1.diff changes the starting set {} to (empty map) in select-keys, and adds tests for the new behavior. The only reason select-keys is moved later in core.clj is to move it after the definition of function 'empty'. Before this patch 'select-keys' is defined before 'declare', even, so we cannot simply put a '(declare empty)' before select-keys.

Comment by Alex Miller [ 01/Nov/13 12:58 PM ]

What happens with records? Does this introduce a new failure case there (due to lack of empty support in records)?

Comment by Andy Fingerhut [ 01/Nov/13 5:43 PM ]

I had not thought about records when making the first patch. It does cause select-keys to fail if given a record, whereas without the patch select-keys works fine on records. clj-1287-2.diff is similar to the earlier patch, but if its argument is a record, it still returns a correct value based upon building up an unsorted map starting from {}. If its argument is not a record, it assumes empty will work on it and uses (empty map) to start with.

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

Patch clj-1287-3.diff is identical to the earlier clj-1287-2.diff 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 Alex Miller [ 14/Feb/14 1:14 PM ]

We do not wish to make this change.





[CLJ-1285] Persistent assoc/conj on a transient-created collision node Created: 28/Oct/13  Updated: 11/Nov/13  Resolved: 11/Nov/13

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

Type: Defect Priority: Critical
Reporter: Christophe Grand Assignee: Unassigned
Resolution: Completed Votes: 4
Labels: transient

Attachments: File persistent-assoc-after-collision.diff     File transient-generative-test.diff    
Patch: Code and Test
Approval: Ok

 Description   

Bug reported by Zach Tellman https://groups.google.com/d/msg/clojure-dev/HvppNjEH5Qc/1wZ-6qE7nWgJ

Since transients were introduced the invariant array.length == count*2 doesn't hold for HashCollisionNode.
However persistent .without still relies on it.

Hence persistent dissoc on a collision node created by transients fails.

(let [a (reify Object (hashCode [_] 42))
      b (reify Object (hashCode [_] 42))]
      (= (-> #{a b} transient (disj! a) persistent! (conj a))
       (-> #{a b} transient (disj! a) persistent! (conj a))))

returns false.

Patch: persistent-assoc-after-collision.diff

Generative test patch: transient-generative-test.diff

The generative test reliably reproduces the error. It is simpler than the original test that found the bug but tests a series conj/disj/transient/persistent actions on a set. I've included it separately in case we decide not to apply.

Screened by: Alex Miller



 Comments   
Comment by Michał Marczyk [ 29/Oct/13 9:58 PM ]

I can confirm that the patch works for me. As per our #clojure conversation, I've done the ClojureScript port; see CLJ-648.

Comment by Reid Draper [ 29/Oct/13 11:28 PM ]

I've run Zach's original test, as well as my own simple-check test. Both are passing.

Comment by Alex Miller [ 30/Oct/13 9:33 AM ]

I don't suppose we could get a generative test (prob need to use test.generative which is already included) to test this stuff similar to the original test that found the bug?

Very much hoping to get this into 1.6.

Comment by Andy Fingerhut [ 31/Oct/13 10:52 AM ]

Alex, I suspect clojure-dev would reach a much wider audience for your request than a comment on this ticket, which only has 3 watchers, and I don't think many people besides you and I watch the stream of all ticket state changes as they go by.

Comment by Michał Marczyk [ 01/Nov/13 5:19 AM ]

Just wanted to note that this patch, apart from preventing the hash-based collections from failing Zach's test suite, also makes avl.clj collections pass (now that I've released fixes for the two bugs uncovered by the test suite in avl.clj 0.0.9). This provides some cross-validation, I think.

(The built-in sorted collections pass either way, because they don't support transient ops.)

Also, David Nolen has merged the ClojureScript port of the patch.

Comment by Alex Miller [ 01/Nov/13 7:35 AM ]

I'm going to take a crack at repro with test.generative this morning - wish me luck!

Comment by Alex Miller [ 03/Nov/13 10:40 PM ]

Added a simplified version of a test-generative test and marked screened.

Comment by Alex Miller [ 11/Nov/13 11:17 AM ]

Patch was applied to master for 1.6.





[CLJ-1281] Clojure 1.6 - reconsider what is "alpha" in core Created: 23/Oct/13  Updated: 22/Nov/13  Resolved: 22/Nov/13

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: docstring

Attachments: Text File alpha.patch    
Patch: Code
Approval: Ok

 Description   

In Clojure 1.5.1, the following things are marked as "alpha - subject to change". We should consider this list and whether some of them are no longer alpha and update them appropriately.

  • Watches (1.0): add-watch, remove-watch
  • Transients (1.1): transient, persistent!, conj!, assoc!, dissoc!, pop!, disj!
  • Exceptions (1.4): ex-info, ex-data
  • Promises (1.1): promise, deliver
  • Compiler warnings (1.4): :disable-locals-clearing
  • Records (1.3) defrecord
  • Types (1.3): deftype
  • Pretty print (1.3): print-table
  • clojure.reflect (1.3) (all)
  • Reducers (1.5) (all)

Patch: alpha.patch

  • Removes alpha marking for everything except reducers, disable-locals-clearing, and clojure.reflect. If Stu wants to remove for clojure.reflect, he should do so.

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 04/Nov/13 8:28 AM ]

Pulling into 1.6 as Rich has given me some feedback on what to change here.

Comment by Alex Miller [ 07/Nov/13 10:29 AM ]

Added patch that removes alpha designation from everything but reducers, disable-locals-clearing, and clojure.reflect (still TBD).

Comment by Andy Fingerhut [ 07/Nov/13 10:58 AM ]

definline is marked experimental in its doc string, and has been marked so since Clojure 1.0. Is it ready to be 'promoted', too?

Comment by Alex Miller [ 07/Nov/13 11:03 AM ]

Excellent question, will find out.

Comment by Alex Miller [ 07/Nov/13 12:40 PM ]

Rich says definline is still experimental, so no change.





[CLJ-1276] Can't make a dispatch map containing forward-declared fns Created: 09/Oct/13  Updated: 18/Apr/14  Resolved: 18/Apr/14

Status: Closed
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: Declined Votes: 0
Labels: None

Attachments: File unbound-eg.tgz    

 Description   

If from (ns tst2) you try to call tst1/c, which calls tst1/f via dispatch map which was defined when tst1/f was forward declared, you get an "unbound fn" error. E.g.

user=> (dorun (map eval 
                   '[(ns tst1) 
                     (declare f) 
                     (def d {:k f}) 
                     (defn c [] ((d :k)))
                     (defn f [] :success)
                     (ns tst2 (:require [tst1]))
                     (tst1/c)]))

IllegalStateException Attempting to call unbound fn: #'tst1/f  clojure.lang.Var$Unbound.throwArity (Var.java:43)
tst2=> (clojure.repl/pst *e)
IllegalStateException Attempting to call unbound fn: #'tst1/f
	clojure.lang.Var$Unbound.throwArity (Var.java:43)
	tst1/c (NO_SOURCE_FILE:5)
	tst2/eval25 (NO_SOURCE_FILE:8)
	clojure.lang.Compiler.eval (Compiler.java:6642)
	clojure.lang.Compiler.eval (Compiler.java:6605)
	clojure.core/eval (core.clj:2883)
	clojure.core/map/fn--4222 (core.clj:2513)
	clojure.lang.LazySeq.sval (LazySeq.java:40)
	clojure.lang.LazySeq.seq (LazySeq.java:49)
	clojure.lang.RT.seq (RT.java:484)
	clojure.core/seq (core.clj:133)
	clojure.core/dorun (core.clj:2811)


 Comments   
Comment by Alex Coventry [ 09/Oct/13 10:43 PM ]

TEttinger pointed out on IRC that the forms in the example run without error if you wrap them in a (do) block. Here is an example using files. Relevant code is in src/unbound_eg/tst[12].clj. Example output shown below.

http://clojure-log.n01se.net/date/2013-10-09.html#23:52

lap% lein repl
nREPL server started on port 50125 on host 127.0.0.1
REPL-y 0.2.1
Clojure 1.5.1
Docs: (doc function-name-here)
(find-doc "part-of-name-here")
Source: (source function-name-here)
Javadoc: (javadoc java-object-or-class-here)
Exit: Control+D or (exit) or (quit)

user=> (require '[unbound-eg.tst2 :as t2])

IllegalStateException Attempting to call unbound fn: #'unbound-eg.tst1/f clojure.lang.Var$Unbound.throwArity (Var.java:43)
user=> (pst)
IllegalStateException Attempting to call unbound fn: #'unbound-eg.tst1/f
clojure.lang.Var$Unbound.throwArity (Var.java:43)
unbound-eg.tst1/c (tst1.clj:4)
unbound-eg.tst2/eval2233 (tst2.clj:3)
clojure.lang.Compiler.eval (Compiler.java:6619)
clojure.lang.Compiler.load (Compiler.java:7064)
clojure.lang.RT.loadResourceScript (RT.java:370)
clojure.lang.RT.loadResourceScript (RT.java:361)
clojure.lang.RT.load (RT.java:440)
clojure.lang.RT.load (RT.java:411)
clojure.core/load/fn--5018 (core.clj:5530)
clojure.core/load (core.clj:5529)
clojure.core/load-one (core.clj:5336)
nil

Comment by Kevin Downey [ 18/Apr/14 12:23 AM ]

this is just a fact of clojure's compilation model and how vars work.

a var is a little mutable cell

(declare foo) declares that a mutable cell exists with the name foo, it doesn't contain a value

foo then gets the value of the mutable cell (which has none)

(defn foo [] 1) then sets the value of the cell named foo to the function created from (fn [] 1)

Comment by Alex Miller [ 18/Apr/14 7:29 AM ]

I agree with Kevin - this is expected behavior.





[CLJ-1273] def discards ^:macro when used outside the top-level Created: 02/Oct/13  Updated: 17/Jan/14  Resolved: 17/Jan/14

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

Type: Defect Priority: Major
Reporter: André Gustavo Rigon Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: :macro, conditional, def, macro, metadata, top-level


 Description   

If I evaluate

(def ^:macro my-defn1 #'defn)

a macro named 'my-defn1' is defined, which I can use just like 'defn'.

However, if I evaluate instead

(if true
  (def ^:macro my-defn2 #'defn))

the var for 'my-defn2' doesn't have the :macro metadata set and I can't use it as a macro, even though the 'def' form is equal to the previous case.

Here is a complete example:

(def ^:macro my-defn1 #'defn)

(if true
  (def ^:macro my-defn2 #'defn))

(println (meta #'my-defn1))    ; => contains :macro

(println (meta #'my-defn2))    ; => doesn't contain :macro!

(my-defn1 hello1 []
          (println "hello 1"))

(hello1)                       ; => prints "hello 1"

(my-defn2 hello2 []            ; => CompilerException: Unable to resolve 
  (println "hello 2"))         ;    symbol: hello2 in this context


 Comments   
Comment by Gary Fredericks [ 09/Dec/13 1:31 PM ]

I hopped around the code for a while and all I could find is that the bindRoot method in Var.java intentionally clears :macro from the metadata.

Comment by Nicola Mometto [ 17/Jan/14 7:03 PM ]

Duplicate of http://dev.clojure.org/jira/browse/CLJ-1021





[CLJ-1271] Reduce protocol callsite overhead Created: 30/Sep/13  Updated: 08/Oct/13  Resolved: 08/Oct/13

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

Type: Enhancement Priority: Critical
Reporter: Alex Miller Assignee: Rich Hickey
Resolution: Completed Votes: 0
Labels: None

Approval: Ok

 Description   

Placeholder for Rich to work on the unused instance member fields emitted by emitProto() in the compiler.






[CLJ-1270] Print control for Java collection types Created: 30/Sep/13  Updated: 18/Oct/13  Resolved: 18/Oct/13

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

Type: Enhancement Priority: Critical
Reporter: Alex Miller Assignee: Stuart Halloway
Resolution: Duplicate Votes: 0
Labels: None

Attachments: Text File clj-1270-1.txt    
Approval: Vetted

 Description   

This feature is described at:
http://dev.clojure.org/display/design/Print+Control+for+Java+Collection+Types



 Comments   
Comment by Andy Fingerhut [ 14/Oct/13 2:08 AM ]

clj-1270-1.txt is just a quick swipe at changing the printing behavior for Java objects with classes implementing the java.util.Queue interface, so pr/pr-str shows them similarly to how they show Clojure lists and seqs. java.util.Set and java.util.List already pr/pr-str similarly to Clojure lists, vectors, or sets.

No change is made in this patch to the behavior of print/print-str, or to how other objects are printed whose classes implement java.util.Collection, but not also one of java.util.Set, .List, or .Queue.

A few tests are added to verify the desired behavior when print-length is set to a value other than nil.

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

Just a comment: Clojure 1.5.1's behavior regarding print-readably and Java collections does seem a bit odd, perhaps backwards:

user=> (import '[java.util HashSet])
java.util.HashSet
user=> (def x (HashSet. (range 10)))
#'user/x
user=> (binding [*print-readably* true] (pr-str x))
"#{0 1 2 3 4 5 6 7 8 9}"
user=> (binding [*print-readably* false] (pr-str x))
"#<HashSet [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]>"
Comment by Alex Miller [ 18/Oct/13 7:59 AM ]

Rich and Stu said this should be in scope for 1.6. Rich said that Stu would likely do the work.

Comment by Stuart Halloway [ 18/Oct/13 8:41 AM ]

This was actually implemented in 1.5. Andy's suggestion about Queue (see patch and https://groups.google.com/forum/#!topic/clojure-dev/q7h4QJCHEvw) could be considered as a separate feature request.

Comment by Stuart Halloway [ 18/Oct/13 8:42 AM ]

This work was done in commit 92b4fc76e59e68d3bdae4ebd5b8deec915cb7ab5

Comment by Alex Miller [ 18/Oct/13 9:21 AM ]

Ah, sweet - thanks Stu!





[CLJ-1269] RFC: Anonymous functions interaction with -> and ->> threading macros Created: 28/Sep/13  Updated: 09/Dec/13  Resolved: 09/Dec/13

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

Type: Enhancement Priority: Minor
Reporter: Michael O. Church Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

This is more of a starting point for discussion than a feature request. It'd be easy to write and submit the patch, but I want to ask if it's a good idea, since it would alter the semantics of two core macros, '> and '>>; and if it is a good idea, what considerations need to be balanced.

For threading macros, I'd like to special-case forms that begin with 'fn and 'fn*. It's often useful (but maybe a bad idea; that's why I'd like to start the discussion) to use the threading macros in conjunction with anonymous functions in addition to forms, like so (contrived example):

(defn sigmoid [x] (-> x 
                      - 
                      Math/exp 
                      (+ 1) 
                      #(/ 1 %)))

This won't compile; #(/ 1 %) expands to the form

(fn* [p1__1389#] (/ 1 p1__1389#))

modulo gensym, of course. The threading macro, not treating fn* and fn specially, alters that to:

(fn* (clojure.core/-> (clojure.core/-> (clojure.core/-> x -) Math/exp) (+ 1)) [p1__1417#] (/ 1 p1__1417#))

which is a fn* with a non-symbol (illegal label) before its binding vector, raising an error.

Is this worth "fixing", or are the benefits to small to justify the added complexity of a special case in the ->, ->> threading macros?



 Comments   
Comment by Michael O. Church [ 29/Sep/13 8:52 AM ]

Thanks Alex!

Comment by Gary Fredericks [ 09/Dec/13 7:53 AM ]

Just one of the special cases you'd want to consider is when fn does not mean clojure.core/fn, which is certainly realistic.

Comment by Michael O. Church [ 09/Dec/13 10:03 AM ]

Yes, I realize that.

Looking at the ticket again, I retract my support of this change.

Here's why: (1) that "fix" would alter the semantics of the threading macros, which would break existing code, and (2) there's a really easy way to get anonymous functions into threading macros: just surround the

#(...)
expression with an extra set of parentheses.

I retract my request for this feature. It's easy enough to do this: instead of,

(-> x f #(g h %))

it's this:

(-> x f (#(g h %)))

Saving 2 characters, like so, is not worth a breaking change IMO.

Comment by Alex Miller [ 09/Dec/13 10:15 AM ]

Retracted by submitter





[CLJ-1268] Require Java 1.6 as minimum for Clojure Created: 28/Sep/13  Updated: 25/Oct/13  Resolved: 25/Oct/13

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

Type: Enhancement Priority: Blocker
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: build

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

 Description   

In Clojure 1.6, we plan to move to JDK 1.6 as the minimum JDK.

Patch: clj-1268.patch

This patch changes the build configurations for both Maven and Ant to assume JDK 1.6 as the "source" and "target" runtimes.

Configuration changes will be necessary on Hudson. We already build Clojure and contrib libraries on JDK 1.6 by default, but we will need to remove matrix test builds for JDK 1.5. See for example clojure-test-matrix and data.csv-test-matrix – coordinate with Stuart Sierra for this change.



 Comments   
Comment by Stuart Sierra [ 04/Oct/13 8:45 AM ]

Screened.

I have verified that both the Ant and Maven builds still work (running on JDK 1.7) and that the output .class files contain the bytecode header for JDK 1.6.





[CLJ-1267] Reader Bug for making vector of numbers Created: 26/Sep/13  Updated: 26/Sep/13  Resolved: 26/Sep/13

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

Type: Defect Priority: Major
Reporter: Amin Razavi Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: bug
Environment:

Windows 8 , Core i7



 Description   

When Trying To Make a Vector of Numbers Like 01 04 ... it's OK!
=> (vector 04)
; [4]
But When Trying To Make a Vector of "09" It Says :
NumberFormatException Invalid Number: 09



 Comments   
Comment by Alex Miller [ 26/Sep/13 10:56 AM ]

Numbers with a leading 0 are read as octal (valid digits = 0-7). Example:

> 0100
64

So, this is the expected behavior.

Comment by Amin Razavi [ 26/Sep/13 7:49 PM ]

shame on me , sorry.





[CLJ-1265] various clojure.core functions with ^:static metadata are missing doc strings Created: 23/Sep/13  Updated: 05/Feb/14  Resolved: 05/Feb/14

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

Type: Defect Priority: Trivial
Reporter: Jozef Wagner Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

Several functions defined with legacy ^:static metadata are not shown in API documentation. Examples are chunk-cons or await1. This defect probably originates in autodoc.



 Comments   
Comment by Tom Faulhaber [ 23/Sep/13 9:25 AM ]

These functions have no doc strings. Autodoc doesn't include Vars without documentation by design.

I'll leave it to others to decide if the functions should have doc strings or not. If they are meant for general use, I would say that the answer is "yes."

Comment by Alex Miller [ 05/Feb/14 12:33 PM ]

The autodoc behavior is intentional. The omission of docstrings is intentional (these are internal functions).





[CLJ-1264] Minor change to Clojure source code to prevent warnings when compiled with JDK 8 Created: 17/Sep/13  Updated: 22/Nov/13  Resolved: 22/Nov/13

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

Type: Enhancement Priority: Critical
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

JDK 8


Attachments: File clj-1264-1.diff     Text File clj-1264-1.txt    
Patch: Code
Approval: Ok

 Description   

When compiling the Clojure source code using the early access version of JDK 8 (I saw this with 1.8.0-ea-b103 in particular), there are 6 warnings produced because the character _ is used as an identifier in Java source code.

    [javac] /home/jafinger/clj/latest-clj/clojure/src/jvm/clojure/lang/PersistentHashMap.java:1089: warning: '_' used as an identifier
    [javac] 	Box _ = new Box(null);
    [javac] 	    ^
    [javac]   (use of '_' as an identifier might not be supported in releases after Java SE 8)

The warning implies that this is likely to continue to be a warning for the lifetime of JDK 8, and could become an error with later JDKs.

Eliminating these warnings is as simple as changing the identifier name used on those 6 lines of one Java source file.

Patch: clj-1264-1.diff

Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 17/Sep/13 11:23 PM ]

Patch clj-1264-1.txt changes the identifier _ used on 6 lines of file PersistentHashMap.java to the name addedLeaf, which is used for a similar purpose elsewhere in the file.





[CLJ-1262] Set equality broken with BigInteger Created: 13/Sep/13  Updated: 13/Sep/13  Resolved: 13/Sep/13

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

Type: Defect Priority: Major
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

The CLJ-1106 bug is still present when using BigInteger:

(let [n1 -5, n2 (BigInteger. "-5")]
  [(= n1 n2) (= #{n1} #{n2}) (= [n1] [n2])])
;; => [true false true]

Andy Fingerhut pointed out that this may be out of scope in the same way as CLJ-1036.



 Comments   
Comment by Alex Miller [ 13/Sep/13 1:14 PM ]

Yes, Rich considers hash/= consistency out of scope for BigInteger.





[CLJ-1260] ConcurrentModificationException thrown during action dispatching after commit in LockingTransaction.run() Created: 12/Sep/13  Updated: 21/Jan/14  Resolved: 21/Jan/14

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

Type: Defect Priority: Major
Reporter: Brandon Ibach Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: STM
Environment:

Discovered on Ubuntu 12.04 with Oracle JDK 1.7.0_25 running Clojure 1.4. Test case produced on Windows 8 with Oracle JDK 1.7.0_09 running Clojure 1.4 and Clojure 1.5.1. Analysis seems to indicate that OS and Java version are not critical. Clojure 1.6 pre-release code has not been tested, but since clojure.lang.LockingTransaction has not changed since Clojure 1.4, it seems likely the defect is still present.


Attachments: File clj-1260.diff     File clj-1260-fixws.diff     Java Source File STMAgentInitBug.java    
Patch: Code
Approval: Vetted

 Description   

Summary

Using the Clojure 1.4 library strictly from Java code, a simple transaction dispatches an action to an Agent. When called from a simple driver, such as a unit test, where there is no interaction with the Clojure library/runtime (specifically, clojure.lang.RT), a ConcurrentModificationException is thrown from inside LockingTransaction.run() while it is iterating through the actions list, dispatching each action to its Agent after committing the transaction.

While the circumstances under which this occurs are probably fairly rare and a simple workaround exists (see final paragraph), thus the "Minor" priority, it seems like it would not be very complicated to fix LockingTransaction to handle the actions list more safely.

Analysis

Based on some debugging, here's what seems to be happening:

  1. Transaction A is run, dispatching action Z, which gets added, via LockingTransaction.enqueue(), to the actions list, which is a java.util.ArrayList<Agent.Action>.
  2. Transaction A completes and is successfully committed.
  3. LockingTransaction.run() does post-commit cleanup, freeing locks and putting a stop() to transaction A, which nulls the transaction's Info object reference.
  4. Notifications are sent and we start iterating the list of actions to be dispatched.
  5. The run() method calls Agent.dispatchAction(). Because the thread's transaction object is no longer considered to be "running" (due to the Info object being null) and no action is being processed on the thread (so its nested vector is null), the action is enqueue()-ed with the Agent.
  6. As part of the enqueue() process, the action is cons()-ed onto the Agent's ActionQueue. Here's where the unique circumstances come into play.
    1. At this point, we haven't really interacted with the Clojure runtime, specifically the clojure.lang.RT class, so its initiation machinery kicks in.
    2. Down in the depths, it executes transaction B to add a library to its list of loaded libraries.
    3. The still-existing-but-not-running thread-local transaction object fires up, runs and commits, with its existing, intact actions list, still containing action Z, enqueued during transaction A, which has not yet finished its post-commit process.
    4. The post-commit process for transaction B runs, including a nested attempt to dispatch action Z, again, which succeeds.
    5. The actions list is cleared before exiting the run() method.
  7. Upon returning way back up the stack to our not-quite-finished-post-processing transaction A, we continue iterating the now-cleared actions list, which promptly throws the ConcurrentModificationException.

A quick perusal of the LockingTransaction code shows that the only interaction with the actions list is adding an item to it in the LockingTransaction.enqueue() method, iterating it in the post-processing section of run() and clearing it in the finally clause of that section, so it's easy to see how a transaction started by any of the action-dispatching machinery would cause problems. Any such activity in the actions themselves would not be an issue, since they'd occur on other threads, but the dispatch stuff all runs on the same thread. The few moving parts that occur in this code seem fairly safe, as long as the runtime, clojure.lang.RT, is already initialized, but if that occurs during this phase, all bets appear to be off.

Test Case

The attached Java class can be compiled and run with just the Clojure 1.4 JAR on the class path. With the change described near the end of the file (comment one line and uncomment another), the Clojure 1.5.1 JAR can be used, instead, producing the same result.

A single Agent named count is created, holding an Integer value of 1. A transaction is run which dispatches an action (referred to as Z in the above description) that will increment the value of count to 2. Following this, another action is dispatched to count to enable monitoring the completion of the incrementing action. Lastly, the final value of count is printed before the application exits.

Running the class with no command-line arguments produces the above-mentioned exception and prints an incorrect final result, due to action Z being run a second time as described in step 6.4. Running with any command-line argument triggers a simple workaround that just references a static value from the clojure.lang.RT class, which invokes the class initialization before anything else happens, such that the exception is not thrown and the correct result is produced.

Patch: clj-1260-fixws.diff

Screened by:



 Comments   
Comment by Alex Miller [ 12/Sep/13 3:29 PM ]

Rich asked me to move this one to Vetted/Release 1.6 when it came in.

Comment by Alex Miller [ 12/Sep/13 3:31 PM ]

FYI: submitter does not have a CA

Comment by Christophe Grand [ 18/Oct/13 4:09 AM ]

Would a patch as simple as triggering the load of RT from the static init of LockingTransaction be ok? (assuming the analysis is correct)

Comment by Guillermo Winkler [ 18/Oct/13 1:07 PM ]

The ConcurrentModificationException is raised because the list of `actions` protocol is being violated.

Last clojure version happens in this loop in LockingTransaction.run(Callable) line: 361

for(Agent.Action action : actions)

{ Agent.dispatchAction(action); }

}

When RT hasn't been initialized before, first time it's used is in PersistentQueue's cons RT.list(o)

public PersistentQueue cons(Object o){ if(f == null) //empty return new PersistentQueue(meta(), cnt + 1, RT.list(o), null); else return new PersistentQueue(meta(), cnt + 1, f, (r != null ? r : PersistentVector.EMPTY).cons(o)); }

With the following call stack.

RT.doInit() line: 447
RT.<clinit>() line: 329
PersistentQueue.cons(Object) line: 127
PersistentQueue.cons(Object) line: 24
Agent.enqueue(Agent$Action) line: 264
Agent.dispatchAction(Agent$Action) line: 255
LockingTransaction.run(Callable) line: 369
LockingTransaction.runInTransaction(Callable) line: 231
STMAgentInitBug.main(String[]) line: 26

Problem is RT initialization triggers a new runInTransaction for new class loading (protocols in this callstack)

Thread [main] (Suspended (breakpoint at line 361 in LockingTransaction))
LockingTransaction.run(Callable) line: 361
LockingTransaction.runInTransaction(Callable) line: 231
protocols__init.load() line: 9
protocols__init.<clinit>() line: not available
Class<T>.forName0(String, boolean, ClassLoader) line: not available [native method]
Class<T>.forName(String, boolean, ClassLoader) line: 247
RT.loadClassForName(String) line: 2099
RT.load(String, boolean) line: 430
RT.load(String) line: 411
core.clj line: 5546
core.clj line: 5545
core$load(RestFn).invoke(Object) line: 408
core__init.load() line: 6174
core__init.<clinit>() line: not available
Class<T>.forName0(String, boolean, ClassLoader) line: not available [native method]
Class<T>.forName(String, boolean, ClassLoader) line: 247
RT.loadClassForName(String) line: 2099
RT.load(String, boolean) line: 430
RT.load(String) line: 411
RT.doInit() line: 447
RT.<clinit>() line: 329
PersistentQueue.cons(Object) line: 127
PersistentQueue.cons(Object) line: 24
Agent.enqueue(Agent$Action) line: 264
Agent.dispatchAction(Agent$Action) line: 255
LockingTransaction.run(Callable) line: 369
LockingTransaction.runInTransaction(Callable) line: 231
STMAgentInitBug.main(String[]) line: 26

So LockingTransaction:run depends indeed on RT to be loaded, since the method re-enters the function if loading is triggered from the inside.

Maybe actions:

final ArrayList<Agent.Action> actions = new ArrayList<Agent.Action>();

Should be better protected to prevent enqueuing during the dispatch loop?

Comment by Brandon Ibach [ 18/Oct/13 2:02 PM ]

The additional detail provided in the last comment is correct and matches what I observed. However, protecting the "actions" list against enqueuing isn't really the issue, since this problem would occur even if the "nested" transaction didn't involve any actions. The problem is that committing a transaction clears the "actions" list.

I think the suggestion to trigger initialization of RT in the static initializer of LockingTransaction would address this issue, though in a somewhat roundabout way. A more direct approach might be to make a temporary copy of the contents of the "actions" list prior to iterating it. Obviously, the latter would make extra work on every transaction commit, so it may not be desirable. I don't know what impact the former approach would have, other than the long-term effect of it being non-obvious to a later observer why that code is needed.

I haven't thought this through completely or done any tests, so my reasoning may be wrong, but one more argument for the approach of making a copy of the "actions" list is that a Ref notification/watch could run a transaction, which, being on the same thread, would also clear the "actions" list. This case wouldn't cause the exception, since the iteration of the list would not have started, yet, but it would cause actions enqueued by the "outer" transaction to not be dispatched.

Comment by Guillermo Winkler [ 18/Oct/13 2:26 PM ]

You're right, problem is not on enqueue are inner transactions clearing the list.

Anyway the protection can be a smarter way of iteration.

Wouldn't a possible solution be treating the action list as a Queue? Pop'in each action would also clear only that action.

Comment by Guillermo Winkler [ 18/Oct/13 2:57 PM ]

Attached patch seems to solve the problem in a cleaner way.

Comment by Andy Fingerhut [ 19/Oct/13 5:32 PM ]

Patch clj-1260-fixws.diff is identical to Guillermo's clj-1260.diff, except it eliminates warnings when applying the patch that were due to trailing white space.

Comment by Stuart Sierra [ 17/Jan/14 1:27 PM ]

Here's my understanding of what happens:

1. User's Java code calls LockingTransaction.runInTransaction.
2. In the transaction's Callable, user calls Agent.dispatch.
3. LockingTransaction adds the agent action to its actions list.
4. The transaction commits.
5. LockingTransaction begins iterating over actions and dispatching them.
6. Agent.dispatchAction checks to see if there is a running transaction, which there is not.
7. Agent.dispatchAction line 255 calls Agent.enqueue.
8. Agent.enqueue line 264 calls PersistentQueue.cons.
9. PersistentQueue.cons line 127 calls RT.list, triggering initization of RT.
10. RT's static initializer calls doInit.
11. doInit loads core.clj.
12. core.clj loads other files which contain ns declarations.
13. ns expands to code which commutes loaded-libs in a transaction.
14. The new transaction gets the same ThreadLocal LockingTransaction.
15. The new transaction executes and clears actions.
16. Back up the stack in the original transaction, LockingTransaction tries to continue iterating over actions and throws ConcurrentModificationException.

In short, this situation can only occur when the mere act of
enqueueing agent actions, not executing them, can create another
transaction on the same thread. This can happen when clojure.lang.RT
has not been initialized.

This would never occur in a Clojure program because RT is already
initialized when the Clojure program starts, and none of the Java code
involved in enqueueing agent actions will create new transactions.

The workaround for Java is trivial: just make sure clojure.lang.RT is
initialized before using any other Clojure classes.

However, the patch clj-1260-fixws.diff is a small and safe change. Its
author, Guillermo Winkler, has a CA.

The approach is to treat the actions list in LockingTransaction
as a queue, popping actions off one at a time, and never to
explicitly clear the queue.

The more important question is if/when RT should be initialized
automatically.

Comment by Guillermo Winkler [ 17/Jan/14 1:46 PM ]

Solving this problem by initializing RT feels to me like complecting two things not necessarily related, if PersistentQueue stops using RT, relationship disappears and you may still have the bug.

Anyway RT could be initialized safely so anyone that may depend on it finds it already there, avoiding potential clashes on initialization.

Comment by Alex Miller [ 17/Jan/14 1:49 PM ]

And following up on that, Clojure 1.6 has a new public Java API in clojure.java.api.Clojure. Loading that class or using any method on the class will serve to load clojure.lang.RT as a side effect.

Comment by Guillermo Winkler [ 21/Jan/14 9:11 AM ]

Alex, do you want a patch for the safe RT initialization here? Or do you prefer to handle it using a different ticket?

Comment by Alex Miller [ 21/Jan/14 9:32 AM ]

When you are using Clojure from Java, it is expected that you should properly initialize the Clojure runtime by (at least) loading the RT class (for Clojure <1.6) or the Clojure class (for Clojure >= 1.6).





[CLJ-1258] (apply and [false true]) gives CompilerException Created: 08/Sep/13  Updated: 09/Sep/13  Resolved: 09/Sep/13

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

Type: Defect Priority: Major
Reporter: Cynthia Qiu Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: bug
Environment:

Ubuntu 13.04. I run 1.4.0 by "apt-get install", and 1.5.1 by "java -cp clojure-1.5.1.jar clojure.main".



 Description   

user=> (apply and [false true])
CompilerException java.lang.RuntimeException: Can't take value of a macro: #'clojure.core/and, compiling:(NO_SOURCE_PATH:1)
user=> (apply and (list false true))
CompilerException java.lang.RuntimeException: Can't take value of a macro: #'clojure.core/and, compiling:(NO_SOURCE_PATH:2)



 Comments   
Comment by Cynthia Qiu [ 08/Sep/13 10:18 PM ]

I got it. "and" is not a function but a special form. Sorry for that.

Comment by Andy Fingerhut [ 08/Sep/13 11:57 PM ]

Minor comment on yours: Clojure and other Lisps often distinguish between "special forms" and macros. I may be missing some of the distinction, but I am pretty sure the main part is that special forms are usually built into the implementation of Lisp, whereas macros are typically defined by the user via defmacro.

In any case, neither special forms nor macros may be used with apply, only functions.

Comment by Alex Miller [ 09/Sep/13 1:26 AM ]

As per Andy's comment, this is expected behavior when trying to apply to macros.





[CLJ-1252] Clojure reader (incorrectly) accepts keywords starting with a number Created: 04/Sep/13  Updated: 31/Oct/13  Resolved: 31/Oct/13

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

Type: Defect Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: reader

Attachments: Text File numkeyword.patch    
Patch: Code and Test
Approval: Screened

 Description   

The reader page at http://clojure.org/reader states that symbols (and keywords) cannot start with a number and the regex used in LispReader (and EdnReader) also has this intention. But:

user> :5
:5
user> (class *1)
clojure.lang.Keyword

Cause: The pattern for keywords is: "[:]?([\D&&[^/]].*/)?(/|[\D&&[^/]][^/]*)". The intention of the [\D&&[^/]] is to accept all non-digits except /. However, if the : matches and 5 does not, the regex will backtrack and unmatch the :, instead matching it in the non-digit charset. The whole match is sent on in the code and the : is stripped later.

Solution: Prevent back-tracking for the first : match by using the possessive quantifier ?+ instead of ?. Once the first : is matched, it will not backtrack to it. (This is also faster.) The patch makes this change in LispReader and EdnReader, updates a couple tests that were using number keywords, and adds a new negative test to check that :5 isn't read.

Patch: numkeyword.patch

Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 05/Sep/13 5:25 PM ]

Given how far this one is along now, perhaps CLJ-1003 should be marked as a duplicate of this one?

Comment by Alex Miller [ 31/Oct/13 9:39 PM ]

Because this broke existing code (several lib projects like java.jdbc), we have rolled this back. Will follow up with an alternate ticket to address this in some other way.





[CLJ-1249] Warning when a record field with the same name as a function exists for it Created: 26/Aug/13  Updated: 31/Jan/14  Resolved: 31/Jan/14

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

Type: Enhancement Priority: Minor
Reporter: Benjamin Peter Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: errormsgs
Environment:

Linux, clojure jar or leiningen repl



 Description   

Hi,

I had the following problem which took me much longer than it should have. I accidentally had a record's field with the same name as one of the functions from a protocol. When I tried to call the function from within the record I got totally weird behavior and didn't find it, until I removed every piece of code when I found the name conflict.

I wish clojure would warn me in such a case. (Disregarding any naming conventions that could have saved me.)

Following a small example to reproduce the problem:

(defprotocol HasPets
  (dogs [this])
  (cats [this])
  (octopus [this])
  (cute-ones [this]))

; Here the field "dog" is added with the same name as the protocol
(defrecord Petshop [dogs] 
  HasPets
  (dogs [this]
    [:pluto :bethoven])
  (cats [this]
    [:tom])
  (octopus [this]
    [:henry])
  (cute-ones [this]
    ; Here it was intended to call the function "dogs", instead the
    ; field is used.
    (concat (dogs this) (cats this))))

(def dogs-in-stock nil)

(let [petshop (->Petshop dogs-in-stock)]
  (println "Dogs for sale:" (dogs petshop))
  (println "All cute animals: " (cute-ones petshop)))

Results in:

clojure core.clj
Dogs for sale: [:pluto :bethoven]
Exception in thread "main" java.lang.NullPointerException
        at user.Petshop.cute_ones(core.clj:20)
        at user$eval84.invoke(core.clj:26)
        at clojure.lang.Compiler.eval(Compiler.java:6514)
        at clojure.lang.Compiler.load(Compiler.java:6955)
        at clojure.lang.Compiler.loadFile(Compiler.java:6915)
        at clojure.main$load_script.invoke(main.clj:283)
        at clojure.main$script_opt.invoke(main.clj:343)
        at clojure.main$main.doInvoke(main.clj:427)
        at clojure.lang.RestFn.invoke(RestFn.java:408)
        at clojure.lang.Var.invoke(Var.java:415)
        at clojure.lang.AFn.applyToHelper(AFn.java:161)
        at clojure.lang.Var.applyTo(Var.java:532)
        at clojure.main.main(main.java:37)

Expected warning:

Warning: the protocol function "dog" from "HasPets" conflicts with the equally named record field of "Petshop"

Without dog as a record's field:

clojure core.clj
Dogs for sale: [:pluto :bethoven]
All cute animals:  (:pluto :bethoven :tom)

Thanks for your help.

Ben.



 Comments   
Comment by Alex Miller [ 26/Aug/13 1:20 PM ]

Thanks for the report.

It seems like there is a scoping issue here where dogs is bound to the field and there is also a context for it being referred to as a function. It's possible that this should really be treated as a compiler bug and not a warning message problem - that requires some more detective work. Since there is only one function dogs (the field name is a function only in keyword form - :dogs), it seems unambiguous what should be done here.

In the meanwhile, presumably a workaround would be to use extend-protocol, etc to attach the protocol to the record outside the record definition.

Comment by Gary Fredericks [ 09/Dec/13 8:24 AM ]

Alex I can't see a lack of ambiguity here. I read your comment as saying that because dogs is in the call position we can deduce that it's meant to refer to the function rather than the field. But we can't assume that a field is not an IFn or intended to be used as one, so I can't make sense of that.

Another workaround should be using a fully qualified reference to the protocol function.

My expectation as a user of defrecord and deftype has always been that ambiguous references always refer to the fields, as if there were a let around each function body. So I've done this kind of thing intentionally I think. I don't know what that means about whether or not it should be a warning.

Should warnings for this kind of thing be accompanied by a way to suppress individual instances of the warning? E.g., a ^:no-warn metadata somewhere?

Comment by Stuart Halloway [ 31/Jan/14 2:41 PM ]

It is correct and legal code to have a record or type field shadow a var name, and as Gary mentions, the var is still reachable via a qualified name.

This might be a good warning for a linter like https://github.com/jonase/eastwood, if it is not present already.

Comment by Andy Fingerhut [ 31/Jan/14 2:55 PM ]

The Eastwood linter currently has no warning for this situation, but I have created an issue on its Github page to record the enhancement idea: https://github.com/jonase/eastwood/issues/55

Comment by Benjamin Peter [ 31/Jan/14 3:26 PM ]

Okay, thanks guys.





[CLJ-1248] Show type information in reflection warning messages when available Created: 24/Aug/13  Updated: 14/Feb/14  Resolved: 14/Feb/14

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

Type: Enhancement Priority: Major
Reporter: Christoffer Sawicki Assignee: Unassigned
Resolution: Completed Votes: 12
Labels: errormsgs

Attachments: Text File clj-1248-2.patch     Text File Include-type-information-in-reflection-warning-messa.patch    
Patch: Code and Test
Approval: Ok

 Description   

The reflection warning messages currently don't show any type information. I think adding this would make the messages more helpful by making it more obvious what the problem is. I suggest these changes:

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

(defn foo [^String x] (.blah x))
Before: reference to field blah can't be resolved.
After:  reference to field blah on java.lang.String can't be resolved.

(defn foo [^String x] (.zap x 1))
Before: call to zap can't be resolved.
After:  call to method zap on java.lang.String can't be resolved (no such method).

(defn foo [] (Integer/valueOf #"boom"))
Before: call to valueOf can't be resolved.
After:  call to static method valueOf on java.lang.Integer can't be resolved (argument types: java.util.regex.Pattern).

(defn foo [x] (Integer/valueOf x))
Before: call to valueOf can't be resolved.
After:  call to static method valueOf on java.lang.Integer can't be resolved (argument types: unknown).

Patch: clj-1248-2.patch



 Comments   
Comment by Alex Miller [ 25/Aug/13 8:31 AM ]

I think these are a good idea. I think it would be better to separate the reflected class from the field though since we are referring to fields that don't exist.

For example:
1) field: "reference to field blah in java.lang.String can't be resolved."
2) method: "call to method zap in java.lang.String can't be resolved."
3) static method: "call to method valueOf in java.lang.Integer can't be resolved."

Your 3rd example actually highlights something more interesting though. In this case the problem is not actually that Integer/valueOf does not exist but rather that it is being called with the wrong types. In these cases, what I want to know is: what is the type of the parameters being passed.

In #2 there are actually two possible sources of error - an unexpected type for x or an unexpected type or arity for the parameters. It would be useful to check whether the method of this name exists and at what arity to determine which of these cases exists and give a more precise error.

In any case, the implementation needs to be supplied as a patch, not a link. See: http://dev.clojure.org/display/community/Developing+Patches

Comment by Christoffer Sawicki [ 25/Aug/13 3:25 PM ]

+1 on all points. I'll begin work on an updated patch.

Yes, there is a deeper more interesting problem lurking here.

One question is what should result in reflection warnings and what in compile-time errors. (I think some types of reflection warnings should be promoted to errors.)

Here are some examples of current behavior with comments:

(fn [] (Integer/valueOff :foo))
CompilerException java.lang.IllegalArgumentException: No matching method: valueOff, compiling:(NO_SOURCE_PATH:1:8) 

^ Error because the compiler can statically see this is never going to work. Fine.

(fn [] (Integer/valueOf :foo))
Reflection warning, NO_SOURCE_PATH:1:8 - call to valueOf can't be resolved.

^ This is never going to work either, but only gives a warning. A bit surprising; I'd prefer an error.

(fn [x] (.foo x))
Reflection warning, NO_SOURCE_PATH:1:9 - reference to field foo can't be resolved.

^ This could work. Fine.

(fn [^String x] (.foo x))
Reflection warning, NO_SOURCE_PATH:1:17 - reference to field foo can't be resolved.

^ This is never going to work if x is a String but x can be of any type at run-time. Personally, I think this should be an error...

Comment by Alex Miller [ 25/Aug/13 4:36 PM ]

You should take any warning/error differences to one (or more) new tickets where they can be evaluated individually. The more you put in one ticket, the more likely it is to get bogged down and/or rejected. My gut feeling is that there would not be a lot of support for the warning->error changes you suggest.

Comment by Christoffer Sawicki [ 28/Aug/13 3:05 PM ]

Here's an updated patch that changes the messages like this:

(defn foo [^String x] (.blah x))
Before: reference to field blah can't be resolved.
After:  reference to field blah of java.lang.String can't be resolved.

(defn foo [^String x] (.zap x 1))
Before: call to zap can't be resolved.
After:  call to method zap of java.lang.String can't be resolved (no such method).

(defn foo [] (Integer/valueOf #"boom"))
Before: call to valueOf can't be resolved.
After:  call to static method valueOf of java.lang.Integer can't be resolved (argument types: [java.util.regex.Pattern]).

(defn foo [x] (Integer/valueOf x))
Before: call to valueOf can't be resolved.
After:  call to static method valueOf of java.lang.Integer can't be resolved (argument types: [unknown]).

(I wish I could edit the issue description.)

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

Updated description per latest patch.

Comment by Alex Miller [ 12/Feb/14 1:12 AM ]

I used this in a local build to resolve an issue tonight and found it very helpful. One comment I have is that in the message part "(argument types: [java.util.regex.Pattern])", I would like to remove the outer [ ] around the argument types. In the case I was working on the first type was actually a long[] which has class name "[J" so I found the outer []s distracting.

Comment by Christoffer Sawicki [ 12/Feb/14 5:41 AM ]

Thanks for the success story!

I think I choose to use a vector to disambiguate the case with one argument that is unknown, i.e. "(argument types: [unknown])". Without the vector, it's not obvious if there's one argument of unknown type or if all of multiple arguments are of unknown type.

Given your input and some more thought, it's probably not worth making every other message worse for this single case. I'll update the patch tonight.

Comment by Alex Miller [ 12/Feb/14 9:49 AM ]

I went ahead and updated the patch. I also fixed a couple whitespace issues and changed the word "of" to "on" before the type as I think it reads better.

Comment by Christoffer Sawicki [ 12/Feb/14 1:57 PM ]

OK, thanks!





[CLJ-1247] Document the availability/usage of *e, *1, *2, ... in REPL Created: 24/Aug/13  Updated: 29/Aug/13  Resolved: 24/Aug/13

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

Type: Enhancement Priority: Major
Reporter: Jakub Holy Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: documentation, repl


 Description   

For new users of Clojure, it is very hard to find out that evaluation results in any Clojure REPL are bound to *1 - *3 and the latest exception to *e. Since it is a pretty useful feature, it should be documented at a visible place. Where that place is, I am not sure.

One possibility would be to add it to the docstring of clojure.main/repl and make http://clojure.org/repl_and_main link to it (i.e. in the "Launching a REPL" section, we could add something like "Read the <link to=somewhere>docstring of clojure.main/repl</link> to learn about options and available vars. See also utility functions/macros in the clojure.repl namespace."

Note: This was originally reported under nREPL in NREPL-43.



 Comments   
Comment by Alex Miller [ 24/Aug/13 2:52 PM ]

I updated the http://clojure.org/repl_and_main page to include much of this info.

Comment by Jakub Holy [ 29/Aug/13 3:16 AM ]

Lovely, thanks!





[CLJ-1246] type-reflect with AsmReflector throws exceptions for classes with annotations Created: 21/Aug/13  Updated: 25/Oct/13  Resolved: 25/Oct/13

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

Type: Defect Priority: Major
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: interop

Attachments: File clj-1246-fix-type-reflect-exception-patch-v1.diff     Text File clj-1246-fix-type-reflect-exception-patch-v1.txt    
Patch: Code and Test
Approval: Ok

 Description   

REPL session reproducing the problem:

% java -cp clojure.jar clojure.main
Clojure 1.6.0-master-SNAPSHOT
user=> (use 'clojure.reflect)
nil
user=> (import '[clojure.reflect AsmReflector])
clojure.reflect.AsmReflector
user=> (def cl (.getContextClassLoader (Thread/currentThread)))
#'user/cl
user=> (def asm-reflector (AsmReflector. cl))
#'user/asm-reflector
user=> (type-reflect 'java.lang.SuppressWarnings :reflector asm-reflector)
AbstractMethodError clojure.reflect.AsmReflector$reify__9181.visitAnnotation(Ljava/lang/String;Z)Lclojure/asm/AnnotationVisitor;  clojure.asm.ClassReader.accept (ClassReader.java:593)

Issue discovered when trying out a build of Clojure source code with JDK8 early access version b103. In Clojure's set of tests, one of the classes tested with type-reflect fails with JDK8 because annotations were added to that class in JDK8, but it did not have annotations in earlier JDK versions. The same issue exists with JDK6 and JDK7 for any class with annotations, though – it is not unique to JDK8.

Analysis:

Definition of AsmReflector type in src/clj/clojure/reflect/java.clj has a reify for a ClassVisitor with no definition for a visitAnnotation method. This method is called for classes with annotations.

Patch: clj-1246-fix-type-reflect-exception-patch-v1.diff
Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 21/Aug/13 8:04 PM ]

Patch clj-1246-fix-type-reflect-exception-patch-v1.txt dated Aug 21 2013 eliminates the exception by implementing the visitAnnotation method in the ClassVisitor object of the AsmReflector implementation. The implementation simply returns nil, which is enough for the caller to keep going through the class definition, ignoring any annotations.

Comment by Alex Miller [ 22/Oct/13 9:25 PM ]

Is there any reason for the addition of SuppressWarnings in the test file?

Comment by Andy Fingerhut [ 22/Oct/13 9:52 PM ]

I added that because that new test fails on JDK6 and 7 without the patch. Without the additional test, the bug is not exposed by any existing tests unless you run on JDK8.

Comment by Alex Miller [ 23/Oct/13 10:56 PM ]

What new test? I just see the SuppressWarnings import?

Comment by Andy Fingerhut [ 24/Oct/13 9:05 AM ]

The line where java.lang.SuppressWarnings is added is not an import, but naming another class in a sequence of classes being iterated over via doseq in a test.

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

Ah, thank you. Diff blindness.





[CLJ-1245] Disable GitHub "Issues" feature to avoid non-CA/non-JIRA issue submissions Created: 14/Aug/13  Updated: 17/Aug/13  Resolved: 15/Aug/13

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

Type: Task Priority: Major
Reporter: Aaron Brooks Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

I couldn't find any better project to file this against so I'm filing it against the Clojure project proper. I believe this applies to all sub-projects governed by the Clojure CA, hosted on GitHub.

The GitHub issues feature is a magnet for people who would like to help out (in what is a very common and expected manner). Unfortunately, the rejection / redirection process seems to annoy some people unnecessarily and takes time and attention of people who know the actual process to direct them to read/sign the CA and submit via JIRA.

Since GitHub allows repo administrators to disable the issues feature it seems prudent to do so. (e.g. https://github.com/clojure/clojure/settings -> uncheck "Issues"; repeat for other CA projects).



 Comments   
Comment by Aaron Brooks [ 14/Aug/13 8:42 PM ]

I misparsed a pull request as an issue via the GitHub email notifications (they look similar). I see that the Clojure repo does have "Issues" disabled and that there is no way to disable pull requests (that I can see). Please feel free to close this issue without further thought.

Comment by Alex Miller [ 17/Aug/13 7:33 AM ]

BTW, there is an existing ticket out there to add a contributing.md file to specify the policy via GitHub - CLJ-1122.

Comment by Aaron Brooks [ 17/Aug/13 3:03 PM ]

Thanks for pointing that out. Clearly this is a very difficult piece of markdown to deploy...





[CLJ-1244] :prefix is ignored when trying to gen-class with custom methods Created: 14/Aug/13  Updated: 16/Aug/13  Resolved: 16/Aug/13

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

Type: Defect Priority: Major
Reporter: Daniel Kwiecinski Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: AOT, gen-class


 Description   

I'm trying to generate several classes defined in one namespace:

(ns aot.core)

(gen-class
:name aot.core.ClassA
:prefix "classA-")

(gen-class
:name aot.core.ClassB
:prefix "classB-")

(defn classA-toString
[this]
"I'm an A.")

(defn classB-toString
[this]
"I'm a B.")

After AOT I can see that

(.toString (ClassA.))

doesn't produce "I'm an A." string but rather uses method from the superclass >> "aot.core.ClassA@33ba4e15"

If on other hand I do:

(ns aot.core)

(gen-class
:name aot.core.ClassA
:prefix "classA-")

(gen-class
:name aot.core.ClassB
:prefix "classB-")

(defn -toString
[this]
"I'm an A.")

(defn -toString
[this]
"I'm a B.")

then both

(.toString (ClassA.))

and

(.toString (ClassB.))

obviously give "I'm a B." as there is only one -toString really defined.

Is it a bug? Am I doing something wrong? How can I make clojure respect :prefix option.



 Comments   
Comment by Daniel Kwiecinski [ 15/Aug/13 5:42 AM ]

Please close the ticket. I have copied an example from a blog post which had wrong keyword.
Instead of :prefix it had :prefix. Notice that fi in :prefix is single character (see: fi fi )

Comment by Alex Miller [ 16/Aug/13 8:59 AM ]

Closed per request





[CLJ-1238] Allow EdnReader to read foo// (CLJ-873 for EdnReader) Created: 28/Jul/13  Updated: 22/Nov/13  Resolved: 22/Nov/13

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: edn, reader

Attachments: Text File 0001-Fix-CLJ-873-for-EdnReader-too.patch     File clj-1238-2.diff    
Patch: Code
Approval: Ok

 Description   

The patch that has been applied in 1.6 for CLJ-873 predated the introduction of EdnReader, as such it only patched LispReader.

This patch makes the same change to allow foo// in EdnReader, and removes two constants in clojure.lang.RT that are not needed anymore after this patch.

To reproduce:

user=> (require 'clojure.edn)
user=> (defn / [& x] 42)
user=> (clojure.edn/read-string "(user// 4 2)")
RuntimeException Invalid token: user//  clojure.lang.Util.runtimeException (Util.java:219)

Approach: copy changes from LispReader in CLJ-873. After:

user=> (require 'clojure.edn)
nil
user=> (defn / [& x] 42)
WARNING: / already refers to: #'clojure.core// in namespace: user, being replaced by: #'user//
#'user//
user=> (clojure.edn/read-string "(user// 4 2)")
(user// 4 2)

Patch: 0001-Fix-CLJ-873-for-EdnReader-too.patch

Screened by: Alex Miller



 Comments   
Comment by Nicola Mometto [ 29/Jul/13 9:37 AM ]

Alex, this title might be misleading – the main purpose of this patch is to allow "foo//" to be read by clojure.edn/read

Comment by Alex Miller [ 30/Jul/13 3:12 PM ]

@Nicola - Please fix!

Comment by Nicola Mometto [ 30/Jul/13 6:28 PM ]

Done - I also changed the description to better describe what the patch actually does

Comment by Andy Fingerhut [ 25/Oct/13 12:27 PM ]

I've not looked into details yet, but screened 0001-Fix-CLJ-873-for-EdnReader-too.patch fails to apply cleanly as of Oct 25, 2013 latest Clojure master.

Comment by Alex Miller [ 25/Oct/13 2:05 PM ]

Updated patch to apply cleanly to master as of Oct 25, 2013.

Comment by Andy Fingerhut [ 31/Oct/13 4:37 PM ]

With the undoing of the change for CLJ-1252 earlier today, the patch 0001-Fix-CLJ-873-for-EdnReader-too.patch applies cleanly again, and clj-1238-2.diff no longer does.

Comment by Alex Miller [ 31/Oct/13 4:48 PM ]

Andy you are uncanny - I was just on my way to update this.

Comment by Alex Miller [ 31/Oct/13 4:51 PM ]

Switching back to prior patch.





[CLJ-1235] Disappearance of non-evaluated metadata for empty local collection Created: 25/Jul/13  Updated: 25/Jul/13  Resolved: 25/Jul/13

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

Type: Defect Priority: Minor
Reporter: Jozef Wagner Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None


 Description   

It's a contrived example, but it shows the problem:

Clojure 1.6.0-master-SNAPSHOT
user=> (def v (quote ^{x 3} []))
#'user/v
user=> (meta v)
{x 3}
user=> (let [v (quote ^{x 3} [])] (meta v))
nil ;; BUG, it should return {x 3}
user=> (let [v (read-string "^{x 3} []")] (meta v))
{x 3}
user=> (let [v (quote ^{x 3} [1 2 3])] (meta v))
{x 3} ;; Non empty colls behave correctly.


 Comments   
Comment by Nicola Mometto [ 25/Jul/13 5:13 AM ]

duplicate of http://dev.clojure.org/jira/browse/CLJ-1187

I'm closing this





[CLJ-1234] Accept whitespace in Record and Type reader forms Created: 24/Jul/13  Updated: 25/Oct/13  Resolved: 25/Oct/13

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

Type: Defect Priority: Minor
Reporter: Jozef Wagner Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: reader

Attachments: File clj-1234-v1.diff     Text File clj-1234-v1.txt    
Patch: Code and Test
Approval: Ok

 Description   

Whitespace should be allowed in the record reader form as with tagged literals. Following example shows the problem:

user=> *clojure-version*
{:major 1, :minor 5, :incremental 1, :qualifier nil}
user=> (defrecord B [x])
user.B
user=> (B. 2)
#user.B{:x 2}
user=> #user.B{:x 3}
#user.B{:x 3}
user=> #user.B {:x 3}
RuntimeException Unreadable constructor form starting with "#user.B "  clojure.lang.Util.runtimeException (Util.java:219)
user=> #inst "2013"
#inst "2013-01-01T00:00:00.000-00:00"
user=> #inst"2013"
#inst "2013-01-01T00:00:00.000-00:00"

Patch: clj-1234-v1.diff

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 25/Jul/13 2:40 PM ]

Stack trace in case it's useful:

user=> (pst *e)
ReaderException java.lang.RuntimeException: Unreadable constructor form starting with "#user.B "
clojure.lang.LispReader.read (LispReader.java:220)
clojure.core/read (core.clj:3407)
clojure.core/read (core.clj:3405)
clojure.tools.nrepl.middleware.interruptible-eval/evaluate/fn-589/fn-592 (interruptible_eval.clj:52)
clojure.main/repl/read-eval-print-6588/fn-6589 (main.clj:257)
clojure.main/repl/read-eval-print--6588 (main.clj:257)
clojure.main/repl/fn--6597 (main.clj:277)
clojure.main/repl (main.clj:277)
clojure.tools.nrepl.middleware.interruptible-eval/evaluate/fn--589 (interruptible_eval.clj:56)
clojure.core/apply (core.clj:617)
clojure.core/with-bindings* (core.clj:1788)
clojure.tools.nrepl.middleware.interruptible-eval/evaluate (interruptible_eval.clj:41)
Caused by:
RuntimeException Unreadable constructor form starting with "#user.B "
clojure.lang.Util.runtimeException (Util.java:219)
clojure.lang.LispReader$CtorReader.readRecord (LispReader.java:1224)
clojure.lang.LispReader$CtorReader.invoke (LispReader.java:1174)
clojure.lang.LispReader$DispatchReader.invoke (LispReader.java:619)
clojure.lang.LispReader.read (LispReader.java:185)
clojure.core/read (core.clj:3407)

Comment by Andy Fingerhut [ 08/Sep/13 3:40 AM ]

Patch clj-1234-v1.txt adds a test that fails without the patch, and passes with it, verifying that white space is allowed after #record.name but before {.

Very straightforward patch, especially since fogus was nice enough to write 2 commented-out lines that did the trick simply by uncommenting them.

Comment by Alex Miller [ 08/Sep/13 3:14 PM ]

Any LispReader fix likely also affects EdnReader.

Comment by Nicola Mometto [ 08/Sep/13 4:57 PM ]

EdnReader doesn't read record/type literals so in this case it's not affected

Comment by Alex Miller [ 09/Sep/13 1:26 AM ]

Thanks for checking!





[CLJ-1233] Allow ** as a valid symbol name without triggering "not declared dynamic" warnings Created: 23/Jul/13  Updated: 25/Oct/13  Resolved: 25/Oct/13

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

Type: Defect Priority: Trivial
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: Compiler
Environment:

All


Attachments: File clj-1233-minimal.diff     File clj-1233-with-test.diff     File clj-1233-with-test-v2.diff     Text File clj-1233-with-test-v2.txt    
Patch: Code and Test
Approval: Ok

 Description   

Currently declaring a symbol ** triggers a compiler warning:

user=> (defn ** [] nil)
Warning: ** not declared dynamic and thus is not dynamically rebindable, but its name suggests otherwise. Please either indicate ^:dynamic ** or change the name. (NO_SOURCE_PATH:1)
#'user/**

** is a useful symbol in many domains, e.g. as an exponent function or as a matrix multiplication operator.

Cause: This warning checks for a def of a symbol that starts and ends with *.

Solution: Change check for name length >2 to skip this particular case.

Patch: clj-1233-with-test-v2.diff

Screened by: Alex Miller



 Comments   
Comment by Mike Anderson [ 23/Jul/13 6:09 AM ]

Link to discussion on Clojure-Dev
https://groups.google.com/forum/#!topic/clojure-dev/OuTMsZQkxN4

Comment by Mike Anderson [ 23/Jul/13 12:29 PM ]

Minimal patch for the ** case only

Comment by Alex Miller [ 24/Jul/13 12:17 PM ]

Minimal patch would be slightly less minimal with a test.

Comment by Mike Anderson [ 25/Jul/13 5:16 AM ]

Hmmm... is there a standard/reliable method for testing the presence / non-presence of emitted warnings?

Comment by Alex Miller [ 25/Jul/13 3:21 PM ]

There are some test helpers in Clojure's test/clojure/test_helper.clj for capturing error messages.

Comment by Mike Anderson [ 29/Jul/13 12:32 PM ]

New patch with a test that includes using "with-err-print-writer" to detect the avoidance of the warning.

Comment by Andy Fingerhut [ 05/Sep/13 6:24 PM ]

Patch clj-1233-with-test-v2.txt is identical to Mike Anderson's clj-1233-with-test.diff (preserving his authorship), except it avoids git warnings when applying by eliminating trailing whitespace in added lines.





[CLJ-1228] Fix typos in 4 namespaces and 2 docs Created: 01/Jul/13  Updated: 25/Oct/13  Resolved: 25/Oct/13

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

Type: Defect Priority: Major
Reporter: Gabriel Horner Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docstring

Attachments: Text File clj-1228-fix-multiple-typos-2.patch     Text File clj-1228-fix-multiple-typos.patch    
Patch: Code
Approval: Ok

 Description   

I used a spell checker I wrote for clojure projects, lein-spell, and found a few typos. No code changes.

Patch: clj-1228-fix-multiple-typos-2.patch

Screened by: Alex Miller



 Comments   
Comment by OHTA Shogo [ 01/Jul/13 8:07 PM ]

s/unnecesary/unnecessary/

Comment by Gabriel Horner [ 02/Jul/13 6:40 AM ]

@OHTA Shogo - Ha. Good catch.

Uploaded clj-1228-fix-multiple-typos-2.patch with that fix





[CLJ-1227] Definline functions do not work as higher-order functions when AOT compiled Created: 27/Jun/13  Updated: 22/Jan/14  Resolved: 21/Jan/14

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

Type: Defect Priority: Blocker
Reporter: Colin Fleming Assignee: Unassigned
Resolution: Declined Votes: 1
Labels: aot
Environment:

OSX 10.8, Linux


Attachments: Text File 0001-CLJ-1227-Fix-definline-in-AOT-scenarios.patch     Zip Archive aot-test.zip    
Patch: Code and Test

 Description   

See discussion on Clojure group: https://groups.google.com/d/topic/clojure/v0ipoiP8X1o/discussion

Functions defined with definline appear to misbehave when AOT compiled and used with higher-order functions - it seems like the macro function is stored instead of the expansion. I've attached a small test project here reproducing the issue. It can be run with:

lein compile
lein uberjar
java -jar target/aot-test-0.1.0-SNAPSHOT-standalone.jar

The relevant part of the test namespace is:

(definline java-list? [element]
  `(instance? List ~element))

(defn -main [& args]
  (println (java-list? 2))
  (println ((var-get #'java-list?) 2))
  (println (filter java-list? [1 2 3]))
  (println (map java-list? [1 2 3])))

The output produced is:

false
(clojure.core/instance? java.util.List 2)
(1 2 3)
((clojure.core/instance? java.util.List 1) (clojure.core/instance? java.util.List 2) (clojure.core/instance? java.util.List 3))


 Comments   
Comment by Tassilo Horn [ 14/Jan/14 9:15 AM ]

I've just fallen into the very same trap. This is definitively a blocker as it means that AOT-compiled code will probably not work correctly if there's a definline in any dependency (transitively).

Comment by Tassilo Horn [ 14/Jan/14 9:57 AM ]

BTW, the problem also applies to the definlines defined in clojure.core such as ints, longs, doubles, etc.

;; This NS is AOT-compiled
(ns aot-test.core
  (:gen-class))

(defn -main [& args]
  (let [ary (make-array Integer/TYPE 5)]
    (println (apply ints [ary]))))

The output is:

% lein uberjar && java -jar target/aot-test-0.1.0-SNAPSHOT-standalone.jar
Compiling aot-test.core
Compiling aot-test.core
Created /home/horn/tmp/aot-test/target/aot-test-0.1.0-SNAPSHOT.jar
Created /home/horn/tmp/aot-test/target/aot-test-0.1.0-SNAPSHOT-standalone.jar
(. clojure.lang.Numbers clojure.core/ints #<int[] [I@39b65439>)
Comment by Tassilo Horn [ 15/Jan/14 3:48 AM ]

I debugged a bit further. What made me wonder is why standard clojure functions with :inline metadata work correctly in AOT-scenarios whereas definlines which expand to normal functions with :inline metadata do not.

The bug can be fixed by adding the :inline metadata immediately in the defn form in the expansion instead of providing it after the defn form using alter-meta!. Here's a demo example:

(ns aot-test.core
  (:gen-class))

;; (definline n? [x]
;;   `(clojure.lang.Util/identical ~x nil))
;;
;; It expands into the following which doesn't work in AOT-scenarios, e.g.,
;; (apply n? [nil]) returns (clojure.lang.Util/identical nil nil) rather than
;; true.
(do
  (clojure.core/defn n? [x] (clojure.lang.Util/identical x nil))
  (clojure.core/alter-meta!
    #'n?
    clojure.core/assoc
    :inline
    (clojure.core/fn n? [x]
      (clojure.core/seq
        (clojure.core/concat
          (clojure.core/list 'clojure.lang.Util/identical)
          (clojure.core/list x)
          (clojure.core/list 'nil)))))
  #'n?)

;; If the expansion would look like this, i.e., the metadata is added directly
;; instead of with alter-meta!, then it works fine in AOT-scenarios.
(do
  (clojure.core/defn nl?
    {:inline (clojure.core/fn fn? [x]
                                 (clojure.core/seq
                                  (clojure.core/concat
                                   (clojure.core/list 'clojure.lang.Util/identical)
                                   (clojure.core/list x)
                                   (clojure.core/list 'nil))))}
    [x] (clojure.lang.Util/identical x nil)))

(defn -main [& args]
  (println "n?")
  (println (meta #'n?))       ;=> {:inline #<core$n_QMARK_ aot_test.core$n_QMARK_@78ffb648>, :ns #<Namespace aot-test.core>, :name n?, :arglists ([x]), :column 3, :line 8, :file aot_test/core.clj}
  (println (apply n? [nil]))  ;=> (clojure.lang.Util/identical nil nil)  BROKEN!!!
  (println (apply n? [1]))    ;=> (clojure.lang.Util/identical 1 nil)    BROKEN!!!
  (println (n? nil))          ;=> true
  (println (n? 1))            ;=> false

  (println "nl?")
  (println (meta #'nl?))      ;=> {:ns #<Namespace aot-test.core>, :name nl?, :file aot_test/core.clj, :column 3, :line 22, :arglists ([x]), :inline #<core$fn_QMARK_ aot_test.core$fn_QMARK_@3b7541a>}
  (println (apply nl? [nil])) ;=> true
  (println (apply nl? [1]))   ;=> false
  (println (nl? nil))         ;=> true
  (println (nl? 1)))          ;=> false
Comment by Tassilo Horn [ 15/Jan/14 4:56 AM ]

Here's a patch fixing the issue by making definline expand only to a defn form where the :inline metadata is added immediately instead of providing it afterwards with alter-meta!.

I also added a deftest in test_clojure/macros.clj which checks for the correctness of the expansions.

Although this patch fixes the problem, it should be somehow/somewhere documented why the former approach didn't work in AOT-scenarios.

Comment by Alex Miller [ 21/Jan/14 9:36 AM ]

definline should be considered to be like defmacro in that it is not a function and cannot be used as a HOF. Additionally, definline is considered to be an experimental feature and Rich would like to discourage its use as the hope is to remove it in the future. The desired replacement is something like common lisp compiler macros that could allow the compiler to detect special situations and optimize the result but leave behind a function invocation for the case where no special behavior is available.

I am declining the ticket based on the info above and per Rich's request.

Comment by Colin Fleming [ 21/Jan/14 2:20 PM ]

This is a little disappointing since there's really no alternative right now, and I'm assuming that this sort of advanced optimisation is a ways off. If this is the plan, I'd definitely recommend marking definline as deprecated and making clear in the docstring that it shouldn't be relied on to return a function. Currently it states: "like defmacro, except defines a named function...", and based on this ticket that is clearly people's expectation.

Comment by Alex Miller [ 21/Jan/14 4:20 PM ]

Sorry about that. I will now split some definitional hairs by saying that definline is marked as "experimental" to indicate it may or may not even become a feature whereas "deprecation" indicates that it was a feature which you should no longer use. I think in this case they serve the same functional intent which is to warn you away from relying on it as part of the language.

Comment by Colin Fleming [ 21/Jan/14 5:28 PM ]

Fair enough, although it's been experimental for five major releases now. If we're convinced it's a failed experiment (and if it can't be relied on to create a function, it pretty much is since you might as well use a macro) I'd argue that deprecation is justified and sends a stronger signal that it doesn't actually work as intended. But either way, I'm certainly convinced now

Comment by Tassilo Horn [ 22/Jan/14 2:58 AM ]

Alex, definline's intention is to be considered like defmacro in case of (my-definline x) where it saves one function call, but to be callable as a normal function in higher-order scenarios like (map my-definline [x y z]). Therefore, it expands to a normal defn form with :inline metadata which is a macro that's used by the compiler.

Wether it should be removed in the future is a separate issue. It is there right now, and people use it. The consequence of this bug is that you cannot trust AOT-compiled code, because there might be some dependency that uses definline. Additionally, all clojure.core definlines (booleans, bytes, chars, shorts, floats, ints, doubles, longs) are broken when applied as HOF, because clojure itself is always distributed in AOT-compiled form.

Really, it would be grossly negligent to release Clojure 1.6 knowing of this bug.

I've written a more detailed mail to the clojure-dev list:

https://groups.google.com/d/msg/clojure-dev/UeLNJzp7UiI/UCyVvYLfHWMJ

Comment by Tassilo Horn [ 22/Jan/14 3:35 PM ]

The root cause of this issue is CLJ-1330 as investigated by Nicola Mometto in the thread cited above: http://dev.clojure.org/jira/browse/CLJ-1330
So fixing the latter will fix this one, too.





[CLJ-1222] Multiplication overflow issues around Long/MIN_VALUE Created: 21/Jun/13  Updated: 22/Nov/13  Resolved: 22/Nov/13

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

Type: Defect Priority: Minor
Reporter: Colin Jones Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: math

Attachments: File min_value_multiplication.diff    
Patch: Code and Test
Approval: Ok

 Description   

Long story short (ha!), there are several related issues where overflow checking doesn't work correctly with Long/MIN_VALUE. The cause of this missed check is that in Java, these both pass:

assertTrue(Long.MIN_VALUE * -1 == Long.MIN_VALUE);
assertTrue(Long.MIN_VALUE / -1 == Long.MIN_VALUE);

This causes the following results in Clojure that do not match their siblings where the order of operands is switched:

user=> (* Long/MIN_VALUE -1)
-922337203685477580 ;; should throw
user=> (*' Long/MIN_VALUE -1)
-9223372036854775808 ;; should be positive
user=> (* Long/MIN_VALUE -1N)
-9223372036854775808N ;; should be positive
user=> (* (bigint Long/MIN_VALUE) -1N)
-9223372036854775808N ;; should be positive

Mailing list discussion here: https://groups.google.com/d/msg/clojure-dev/o1QGR1QK70M/A1KykR9OQqwJ

Patch: min_value_multiplication.diff

Approach: Modifies BigInt.multiply() and multiply/multiplyP in Numbers to take into account Long.MIN_VALUE.

After:

user=> (* Long/MIN_VALUE -1)
ArithmeticException integer overflow  clojure.lang.Numbers.throwIntOverflow (Numbers.java:1390)
user=> (*' Long/MIN_VALUE -1)
9223372036854775808N
user=> (* Long/MIN_VALUE -1N)
9223372036854775808N
user=> (* (bigint Long/MIN_VALUE) -1N)
9223372036854775808N

Screened by: Alex Miller (should also include CLJ-1225, CLJ-1253, CLJ-1254)



 Comments   
Comment by Andy Fingerhut [ 21/Jun/13 12:56 PM ]

Is there some proof that Long/MIN_VALUE and -1 is the only pair of values that causes a problem with the way overflow-checking is currently done? It might be, but it would be nice to be sure if a proposed patch relies on that.

Comment by Colin Jones [ 21/Jun/13 1:54 PM ]

I don't have a proof, just a half-hour or so of digging for an alternate case (found none), plus taking a look at what Guava does in checkedMultiply: https://code.google.com/p/guava-libraries/source/browse/guava/src/com/google/common/math/LongMath.java#522

I didn't do the leading zeros optimization they're doing to avoid even the division in the overflow check we already have; that felt like a bigger change, and not one that impacts correctness.

Comment by Andy Fingerhut [ 25/Jun/13 11:06 AM ]

I have created CLJ-1225 for the corresponding issues with / and quot with these arguments. I think that Long/MIN_VALUE and -1 should be the only arguments that cause problems for multiplication overflow detection, too, since it is based upon division, and I give some reasons in CLJ-1225 why I believe these are the only arguments that give the numerically wrong answer for the Java division operator / on longs.

Comment by Rich Hickey [ 22/Nov/13 7:28 AM ]

user=> (*' Long/MIN_VALUE -1)
9223372036854775808N

Is wrong - there should be no coercion, this is an overflowing operation.

Comment by Colin Jones [ 22/Nov/13 8:14 AM ]

Rich, can you clarify? My understanding is that *' is an auto-promoting operation. For instance, in 1.5.1, *' auto-promotes in the positive direction:

user=> (*' Long/MAX_VALUE Long/MAX_VALUE)
85070591730234615847396907784232501249N

and in the negative:

user=> (*' Long/MIN_VALUE -2)
18446744073709551616N

The only outlier is the one addressed in this ticket. Am I missing something? What should the result be here if not the one given by this patch?

Comment by Rich Hickey [ 22/Nov/13 10:14 AM ]

missed the ' earlier





[CLJ-1220] instance? should either verify all operands or throw if more than one passed Created: 19/Jun/13  Updated: 02/Sep/13  Resolved: 02/Sep/13

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

Type: Defect Priority: Minor
Reporter: Irakli Gozalishvili Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None


 Description   

(instance? Number 1) ;; => true
(instance? Number "a") ;; => false
(instance? Number 1 "a") ;; => true

I find behavior of the last expression very surprising, I would
expect it to either desugar to logical "and" over all operands:

(and (instance? Number 1) (instance? Number "a") ...)

Or throw "Wrong number of args (3) passed to instance?" exception.



 Comments   
Comment by Andy Fingerhut [ 19/Jun/13 5:32 PM ]

Irakli, one of the patches for CLJ-1171 addresses this issue by causing (instance? Number 1 "a") to throw a Wrong number of args (3) passed to core$instance-QMARK- ArityException.

Comment by Alex Miller [ 02/Sep/13 7:52 PM ]

Fixed by CLJ-1171. In 1.6 master, now gets:

user=> (instance? Number 1 "a")
ArityException Wrong number of args (3) passed to: core/instance?  clojure.lang.AFn.throwArity (AFn.java:436)




[CLJ-1215] Mention where to position docstring when using pre/postconditions on the Special Forms page Created: 07/Jun/13  Updated: 03/Sep/13  Resolved: 03/Sep/13

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

Type: Enhancement Priority: Minor
Reporter: Jakub Holy Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: documentation


 Description   

The description of pre- and post-conditions doesn't mention where docstring should be when using them. Placing it wrong will lead to the conditions to be ignored.

At http://clojure.org/Special%20Forms--(fn%20name?%20[params*%20]%20condition-map?%20exprs*), change

(fn name? [params* ] condition-map? exprs*)

to

(fn name? [params* ] condition-map? docstring? exprs*)

or at least mention it in the description or use it in the example.

Thank you!



 Comments   
Comment by Alex Miller [ 03/Sep/13 8:59 AM ]

The fn special form does not take a docstring so this modification is not valid. fn takes docstrings via meta.

The defn macro does take a docstring but it is placed prior to the attr-map, not after the condition map. The defn docstring does mention that the docstring will be added to the var metadata for the fn (not on the fn itself).





[CLJ-1214] Compiler runs out of memory on a small snippet of code Created: 31/May/13  Updated: 18/Apr/14  Resolved: 18/Apr/14

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

Type: Defect Priority: Major
Reporter: Praki Prakash Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: compiler
Environment:

Linux 3.2.0-39-generic


Attachments: File fubar.clj    

 Description   

Clojure compiler runs out of memory when loading the attached file. Transcript below.

$ java -cp ~/.m2/repository/org/clojure/clojure/1.5.1/clojure-1.5.1.jar:. clojure.main
Clojure 1.5.1
user=> (load "fubar")
OutOfMemoryError GC overhead limit exceeded  [trace missing]
user=> 

The file contents are:

  (ns fu.bar)

  (defn foo[l] (concat (drop-last l) (repeat (last l))))

  (def ^:const bar (foo [#(print "") #(println ";")]))

  bar

If I remove the metadata on bar, it works. Removal of namespace seems to fix it as well. Pretty strange.

Although I realize this code is not quite kosher, it would be nice to have the compiler deal with it.



 Comments   
Comment by Andy Fingerhut [ 01/Jun/13 7:40 PM ]

If you really do have 'bar' on a line by itself at the end of file fubar.clj, it seems you are asking it to evaluate the value of bar, which by the definitions is an infinite list, and will of course exhaust all available memory if you attempt to evaluate it.

It seems to me the more odd thing is not that it runs out of memory as shown, but that it does not run out of memory when you remove the metadata on bar.

What is the purpose of having 'bar' on a line by itself at the end of the file?

If I try this but remove 'bar' as the last line of the file, loading the file causes no errors regardless of whether there is metadata on bar's definition or not. It is strange that doing (load "fubar") followed by (first fu.bar/bar) seems to go into an infinite loop if the :const is there on bar, but quickly returns the correct answer if the metadata is removed.

Comment by Praki Prakash [ 01/Jun/13 8:40 PM ]

This code snippet is a minimal test case to show that the compiler runs out of memory. What I meant by "it works" was that the compiler doesn't run out of memory and successfully loads the file (or in my real code base, the namespace is compiled).

In my code, I use bar (or whatever the real thing is) as source of sequence of functions. The sole reference to bar is needed to trigger this issue. I believe that bar is not being fully evaluated here and thus no infinite loop. If I try to print it, yes, it will ultimately fail.

Comment by Praki Prakash [ 01/Jun/13 9:04 PM ]

Having thought about this a bit more, it seems to me that when bar is annotated with const, the compiler is trying to evaluate the associated expression which exhausts the heap? I have never looked at the compiler source and thus not sure if this is what is happening. If it is, then it seems like one should be really careful when adding metadata.

That still leaves the other question about the namespace requirement to cause memory exhaustion. I quite distinctly recall having to add the namespace when trying to come up with minimal test case to reproduce the bug.

If you think this is really user error, I would accept it

Comment by Andy Fingerhut [ 02/Jun/13 4:56 AM ]

It is not just any old metadata that causes this issue, only :const metadata. I tried testing with :const replaced with :dynamic and :private, and there was no problem.

This might shed some light on the issue: https://github.com/clojure/clojure/blob/master/changes.md#215-const-defs

It appears that ^:const is causing the compiler to evaluate the value at compile time. The value in your example is unbounded, so that can never complete.

Comment by Kevin Downey [ 17/Apr/14 10:39 PM ]

the problem is for complex objects :const results in the pr-str of the object being embedded in the bytecode, a pr-str of an infinite seq is going to blow the heap. I think the limits on the usage of :const are not well defined and instead of falling back on pr-str it should throw a compilation error for constants that are not jvm primitves or strings

Comment by Alex Miller [ 17/Apr/14 10:46 PM ]

There is support for handling a variety of other useful cases (core Clojure collections for example) as constant objects. And it is useful to allow code that is not constant but evaluates to a constant result (for creating data tables, etc). In the face of that, I'm not sure it's possible to bound this usefully without solving the halting problem. I'm tempted to just put this issue in the category of "don't do that".

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

After sleeping on it, I don't think this is worth working on (if there is anything that could even be done).





[CLJ-1213] consistency with def and Unbound Created: 29/May/13  Updated: 03/Sep/13  Resolved: 03/Sep/13

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

Type: Defect Priority: Minor
Reporter: Trevor Wennblom Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

In this example I'd expect `b` to return `Unbound` for consistency.

Clojure 1.5.1
user=> (def a "MyA")
#'user/a
user=> (def a "MyA2")
#'user/a
user=> (def b "MyB")
#'user/b
user=> (def b) ;; unbound b
#'user/b
user=> (def c) ;; unbound c
#'user/c
user=> a
"MyA2"
user=> b
"MyB"
user=> c
#<Unbound Unbound: #'user/c>


 Comments   
Comment by Alex Miller [ 03/Sep/13 9:27 AM ]

The docstring for def states that the init? function is optional and that in the case where it is omitted, the root binding is unaffected.





[CLJ-1211] Protocol call sites emit many unused fields and bytecodes Created: 25/May/13  Updated: 25/May/13  Resolved: 25/May/13

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

Type: Enhancement Priority: Minor
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None

Attachments: File protocol-sites.diff    
Patch: Code

 Description   

Three unused instance member fields are emitted by emitProto() in the compiler - seems that at some point it was for inline caching of protocol implementations, but it looks like that's all handled by MethodImplCache now.

This patch eliminates it, and brings down protocol heavy classes like the new ManyToManyChannel in core.async from 120+ fields down to 23 fields, mostly Var caches.

There should be a lot less bytecode in such classes now, and less memory overhead.



 Comments   
Comment by Ghadi Shayban [ 25/May/13 10:49 AM ]

Needs work





[CLJ-1205] Update Maven build for Nexus 2.4 Created: 22/Apr/13  Updated: 14/Aug/13  Resolved: 14/Aug/13

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

Type: Task Priority: Major
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-nexus-2.4-releases.patch    
Approval: Ok

 Description   

These additions to the build configuration are necessary to support changes to the Sonatype Nexus server at oss.sonatype.org, which we use to promote our build artifacts into the Maven Central Repository.

See Sonatype's announcement at https://groups.google.com/d/msg/clojure-dev/lBpfII2u6vM/LQvr_rO5UGgJ



 Comments   
Comment by Stuart Halloway [ 19/Jun/13 10:07 AM ]

Am I right in assuming that the only way we will know this works is by trying it in a release?

Comment by Stuart Sierra [ 19/Jun/13 10:51 AM ]

Yes.





[CLJ-1204] hash is inconsistent with = for many BigInteger values Created: 18/Apr/13  Updated: 05/Sep/13  Resolved: 05/Sep/13

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

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

Attachments: Text File clj-1204-make-hash-consistent-with-equal-for-bigintegers-v1.txt    
Patch: Code and Test

 Description   

The function hash is documented to be consistent with =. For many BigInteger values, hash is inconsistent with =. This leads to incorrect behavior for data structures like hash maps that use hash.

user> (apply = [-1 -1N (biginteger -1)])
true
user> (map hash [-1 -1N (biginteger -1)])
(0 0 -1)

;; Incorrect return value with multiple keys = to each other
user> (assoc (hash-map -1N :should-be-replaced) (biginteger -1) :new-val)
{-1N :should-be-replaced, -1 :new-val}

;; array-map gives correct value, since it uses =, not hash
user> (assoc (array-map -1N :should-be-replaced) (biginteger -1) :new-val)
{-1N :new-val}


 Comments   
Comment by Andy Fingerhut [ 18/Apr/13 8:10 PM ]

Patch clj-1204-make-hash-consistent-with-equal-for-bigintegers-v1.txt dated Apr 18 2013 takes the following approach to the issue:

Change the behavior of hasheq so that when given a BigInteger value that could fit into a long, returns the same hash code as that long value.

hasheq continues to return x.hashCode() if the BigInteger value does not fit into a long. This is consistent with the hash value returned by a BigInt value that does not fit into a long.

New tests are included, some of which fail without the change to hasheq, but pass with the change.

Comment by Andy Fingerhut [ 18/Apr/13 8:19 PM ]

Overwrite patch with one that leaves out some unnecessary code.

Comment by Andy Fingerhut [ 25/Apr/13 6:42 PM ]

Changing priority to minor, since I suppose one could work around this issue, if you were diligent about it, by converting all BigIntegers to BigInts before they are ever used in a place where they are hashed.

Comment by Andy Fingerhut [ 05/Sep/13 9:00 AM ]

with CLJ-1036, which is out of scope for Clojure, so this one should be as well.





[CLJ-1203] Fallback to hash-based comparison for non-Comparables in Util.compare() Created: 17/Apr/13  Updated: 29/Apr/13  Resolved: 29/Apr/13

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

Type: Enhancement Priority: Minor
Reporter: Tassilo Horn Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: enhancement, patch

Attachments: Text File 0001-Add-hasheq-based-fallback-to-Util.compare.patch    
Patch: Code

 Description   

I oftentimes use sorted collections, and my comparator functions usually look like that:

(fn [a b]
  (let [x (compare-according-to-my-use-case a b)]
    (if (zero? x)
       (compare a b)
       x)))

That is, I define the sorting order depending on the requirements of my use-case, and if that doesn't suffice to make a distinction, I simply fall back to the standard `compare` function in order to get a stable ordering anyhow. I think that's a widely used idiom, e.g., also used in "Clojure Programming" (for example page 109).

The problem with this approach is that several data structures don't implement Comparable, and thus aren't applicable to `compare` (you'll get a ClassCastException). Examples are maps, records, deftypes, and sequences.

The patch I'm going to attach extends `Util.compare()` with a fallback clause that performs a `hasheq()`-based comparison. This results in a meaningless but at least stable sorting order which suffices for use-cases like the one shown above.



 Comments   
Comment by Andy Fingerhut [ 18/Apr/13 9:01 PM ]

Patch 0001-Add-hasheq-based-fallback-to-Util.compare.patch dated Apr 17 2013 applies cleanly to latest master, but causes several unit tests in data_structures.clj to fail. These are the kinds of things you would expect to fail with the change made in the patch, because the failing tests expect an exception to be thrown when comparing objects that don't implement Comparable, and with this patch's changes they no longer do. If this patch's change is desired, those tests should probably be removed.

Comment by Tassilo Horn [ 19/Apr/13 2:34 AM ]

Thanks Andy, I can't believe I've forgotten to re-run the tests.

Anyway, I'm attaching a new version of the patch that deletes the few sorted-set and sorted-set-by invocations that check that an exception is thrown when creating sorted sets containing non-Comparables.

Comment by Tassilo Horn [ 19/Apr/13 2:35 AM ]

New version of the patch.

Comment by Andy Fingerhut [ 26/Apr/13 2:47 PM ]

Tassilo, you say that one of your use cases is sorted collections. Note that any compare function that returns 0 for two values will cause sorted sets and maps to treat the two compared values as equal, and at most one of them will get into the ordered set/map, the second treated as a duplicate, even though the values are not equal. See https://github.com/jafingerhut/thalia/blob/master/doc/other-topics/comparators.md#comparators-for-sorted-sets-and-maps-are-easy-to-get-wrong for an example (not based on your modified compare, but on a comparator that returns 0 for non-equal items).

With your proposed change to compare, this occurrence would become very dependent upon the hash function used. Probably quite rare, but it would crop up from time to time, and could potentially be very difficult to detect and track down the source.

Comment by Tassilo Horn [ 29/Apr/13 2:29 AM ]

Hi Andy, you are right. It's possible to add an explicit equals()-check in cases the hashcodes are the same, but I think there's nothing you can do in the hashcodes-equal-but-objects-are-not case. That is, there's no way to ensure the rule sgn(compare(x, y)) == -sgn(compare(y, x)), the transitivity rule, and the compare(x, y)==0 ==> sgn(compare(x, z))==sgn(compare(y, z)) for all z rule.

I'm closing that ticket.





[CLJ-1202] protocol fns with dashes may get compiled into property access when used higher order Created: 16/Apr/13  Updated: 14/Aug/13  Resolved: 14/Aug/13

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

Type: Defect Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJ-1202.patch    
Patch: Code and Test
Approval: Ok

 Description   

Problem: Protocol functions with leading hyphens may be incorrectly compiled into field accesses.

Demonstration:

(defprotocol Foo (-foo [x]))

(deftype Bar [] Foo (-foo [_] "foo"))

(map -foo (repeatedly 3 ->Bar))
;; IllegalArgumentException No matching field found: foo for class user.Bar  
;; clojure.lang.Reflector.getInstanceField (Reflector.java:271)

Cause: This was caused by CLJ-872, dash property support.

Solution: Emit forms like (. foo (bar)) instead of (. foo bar), so that names starting with a - don't look like field accesses.

Patch: CLJ-1202.patch



 Comments   
Comment by Alan Malloy [ 18/Apr/13 7:18 PM ]

Attached patch fixes the issue, and adds regression test for it.

Comment by Gabriel Horner [ 10/May/13 3:19 PM ]

Verified patch works

Comment by Stuart Sierra [ 02/Aug/13 2:21 PM ]

Screened and cleaned up description.

Comment by Alex Miller [ 08/Aug/13 1:07 AM ]

More cleansing.





[CLJ-1200] ArraySeq dead code cleanup and ArraySeq_short gap filling Created: 14/Apr/13  Updated: 31/Jan/14  Resolved: 31/Jan/14

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

Type: Enhancement Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 6
Labels: None

Attachments: Text File no-getComponentType--v001.patch     Text File no-getComponentType--v002.patch    
Patch: Code
Approval: Ok

 Description   

The ArraySeq constructor and all methods retain support for primitive ArraySeq but that code has all been shunted into type-specific ArraySeq primitive array variants (ArraySeq_long, etc). The ArraySeq_short variant is currently missing.

Approach: Remove the vestigial primitive array code paths in ArraySeq and fields (ct and oa). This may provide a performance benefit but it has been hard to find a measurable impact.

The patch also fills in the missing ArraySeq_short variant. Before this patch, reduce on an array of primitive short would throw ClassCastException.

Patch: no-getComponentType--v002.patch
Screened by: Stuart Sierra



 Comments   
Comment by Zach Tellman [ 07/Jun/13 5:40 PM ]

As a data point, I was recently profiling a Hadoop job where the code made heavy use of 'partial', which apparently unlike 'comp' and 'juxt', always uses apply [1]. As a result, .getComponentType accounted for 41% of the overall compute time. This might be as much a comment on the implementation of partial as anything else, but it certainly shows that this can have a significant effect on performance.

[1] https://github.com/clojure/clojure/blob/master/src/clj/clojure/core.clj#L2388

Comment by Kevin Downey [ 07/Jun/13 5:46 PM ]

prepRet usage appears to be all about enforcing canonical Boolean values, so I think using Object.class is not the best. Maybe splitting ArraySeq in to ArraySeq and ArraySeqBoolean would be better. ArraySeq would no longer do a prepRet and ArraySeqBoolean would

Comment by Kevin Downey [ 07/Jun/13 7:19 PM ]

ArraySeq actually already has specialized implementations, and interestingly the specialized boolean implementation doesn't call prepRet

Comment by Kevin Downey [ 07/Jun/13 7:35 PM ]

The ArraySeq split I proposed above will fail if you have an array of Objects that happen to be Booleans, it seems like the best bet would be something like preRet that did a instanceof Boolean check without the requirement of passing in a class

Comment by Brandon Bloom [ 15/Jul/13 11:43 AM ]

I traced the surrounding code & iirc, the result was always Object[], so the return value was always Object.class. I'm pretty sure that code wasn't actually doing anything useful here.

Comment by Brandon Bloom [ 17/Jul/13 4:54 PM ]

What does "triaged" mean in this context? Doesn't triage require some sort of decision or classification?

Comment by Andy Fingerhut [ 17/Jul/13 5:19 PM ]

See http://dev.clojure.org/display/community/JIRA+workflow for way more than the answer to your question (especially the flowchart in the "Workflow" section), but basically it means that a screener believes the ticket description represents an actual problem to be addressed, and they ask that Rich examine the ticket to see if he agrees.

Comment by Brandon Bloom [ 17/Jul/13 5:22 PM ]

Gotcha. Thanks.

Comment by Alex Miller [ 07/Oct/13 10:25 PM ]

The existing ArraySeq class has support for different two modes - as an array of Objects and as an array of primitives. In the Object case, this.oa will be set to the array and this.ct will be set to the component type of the array. From running a bunch of code, this.ct is not always Object - it is easy to find cases of Class and many other cases as well.

However, any kind of non-primitive array will cause this.oa to be set and this prevents the calls to prepRet from being called with ct in every method where this is done. All primitive arrays are now being handled via the switch in ArraySeq.createFromObject.

The only thing prepRet is effectively doing is returning canonical Boolean objects. It is possible to create an ArraySeq on a Boolean[]. However, even in this case, Boolean[] is an instanceof Object[] and the object path is triggered.

Thus, I agree that prepRet is never being called from ArraySeq and this path can be removed, which also allows us to remove this.ct and importantly the array.getClass().getComponentType() check. This also allows us to go further though and remove the oa field entirely and all of the prepRet calls.

I have attached an updated patch that makes this change. I also found (based on some test failures) that the ArraySeq_short was inexplicably missing so I added that in as well.

ArraySeq could be made even cleaner with the addition of another variant that specifically handled the case of a null array (ArraySeq_null). I have no idea if that would be worth doing and I have not done that here.

Comment by Brandon Bloom [ 08/Oct/13 9:43 AM ]

Hey Alex: Thanks for following through on the vestiges removal.

Comment by Rich Hickey [ 22/Nov/13 1:44 PM ]

please add perf tests that demonstrate benefit

Comment by Alex Miller [ 17/Dec/13 11:40 AM ]

Removed performance angle and focus on clean-up and gap-filling (ArraySeq_short) aspects instead.

Comment by Alex Miller [ 11/Jan/14 8:54 AM ]

Point to considered patch, this got lost at some point in the description.





[CLJ-1197] Allow fold to parallelize over lazy sequences Created: 10/Apr/13  Updated: 27/Sep/13  Resolved: 27/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Paul Butcher Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: reducers

Attachments: File foldable-seq.diff    
Patch: Code

 Description   

This patch implements foldable-seq, which allows fold to parallelize over a lazy sequence. See this conversation on the Clojure mailing list:

https://groups.google.com/forum/#!msg/clojure/8RKCjF00ukQ/b5mmmOB5Uh4J

The patch is code only, sadly. No tests because I've not been able to find any existing tests for fold:

https://groups.google.com/d/msg/clojure-dev/plQ16L1_FC0/CIyMVIgSZkkJ

However, I have tested it in a separate project successfully.



 Comments   
Comment by Stuart Halloway [ 27/Sep/13 3:12 PM ]

Hi Paul,

Seqs are fundamentally not foldable. That said, what you seem to be trying to do (partition a seq into foldable subjobs) is straightforward in Clojure, see

https://github.com/stuarthalloway/exploring-clojure/blob/master/examples/exploring/reducing_apple_pie.clj?#L62-73

That said, if the input is truly arriving sequentially, pmap or some variant may do as well or better, e.g.

https://github.com/stuarthalloway/exploring-clojure/blob/master/examples/exploring/reducing_apple_pie.clj?#L77-83





[CLJ-1196] eval of defrecord instance with no fields produces {}, not itself Created: 10/Apr/13  Updated: 10/Apr/13  Resolved: 10/Apr/13

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

Type: Defect Priority: Minor
Reporter: Jason Wolfe Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None
Environment:

Mac OS X Clojure 1.5.1



 Description   

user> (defrecord Foo [])
user.Foo
user> (Foo.)
#user.Foo{}
user> (eval (Foo.))
{}

user> (defrecord Foo [a b])
user.Foo
user> (eval (Foo. 1 2))
#user.Foo{:a 1, :b 2}

As you can see, evaluating the record with no fields loses the type information. This came up in code that uses records to describe argument schema information for functions in a macro, with no direct use of 'eval'. It is unexpected because of the inconsistency between empty and non-empty defrecords, and because the Clojure docs say 'Any object other than those discussed above will evaluate to itself.'

http://clojure.org/evaluation

I'm happy to elaborate on the use case if there are questions.



 Comments   
Comment by Nicola Mometto [ 10/Apr/13 5:11 AM ]

I think this is a duplicate of http://dev.clojure.org/jira/browse/CLJ-1093

Comment by Jason Wolfe [ 10/Apr/13 10:51 AM ]

You are right, sorry for the duplication. I searched for empty record in JIRA and somehow missed that ticket.

Comment by Andy Fingerhut [ 10/Apr/13 11:42 AM ]

Close as duplicate of CLJ-1093.





[CLJ-1194] Typo in core.reducers Created: 08/Apr/13  Updated: 09/Apr/13  Resolved: 09/Apr/13

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

Type: Defect Priority: Minor
Reporter: Dmitry Groshev Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

As far as I can say, there is a typo in core.reducers (and therefore core.reducers/monoid is unusable in 1.5.1):

https://github.com/clojure/clojure/blob/master/src/clj/clojure/core/reducers.clj#L321

(fn m <-- this
    ([] (ctor))
    ([a b] (op a b))))

Should I submit a patch to fix this?



 Comments   
Comment by Ghadi Shayban [ 08/Apr/13 3:09 PM ]

I guess you didn't try to actually use the function to see if it is actually broken...

m is not a argument vector, those are below in the function clauses.

Comment by Dmitry Groshev [ 08/Apr/13 3:26 PM ]

I did, but as it turns out the error was somewhere else and I can't reproduce it within a "clean" environment. Sorry for this hasty ticket.

Comment by Andy Fingerhut [ 08/Apr/13 11:31 PM ]

Dmitry, do you think it is OK to close this ticket as declined, meaning no change will be made in response to it? I can do that for you, if you wish.

Comment by Dmitry Groshev [ 09/Apr/13 4:49 AM ]

Andy, I'll close it myself if you don't mind. I would done it earlier, but didn't have sufficient rights in JIRA.





[CLJ-1193] bigint, biginteger throw on double values outside of long range Created: 07/Apr/13  Updated: 14/Aug/13  Resolved: 14/Aug/13

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

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

Attachments: Text File clj-1197-make-bigint-work-on-all-doubles-v1.txt    
Patch: Code and Test
Approval: Ok

 Description   

The bigint and biginteger functions throw on double values outside of long range

This works fine:

user> (bigint (* 0.99 Long/MAX_VALUE))
9131138316486227968N

but passing any Float or Double values outside the range of a long throw an exception:

user> (bigint (* 1.01 Long/MAX_VALUE))
IllegalArgumentException Value out of range for long: 9.315605757223324E18  clojure.lang.RT.longCast (RT.java:1178)

Cause: bigint and biginteger cover a series of possible input cases but did not have an explicit case for Float or Double, so was falling back to default.

Solution: Add check for Float/Double case that coerces the input to double, then uses BigDecimal.valueOf(), then converts to a BigInteger and so on as with other types.

Patch: clj-1197-make-bigint-work-on-all-doubles-v1.txt



 Comments   
Comment by Andy Fingerhut [ 07/Apr/13 4:38 PM ]

Patch clj-1197-make-bigint-work-on-all-doubles-v1.txt dated Apr 7 2013 changes bigint and biginteger so that they return the correct value for all float and double values, and no longer throw an exception.

Comment by Gabriel Horner [ 17/May/13 10:52 AM ]

Looks good. Tests pass and the failing example now converts correctly

Comment by Alex Miller [ 08/Aug/13 1:51 AM ]

Cleaned up the description a bit.





[CLJ-1190] Javadoc for public Java API Created: 03/Apr/13  Updated: 11/Jan/14  Resolved: 11/Jan/14

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

Type: Enhancement Priority: Critical
Reporter: Stuart Halloway Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: documentation

Attachments: File clj-1190-2.diff     File clj-1190-3.diff     Text File clj-1190.patch    
Patch: Code
Approval: Ok

 Description   

Publish javadoc for http://dev.clojure.org/jira/browse/CLJ-1188.

  • Add javadoc for public API classes (Clojure and IFn)
  • Add ant build target to generate the javadoc
  • Publish javadoc to the clojure project github pages (will be done manually)

Patch: clj-1190-3.diff (javadoc updates and a new Ant target)

Screened by: Stuart Halloway

Updated by: Alex Miller (moved clojure.api.API to clojure.java.api.Clojure and tweaked one example per Rich's suggestion)



 Comments   
Comment by Andy Fingerhut [ 18/Aug/13 3:57 AM ]

It seems that when this ticket is completed, it may also complete CLJ-19, too.

Comment by Alex Miller [ 04/Oct/13 1:46 PM ]

Maven javadoc is something like this (configuration as in http://maven.apache.org/plugins/maven-javadoc-plugin/javadoc-mojo.html):

<plugin>
        <groupId>org.apache.maven.plugins</groupId>
        <artifactId>maven-javadoc-plugin</artifactId>
        <version>2.9.1</version>
        <configuration>
          <show>public</show>
          <verbose>true</verbose>
          <sourceFileIncludes>
            <include>API.java</include>
          </sourceFileIncludes>
          <sourcepath>${basedir}/src/jvm/clojure/api</sourcepath>
        </configuration>
      </plugin>

What is desired for inclusion in the javadoc? Just the clojure.api package and API class? If so is it worth automating the publishing of this API or just documenting a process to put someplace static?

Comment by Alex Miller [ 21/Oct/13 10:14 PM ]

Javadoc can be told what to doc either by package or by java source file. The Maven plugin only supports the source file version if the files are in the same directory (as far as I can tell).

Ant was a little easier to get going:

<target name="javadoc" depends="build"
         description="Creates javadoc for Clojure API.">
    <javadoc destdir="javadoc">
      <classpath>
        <path location="${build}"/>
      </classpath>
      <fileset dir="src/jvm">
        <include name="clojure/api/API.java"/>
        <include name="clojure/lang/IFn.java"/>
      </fileset>
    </javadoc>
  </target>

However, there is no way to omit the nested IFn primitive interfaces in either case. That's just not a thing. Presumably it would be possible to customize the standard doclet to omit whatever you wanted although it makes me throw up in my mouth a little to even suggest that as a necessity.

Comment by Stuart Halloway [ 22/Oct/13 9:12 AM ]

Including nonpublic interfaces is worse than having no Javadoc at all, IMO. We plan to change this API almost never, so one possibility it to just generate from a copy of the IFn source file that omits the primitives.

API should have a class level document.

The goal here is to publish the official Javadoc on the web. Following the javadoc jar convention is a potential separate goal, but out of scope here.

Comment by Alex Miller [ 22/Oct/13 9:36 AM ]

When I next get a chance I'll take a look at what customizing the standard doclet would take. If it looks ugly I'll bail out.

Comment by Alex Miller [ 22/Oct/13 9:43 PM ]

Modifying the standard doclet (a mystery shrouded in a thicket of Oracle docs) to generate javadoc for 2 classes is ridiculous. As Stu suggested, I just hacked the inner interfaces out of IFn and generated the attached javadoc with the ant target prior in the comments. I think we could probably turn off a few more of the bits and improve the title,etc then manually publish this set of files in the github pages somewhere.

Comment by Alex Miller [ 23/Oct/13 8:48 PM ]

Added an Ant build target that strips the inner interfaces of IFn and generates the javadoc. Still need to add class javadoc for both API and IFn.

Comment by Alex Miller [ 23/Oct/13 10:49 PM ]

Added a new patch that adds javadoc for the clojure.lang package, the IFn and API classes. It also has an Ant task that generates the javadoc in target/javadoc which you can run with ant javadoc.

Comment by Alex Miller [ 21/Nov/13 3:32 PM ]

The API will live here (prelim version there now): http://clojure.github.io/clojure/javadoc/

Comment by Rich Hickey [ 22/Nov/13 10:37 AM ]

We should never show things like (lookup and simultaneously use var):

API.var("clojure.core", "deref").invoke(printLength);

because people will put that in a loop.

Also, API.var seems like a lot of CAPS. I understand that's proper Java conventions but maybe we need to pick another name?

Comment by Alex Miller [ 05/Dec/13 10:58 AM ]

At Stu's request, I made the updates from our prior conversation, namely:
1) Moved clojure.api.API to clojure.java.api.Clojure and updated all references.
2) Made the example change suggested by Rich to avoid creating and invoking the var in one step.

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

Andy, please let me know before modifying screened patches as they need to be re-screened.

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

In this case, it looks like you didn't leave the old one so I'm not even sure what's different?

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

Oh, I'm misreading the comment history, you just changed the patch field. Carry on.





[CLJ-1188] Public Java API Created: 30/Mar/13  Updated: 12/Apr/13  Resolved: 12/Apr/13

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

Type: Enhancement Priority: Major
Reporter: Stuart Halloway Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJ-1188.patch     Text File CLJ-1188-via-var-intern.patch     Text File CLJ-1188-wrapper-free.patch    
Patch: Code and Test
Approval: Ok

 Description   

Problem: Java consumers need an API into Clojure that does not drag in a ton of concrete implementation detail.

Solution: Very small API class that allows looking up Vars (returning IFn), and reading data (as edn). Uses Clojure logic where possible, e.g. Var.intern.

Current patch: CLJ-1188-via-var-intern.patch

Also considered:

  • wrapper class (inconvenient for users, wrappers anathema in Clojure)
  • common superinterface for IFn and Deref (unnecessary, might bloat vtable)

See also http://dev.clojure.org/display/design/Improvements+to+interop+from+Java



 Comments   
Comment by Kevin Downey [ 02/Apr/13 11:34 AM ]

the attached patch would turn

...
public static Var ENQUEUE = RT.var("greenmail.smtp","enqueue");
...
ENQUEUE.fn().invoke(userManager, state.getMessage());
...

in to something like

...
public static VRef ENQUEUE = API.vref("greenmail.smtp/enqueue");
...
ENQUEUE.fn().invoke(userManager, state.getMessage());
...

what is the value of VRefs over using Vars directly?

Comment by Kevin Downey [ 02/Apr/13 5:56 PM ]

using this from a java api, it looks like if the namespace the var is in is not loaded when you go to create a VRef it will return null, generally my java code that calls clojure looks something like

public static Var FOO = RT.var("namespace", "name");
public static NAMESPACE = Symbol.intern("namespace");
public static Var REQUIRE = RT.var("clojure", "require");

static {
  REQUIRE.invoke(NAMESPACE);
}

can you tell me without checking the java/jvm spec if FOO is null? does the static init block run before or after static fields are init'ed? returning null just seems like a bad idea.

Comment by Stuart Halloway [ 03/Apr/13 6:53 AM ]

Per discussion on the ticket and with Rich, the wrapper-free approach (Apr 3 patch) is preferred.

Comment by Stuart Halloway [ 03/Apr/13 7:03 AM ]

Hi Kevin,

The purpose of not returning Vars is outlined in the design discussion. That said, it is possible to return IFn, which does not drag in too much implementation detail (just a javadoc config tweak, see CLJ-1190). Today's patch returns IFn, which addresses the wrapper ickiness demonstrated by your code examples.

Java static initializers run in lexical order, and I trust Java programmers to know Java.

I can think of several options other than returning null when a var is not available, and they are all complecting, inconsistent with Clojure, or both.

Comment by Kevin Downey [ 03/Apr/13 12:08 PM ]

Hey,

Always returning the var is very consistent with clojure, it is what RT.var does. It is what the var lookups emitted by the compiler do. RT.var is most likely the point through which most java that calls clojure goes at the moment.

As to "hiding" vars, how about creating a clojure.lang.ILink interface with both deref() and fn(), have Var implement it. The previous discussion I see linked all seems to presume hiding Var without discussion of why it should be hidden. What I see Rich saying is:

So RT.var is the right idea. It would be nice to hide the Var class,
but unfortunately we can't make ClojureAPI.var() return both IFn and
IDeref without inventing a common subinterface. There are other
similar details.

and

We will need an interface unifying IDeref and IFn for the return type
of API.var()

It seems like Rich is suggesting that ILink or whatever should also bring in IFn, but I wonder if having a fn() that does the cast for you is an acceptable alternative to that.

Comment by Rich Hickey [ 12/Apr/13 8:24 AM ]

The API should be called var, not fn, and should return IFn. Also, I really want the logic of RT.var (i.e. Var.intern) to be used, not this new logic. Please find another way to handle the string/symbol support.





[CLJ-1184] Evaling #{do ...} or [do ...] is treated as the do special form Created: 16/Mar/13  Updated: 25/Oct/13  Resolved: 25/Oct/13

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

Type: Defect Priority: Trivial
Reporter: Jiří Maršík Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: Compiler

Attachments: Text File CLJ-1184-p1.patch     Text File CLJ-1184-p2.patch     Text File CLJ-1184-p3.patch     Text File CLJ-1184-p4.patch    
Patch: Code and Test
Approval: Ok

 Description   

Problem: Evaluating a persistent collection for which the function first returns the symbol do leads to that collection being treated as the do special form, even though it may be a vector or even a set. IMHO, the expected result would be to report that do cannot be resolved.

[do 1 2]
;=> 2

#{"hello" "goodbye" do}
;=> "hello"
; Wat?

Cause: The check for do is checking for IPersistentCollection instead of ISeq.

Solution: Change the cast (occurs in two places) for the do form check from IPersistentCollection to ISeq:

if(form instanceof IPersistentCollection && Util.equals(RT.first(form), DO))

to

if(form instanceof ISeq && Util.equals(RT.first(form), DO))

Current patch: CLJ-1184-p4.patch

Screened by: Alex Miller



 Comments   
Comment by Gary Fredericks [ 26/May/13 2:13 PM ]

Attached a patch that changes IPersistentCollection to ISeq on the referenced line, and a regression test.

Comment by Nicola Mometto [ 09/Jun/13 2:52 PM ]

As found out on #clojure, there are still some cases where the symbol "do" behaves in unexpected ways that this patch doesn't address.

user=> ((fn [do] do) 1)
nil
user=> (let [do 1] do)
nil

Comment by Gary Fredericks [ 09/Jun/13 3:00 PM ]

Presumably the same issue is the difference between

(let [x 5] do x)

which returns 5 and

(let [x 5] do do x)

which gives a compile error.

Comment by Nicola Mometto [ 09/Jun/13 4:31 PM ]

This is actually a different bug.
I created another ticket with patch+test see http://dev.clojure.org/jira/browse/CLJ-1216

Comment by Alex Miller [ 02/Jul/13 7:11 PM ]

There is a similar case that shows up in Compiler.compile1() around line 7139. Should this also change?

Whatever case that is, would also be nice to have a test for it too.

Comment by Gary Fredericks [ 02/Jul/13 8:59 PM ]

Good catch; this one's a bit trickier to test, since you either have to have a resource on the classpath to compile using clojure.core/compile, or else call the lower-level Compiler/compile directly. To keep the filesystem clean I'm opting for the second one. Path coming shortly.

Comment by Gary Fredericks [ 02/Jul/13 9:52 PM ]

Attached a replacement patch with the second fix. For testing I couldn't call Compiler.compile directly without getting an NPE (that I couldn't reproduce at the repl), so instead I opted to write to a file, use clojure.core/compile, and cleanup the files inside the test.

Comment by Andy Fingerhut [ 05/Jul/13 3:06 PM ]

Presumptuously changing back to its former Vetted state, since the latest patch seems to address the reasons it was marked Incomplete on July 2, 2013.

Comment by Alex Miller [ 23/Jul/13 10:49 PM ]

Updated patch very slightly to fix a spelling typo.

Comment by Alex Miller [ 23/Jul/13 10:55 PM ]

Updated description to help it along a bit and marked Screened.

Comment by Andy Fingerhut [ 14/Aug/13 7:55 PM ]

All 3 of the attached patches no longer apply cleanly to latest master as of Aug 14 2013. This may simply be due to extra tests added to file compilation.clj by the patch for CLJ-1154, which was committed earlier today. If so, it should be pretty straightforward to update the stale patch(es). See the section "Updating Stale Patches" at http://dev.clojure.org/display/community/Developing+Patches

Comment by Gary Fredericks [ 14/Aug/13 9:23 PM ]

Attached a new patch (p4) that should apply. Also halfway reverted a change that Alex made regarding which files are cleaned up after the test. When I run the tests on my machine with his version, several class files are leftover.

Comment by Alex Miller [ 17/Aug/13 7:50 AM ]

Screened.





[CLJ-1183] java interop - cannot call a final method on non-public superclass Created: 13/Mar/13  Updated: 17/Nov/13  Resolved: 17/Nov/13

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

Type: Defect Priority: Major
Reporter: Shlomi Assignee: Unassigned
Resolution: Duplicate Votes: 1
Labels: interop

Attachments: GZip Archive call-test.tar.gz    
Approval: Triaged

 Description   

when trying to call a method on a concrete class that is defined as final on its super class that is not public, the runtime throws:

"java.lang.IllegalArgumentException: Can't call public method of non-public class"

even when fully annotated, Reflection is still used and the call fails.

you can read the full description here https://groups.google.com/d/msg/clojure/p2tBMT-BIYc/mDQB8cSponMJ

I included a sample project that demonstrate the problem



 Comments   
Comment by Shlomi [ 13/Mar/13 6:51 AM ]

in my sample project, i used a nested class, but i didnt have to (as pointed by Marko Topolnik). changing the java code to:

abstract class AbstractParent{
final public int x() {return 6;}
}

public class test extends AbstractParent {}

and the clojure to:

(ns call-test.core (:gen-class))
(defn -main [& args](.x ^AbstractParent (test.)))

would produce the same error,

java.lang.IllegalArgumentException: Can't call public method of non-public class: public final int AbstractParent.x()
at clojure.lang.Reflector.invokeMatchingMethod(Reflector.java:88)

Comment by zoowar [ 16/May/13 12:05 PM ]

This issue affects the upcoming netty-4.0 release in which the public modifier of AbstractBootstrap was removed.

Comment by Matthew Phillips [ 18/May/13 3:48 AM ]

To get Netty 4 working with Clojure I had to create a set of public static Java methods for the various inaccessible Netty calls, which I then call from Clojure. A PITA, but works fine. Happy to post code if anyone would find it useful.

Comment by Shlomi [ 18/May/13 4:31 AM ]

Matthew, i kinda left that project after running to these and other troubles (focused on previous Netty until version 4 will become ready and be properly documented), but i'd still like to see your code. you have a github account or a gist with it?

Clojure devs - are there any plans of checking this problem out? it came up from Netty, but the problem is pretty generic

Comment by Matthew Phillips [ 18/May/13 7:22 PM ]

Shlomi: here's a gist with the code I'm using in it. It's not comprehensive, just the bits I needed.

https://gist.github.com/scramjet/5606195

Comment by Shlomi [ 15/Nov/13 6:17 PM ]

been some time - any plans on addressing this issue?

Comment by Alex Miller [ 16/Nov/13 5:00 PM ]

Someone (I think Stuart Sierra?) did look into this but it was not obvious what the solution could be. It is not currently being considered for Clojure 1.6 but I will bump it into Triaged for post-1.6 consideration.

Comment by Shlomi [ 17/Nov/13 8:12 AM ]

Thanks Alex, I tried looking at it myself, but i couldnt find where in the code clojure chooses between emitting the correct code and going to reflective way. If Stuart or anyone could help me out, i'd be glad to continue looking into it..

Comment by Stuart Sierra [ 17/Nov/13 1:01 PM ]

This looks like the same problem as CLJ-1243. See there for diagnosis.

Comment by Shlomi [ 17/Nov/13 1:28 PM ]

Thanks! I wished this was posted here earlier, would have saved me some time in the past two days

Comment by Alex Miller [ 17/Nov/13 10:21 PM ]

Duplicate of CLJ-1243





[CLJ-1182] Regexp are never equal Created: 12/Mar/13  Updated: 11/Apr/14  Resolved: 05/Sep/13

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

Type: Enhancement Priority: Minor
Reporter: Christian Fortin Assignee: Omer Iqbal
Resolution: Declined Votes: 0
Labels: None

Attachments: File fix-CLJ-1182.diff    
Patch: Code
Approval: Triaged

 Description   

The following (= #"asd" #"asd") will return false in CLJ, even if they are the same.

Consequently, everything with a regexp in it (lists, vectors, maps...) will never be equal.

It seems to have been fixed for CLJS, but not for CLJ.
https://github.com/clojure/clojurescript/commit/e35c3a57472fa62ae41591418a73794dc8ac6dde



 Comments   
Comment by Omer Iqbal [ 12/Mar/13 4:08 PM ]

added an implementation for the equiv method if both args are java.util.regex.Pattern

Comment by Andy Fingerhut [ 12/Mar/13 5:54 PM ]

Omer, could you also include in your patch a few test cases? At least one that validates that two regex's that should be equal, are equal, and another that two regex's that should be different, are non-equal. Preferably the first of those tests should fail with the existing Clojure code, and pass with your changes.

Comment by Omer Iqbal [ 13/Mar/13 5:39 AM ]

I updated the patch with some tests. Hope I added them in the correct file. I also changed the implementation a bit, by instead of adding another implementation of equiv with a different signature, checking the type of the Object in the equiv method with signature (Object k1, Object k2).
For the sake of consistency I also added an EquivPred implementation, though I'm not entirely sure when its used.

Comment by Andy Fingerhut [ 13/Mar/13 1:04 PM ]

All comments here refer to the patch named fix-CLJ-1182.diff dated Mar 13, 2013.

The location for the new tests looks reasonable. However, note that your new patch has your old changes plus some new ones, not just the new ones. In particular, the new signature for equiv is still in your latest patch. You should start from a clean pull of the latest Clojure master and make only the changes you want when creating a patch, not build on top of previous changes you have made.

Also, there are several whitespace-only changes in your patch that should not be included.

Comment by Omer Iqbal [ 13/Mar/13 1:39 PM ]

I uploaded a clean patch, removing the whitespace diff and only adding the latest changes. Thanks for clarifying the workflow!
Just to clarify, this refers to the patch named fix-CLJ-1182.diff dated Mar 13 1:34 PM

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

I am recategorizing this as an enhancement, because if this is a bug it is a bug in Java – the Java Patterns class documents being immutable, but apparently does not implement .equals.

Other recent "make Clojure more complicated to work around problems in Java" patches have been rejected, and I suspect this one will be too, because it might impact the performance of equality everywhere.

Comment by Stuart Halloway [ 12/Apr/13 9:04 AM ]

At first pass, Rich and I both believe that, as regex equality is undecidable, that Java made the right choice in using identity for equality, that this ticket should be declined, and the ClojureScript should revert to match.

But leaving this ticket open for now so that ClojureScript dev can weigh in.

Comment by Michael Drogalis [ 12/Apr/13 9:32 AM ]

What do you mean when you say "undecidable"?

Comment by Alexander Redington [ 12/Apr/13 10:03 AM ]

If Regex instances were interned by the reader, but still used identity for equality, then code like

(= #"abc" #"abc")

would return true, but it wouldn't diverge in the definition of equality for regular expressions between Java and Clojure.

Comment by Fogus [ 12/Apr/13 10:13 AM ]

Undecidable means that for any given regular expression, there is no single way to write it. For example #"[a]" #"a" both match the same strings, but are they the same? Maybe. But how can we decide if /any/ given regular expression matches all of the same strings as any other? The answer is, you can't. Java does provide a Pattern#pattern method that returns the string that was used to build it, but I'm not sure if that could/should be used as a basis for equality given the potential perf hit.

Comment by Herwig Hochleitner [ 12/Apr/13 10:31 AM ]

I posted in Stu's thread: https://groups.google.com/d/topic/clojure-dev/OTPNJQbPtds/discussion
TL;DR: Disagree with undecidability, agree with reverting to identity based equality

Comment by Michael Drogalis [ 12/Apr/13 10:32 AM ]

That makes sense to me. Thanks Fogus.

Comment by Herwig Hochleitner [ 12/Apr/13 9:42 PM ]

From my post to the ml thread, it might not be entirely clear, why I don't think we should implement equality for host regexes:

It would involve parsing and would leave a lot of room for errors and platform-idiosycracies to leak. And it would be different for every platform.

As Alexander said, I also think this ticket could be resolved by interning regex literals, akin to keywords. That, however, would warrant a design page first, because there are tradeoffs to be made about how far the interning should go.

Comment by Rich Hickey [ 13/Apr/13 8:51 AM ]

Why are we spending time on this? Where is the problem statement? Does someone actually need this for a real world purpose not solved by using regex strings as keys?

Comment by Michael Drogalis [ 13/Apr/13 9:13 PM ]

It was prompted as a matter of consistency, which I think is valid. I can't think of a good reason to use regex's as keys though.

Comment by Trevor Hartman [ 11/Apr/14 11:13 AM ]

This issue surfaced as a bug in my code, e.g.: (get {#"^named$" 1} #"^named$") ;=> nil ; surprise!





[CLJ-1179] distinct? does not accept zero arguments Created: 09/Mar/13  Updated: 04/Jan/14  Resolved: 12/Apr/13

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

Type: Enhancement Priority: Minor
Reporter: Jean Niklas L'orange Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: bug

Attachments: Text File clj-1179-distinct-zero-arguments.txt    
Patch: Code

 Description   

distinct? cannot be invoked with zero arguments. When using the pattern (apply distinct? x), this is bothersome as you have to check whether x is empty or not. It is also logical that distinct? should return true if no arguments are given, since there are no duplicates.

What (small set of) steps will reproduce the problem?

user=> (apply distinct? [])
ArityException Wrong number of args (0) passed to: core$distinct-QMARK-  clojure.lang.AFn.throwArity (AFn.java:437)

What is the expected output? What do you see instead?

I would expect distinct? to return true whenever given zero arguments.

What version are you using?

This was tested under Clojure 1.4 and Clojure 1.5.



 Comments   
Comment by Jean Niklas L'orange [ 09/Mar/13 6:08 AM ]

Attached the straightforward patch which solves this issue.

Comment by Devin Walters [ 04/Jan/14 1:32 PM ]

Is there a reason this was closed without a comment?

Comment by Alex Miller [ 04/Jan/14 2:12 PM ]

Rich declined it, implying that it was not a change he wanted. Not sure of the reason.





[CLJ-1178] Requiring the same ns doesn't throw an exception Created: 07/Mar/13  Updated: 29/Mar/13  Resolved: 29/Mar/13

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

Type: Defect Priority: Trivial
Reporter: Michael Drogalis Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

The compiler will happily allow requiring the same ns twice. I can't see any reason you'd intentionally do this.

(ns program
  (:require [program.a :as a]
            [program.a :as b])

This caused some confusion for a while as to why b wasn't producing the expected functionality. Just a simple mistake that I think can be caught at compile time.



 Comments   
Comment by Stuart Halloway [ 29/Mar/13 5:57 AM ]

The example shows valid Clojure code.





[CLJ-1177] java.io URL to File coercion and encoding of non-ASCII characters Created: 07/Mar/13  Updated: 25/Oct/13  Resolved: 25/Oct/13

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

Type: Defect Priority: Minor
Reporter: Trevor Wennblom Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: io

Attachments: Text File clj-1177-patch-v1.txt     File clj-1177-patch-v2.diff     Text File clj-1177-patch-v2.txt    
Patch: Code and Test
Approval: Ok

 Description   

clojure.java.io/resource corrupts path containing UTF-8 characters without issuing warning. (The behavior in the example below is not specific to JDK 8 or Clojure 1.5.0. It is seen with the latest Clojure master as of Sep 15, 2013, and with JDK 6 and JDK 7.)

user=> (System/getProperty "java.runtime.version")
"1.8.0-ea-b79"
user=> (clojure-version)
"1.5.0"
user=> (System/getProperty "user.dir")
"/dir/déf"
user=> (clojure.java.io/resource "myfile.txt")
#<URL file:/dir/d%c3%a9f/resources/myfile.txt>
user=> (slurp (clojure.java.io/resource "myfile.txt") :encoding "UTF-8")
FileNotFoundException /dir/déf/resources/myfile.txt (No such file or directory)  java.io.FileInputStream.open (FileInputStream.java:-2)

Analysis:

The implementation of method as-file of protocol Coercions for class java.net.URL transforms each occurrence of '%xy', where x and y are hex digits in ASCII, to a separate character in the result. The correct behavior is to treat sequences of more than one '%xy' as a byte sequence encoded in UTF-8, where single Unicode code points (i.e. 'Unicode characters') are encoded with anywhere from 1 to 4 bytes.

Patch: clj-1177-patch-v2.diff

Approach:

Change method as-file for class java.net.URL to use method java.net.URLDecoder.decode to decode the contents of a URL string.

http://docs.oracle.com/javase/6/docs/api/java/net/URLDecoder.html#decode%28java.lang.String,%20java.lang.String%29

The only issue with java.net.URLDecoder.decode's behavior is that it changes plus-sign characters to spaces, which according to at least one of the existing unit tests should not happen in as-file. To work around this, first explicitly encode any plus-sign characters in the given URL string, using method java.net.URLEncoder.encode. After that, pass the result to method decode.

http://docs.oracle.com/javase/6/docs/api/java/net/URLEncoder.html#encode%28java.lang.String,%20java.lang.String%29

Other approaches:

Patch clj-1177-patch-v1.txt represents an alternate approach that does its own 'unescaping' of UTF-8 encoded URL strings, without relying on class java.net.URLDecoder. As a result, it is longer and more detailed.

Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 08/Mar/13 12:10 AM ]

Below is a workaround, at least. I don't know, but perhaps the as-file method for URLs in io.clj of Clojure, the part that converts %hh sequences to a character with code point in the range 0 through 255, is at least partly at fault here. I don't know right now if it is possible to modify that code to handle the general case of whatever character encoding munging is going on here to when .getResource creates the URL object.

clojure.java.io/resource is documented to return a Java object of type java.net.URL, which seems like it does %hh escaping of many characters. Reference [1] to a Java bug from 2001 where a Java user was surprised by the then-recent change in behavior of the getResource method [2].

Doing a little searching I found this StackOverflow question [3], which has what might be a workaround. I tried it on my Mac OS X 10.6 system running JDK 1.6 and it seemed to work:

(slurp (.getContent (clojure.java.io/resource "abcíd/foo.txt")))

That getContent is a method for class java.net.URL [4]

[1] http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4466485
[2] http://docs.oracle.com/javase/1.5.0/docs/api/java/lang/Class.html#getResource%28java.lang.String%29
[3] http://stackoverflow.com/questions/13013629/best-international-alternative-to-javas-getclass-getresource
[4] http://docs.oracle.com/javase/1.5.0/docs/api/java/net/URL.html#getContent%28%29

Comment by Trevor Wennblom [ 08/Mar/13 9:56 AM ]

Hi Andy,

Thanks for the background and suggestions, that's very helpful.

I'm gradually learning Clojure with no Java experience. In this case I was searching for the preferred Clojure way to access items in directories declared under :resource-paths in a Leiningen project.clj file. Perhaps clojure.java.io/resource isn't the best way to do this as it's possibly too tied to the expectation for a URI instead of a more general IRI.

You're suggested workaround did work for my use case:

(slurp (.getContent (clojure.java.io/resource "abcíd/foo.txt")))

but hopefully there would be more native/direct Clojure way to accomplish the same eventually.

I don't know if java.net.IDN would be useful internally as a fix in clojure.java.io/resource — I'm assuming not since it wasn't added until Java 6.[1]

user=> (import 'java.net.IDN)
java.net.IDN
user=> (java.net.IDN/toASCII "/dir/déf")
"xn--/dir/df-gya"
user=> (java.net.IDN/toUnicode "xn--/dir/df-gya")
"/dir/déf"

[1]: http://docs.oracle.com/javase/6/docs/api/java/net/IDN.html

Comment by Andy Fingerhut [ 08/Mar/13 1:30 PM ]

Patch clj-1177-patch-v1.txt dated Mar 8 2013 is an attempt to solve this issue, in what I think may be a correct way. As specified in RFC 3986, when taking a Unicode string and making a URL of it, it should be encoded in UTF-8 and then each individual byte is subject to the %HH hex encoding. This patch reverses that to turn URLs into file names.

Tested on Mac OS X 10.6 with a command line like this (it doesn't work without the -Dfile.encoding=UTF-8 option on my Mac, probably because the default encoding is MacRoman):

% java -cp clojure.jar:path/to/resource -Dfile.encoding=UTF-8 clojure.main
user=> (require '[clojure.java.io :as io])
nil
user=> (io/resource "abcíd/foo.txt")
#<URL file:/Users/jafinger/clj/clj-ns-browser/resource/abc%c3%add/foo.txt>
user=> (slurp (io/resource "abcíd/foo.txt"))
"The quick brown fox jumped over the lázy dög!\n"

Comment by Alex Miller [ 24/Jul/13 10:08 PM ]

I think the original code and all of these suggestions are missing more obvious answers already in the JDK (and better).

1. URLs can be converted to URIs which can be passed to the File constructor:

(java.io.File. (.toURI (io/resource "abcíd/foo.txt")))

2. Or we could also leverage URLDecoder instead of that nasty escaping mess currently in the code.

(java.io.File. 
  (URLDecoder/decode 
    (.getFile (io/resource "abcíd/foo.txt")) 
    "UTF-8")))
Comment by Alex Miller [ 24/Jul/13 10:41 PM ]

One big caveat: the alternatives I gave above only work for absolute URLs. Relative URLs would need some massaging. I think to cover those, #2 would be better as it gives you a hook to look at the output of getFile and decide whether it's relative.

Comment by Andy Fingerhut [ 25/Jul/13 8:46 PM ]

On my system (Mac OS X 10.8.4, JVM 1.7.0_15):

#1 has the same problem of munging characters as the current code does. At least, I got errors trying to open a file with an accented "a" in it, because it tried to open a file with a name that had two characters in place of the accented "a".

#2 is better, but it fails with one of the tests that calls (clojure.java.io/as-file (URL. "file:bar+baz")). With your version #2, URLDecoder/decode changes the plus to a space, and the test comparison to the expected result of (File. "bar+baz") fails. I don't know if that is a good test or not, but if it is, the documentation I read for URLDecoder/decode suggests that it will always change plus to space, regardless of whether it is an absolute or relative URL.

Comment by Andy Fingerhut [ 01/Sep/13 10:51 AM ]

Patch clj-1177-patch-v2.txt dated Sep 1 2013 uses URLDecoder/decode to do the decoding of the URL, but only after encoding any plus signs in the URL first, so that they remain plus signs in the returned file name, and are not changed to spaces.

This patch also adds one new test for as-file.





[CLJ-1176] clojure.repl/source fails when *read-eval* bound to :unknown Created: 06/Mar/13  Updated: 31/Jan/14  Resolved: 31/Jan/14

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

Type: Defect Priority: Minor
Reporter: Tim McCormack Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: repl

Attachments: Text File 0001-CLJ-1176-Bind-read-eval-true-in-clojure.repl-source-.patch     Text File clj-1176-source-read-eval-2.patch     Text File clj-1176-source-read-eval-3.patch    
Patch: Code and Test
Approval: Ok

 Description   

clojure.repl/source is broken in Clojure 1.5.1 when *read-eval* is bound to :unknown, since source-fn reads without binding.

user> (alter-var-root #'*read-eval* (constantly :unknown))
:unknown
user> (source drop-last)
RuntimeException Reading disallowed - *read-eval* bound to :unknown  clojure.lang.Util.runtimeException (Util.java:219)

Approach: Throw explicit error stating the cause in this case.

Patch: clj-1176-source-read-eval-3.patch

Screened by: Stuart Sierra



 Comments   
Comment by Tim McCormack [ 06/Mar/13 4:04 PM ]

The attached patch just binds *read-eval* to true inside source-fn.

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

Note: Allowing this implies that you trust the data on your classpath. If there are reasons somebody might not, we should reject this patch and people will have to be explicit when calling source.

Comment by Tim McCormack [ 29/Mar/13 6:37 AM ]

Ugh, that's a fair point when it comes to sandboxing. I'll check with the owners of clojurebot and lazybot.

Comment by Tim McCormack [ 04/May/13 10:40 PM ]

I haven't come up with any scenarios where this is problematic, and I haven't heard anything back from the bot owners. As for sandboxing, clojure.repl can easily be excluded.

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

Looks good

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

Would like to screen this one again. I think it has open questions and is worth a discussion somewhere.

Comment by Alex Miller [ 11/Oct/13 4:47 PM ]

To me, this seems like we would be opening a security hole and a cleverly concocted resource could exploit it.

Other alternatives:
1) do nothing (user can always bind read-eval around a call to source if they want to do this safely)
2) add a source-unsafe or other wrapper function that did this
3) change source-fn to use edn/read instead? This may introduce some cases where source using non-edn features could not be read. I'd be ok with that.

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

Maybe this is well known to everyone already, but in case not, doing a require or use on a namespace containing the following function on a Unix-like system to create and/or update the last modification time of the file bartleby.txt. If you remove that file, and then do (source badfn) while read-eval is bound to true, you can see that it will do the shell command again. Obviously much more dangerous side effects could be performed instead of that.

(ns bad.fn)

(defn badfn [x]
  (let [y [#=(clojure.java.shell/sh "touch" "bartleby.txt")]]
    x))

Avoiding that behavior in source-fn, yet still showing the source code, would require a different implementation of read other than clojure.core/read and clojure.edn/read.

Comment by Alex Miller [ 17/Oct/13 8:21 AM ]

Based on comments on the mailing list, most people are not concerned about this from a security point of view. I'm going to let this one through and Rich can decide further.

Comment by Rich Hickey [ 25/Oct/13 7:20 AM ]

If you haven't set read-eval and you need to read-eval, then you'd better set it, right? We're not going to do that for you. The only patch that will be accepted for this is one that generates a better error message.

Comment by Alex Miller [ 29/Dec/13 10:47 PM ]

Updated with new patch that detects and throws an error if calling source with read-eval is false.

Comment by Stuart Sierra [ 10/Jan/14 4:37 PM ]

Screened OK.

Note that this only changes the error message when read-eval is bound to false, not when it is bound to :unknown.

Comment by Stuart Sierra [ 10/Jan/14 4:44 PM ]

Changed my mind: resetting to 'incomplete'.

This patch doesn't fix the situation in the original report. To improve the error message, it should handle the :unknown case.

If *read-eval* is false, then source still works as long as the source form doesn't contain #=

Comment by Alex Miller [ 14/Jan/14 9:01 AM ]

Stuart - totally good catch. Things did not work how I thought they worked! I have updated the patch.

Comment by Stuart Sierra [ 17/Jan/14 9:44 AM ]

Screened ✔





[CLJ-1175] NPE in clojure.lang.Delay/deref Created: 06/Mar/13  Updated: 14/Aug/13  Resolved: 14/Aug/13

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

Type: Defect Priority: Minor
Reporter: Alan Malloy Assignee: Stuart Halloway
Resolution: Completed Votes: 3
Labels: None

Attachments: Text File delayed-exceptions.patch    
Patch: Code and Test
Approval: Ok

 Description   

Delays get into a corrupt state if an exception is thrown, allowing deref to behave differently on subsequent calls.

This can manifest in multiple ways, depending on the expression being delayed:

;; calling a local as a function causes an NPE inside clojure.lang.Delay
(let [f #(/ 1 0) d (delay (f))]
  [(try (force d) 
        (catch Exception e e))
   (try (force d)
        (catch Exception e e))])
[#<ArithmeticException java.lang.ArithmeticException: Divide by zero> 
 #<NullPointerException java.lang.NullPointerException>]
;; if nil is a valid value, this can cause subsequent forces to "succeed"
;; even though the first fails as it should
(let [x (java.util.Date.) d (delay (seq x))]
  [(try (force d) 
        (catch Exception e e))
   (try (force d)
        (catch Exception e e))])
[#<IllegalArgumentException java.lang.IllegalArgumentException: Don't know how to create ISeq from: java.util.Date> 
 nil]

Cause: clojure.core/delay creates a ^:once function, promising to call it only once, so that the compiler can do locals-clearing on its lexical closure. However, when an exception is thrown the Delay object is left thinking it has never called the function, so when it is forced again it calls the function again, breaking its promise to the compiler and causing the function to run after its locals have been cleared.

Solution: Make Delay remember the first exception that was thrown, and rethrow it on subsequent force attempts. This makes sense, in a way: the "result" of the expression was this exception e being thrown, and so this should happen every time.

Patch: delayed-exceptions.patch



 Comments   
Comment by Alex Miller [ 08/Aug/13 2:07 AM ]

Cleaned up description.





[CLJ-1174] Website doc link for 1.4 api docs returns a 404 Created: 05/Mar/13  Updated: 05/Mar/13  Resolved: 05/Mar/13

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

Type: Defect Priority: Minor
Reporter: Ed O'Loughlin Assignee: Tom Faulhaber
Resolution: Completed Votes: 0
Labels: None
Environment:

All



 Description   

The API docs for Clojure 1.4 (http://clojure.github.com/clojure/branch-clojure-1.4.0/index.html), linked to from http://clojure.github.com/clojure/, returns a 404.

I logged it under this category because I can't see anywhere else to log bugs about clojure.org.



 Comments   
Comment by Tom Faulhaber [ 05/Mar/13 8:29 PM ]

Fixed. Thanks for pointing this out.





[CLJ-1173] One-arg protocol functions whose name begins in a dash generates a call to a wrong field in the emitted code Created: 01/Mar/13  Updated: 17/May/13  Resolved: 17/May/13

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

Type: Defect Priority: Major
Reporter: Meikel Brandmeyer Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None
Environment:

Clojure 1.4



 Description   
(defprotocol P (-foo [this]))

This code generates a reflective call to a non-existing foo field instead of the correct -foo method.

I was told by Christophe Grand that changing the line 557 in core_deftype.clj from:

(. ~(with-meta target {:tag on-interface}) ~(or on-method method) ~@(rest gargs))

to

(. ~(with-meta target {:tag on-interface}) (~(or on-method method) ~@(rest gargs)))

is a quick fix. However I don't know too much about the compilation specifics of . to judge whether this is the correct fix.

Issue reproduction:

Clojure
user=> (set! *warn-on-reflection* true)
true
user=> (defprotocol P (-foo [this]))
P
Reflection warning, REPL:4 - reference to field foo can't be resolved.


 Comments   
Comment by Gabriel Horner [ 17/May/13 1:36 PM ]

CLJ-1202 addresses this exact issue with the same fix and includes tests





[CLJ-1171] Compiler macro for clojure.core/instance? disregards lexical shadows on class names Created: 27/Feb/13  Updated: 14/Aug/13  Resolved: 14/Aug/13

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

Type: Defect Priority: Major
Reporter: