<< Back to previous view

[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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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-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: Herwig Hochleitner Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-CLJ-1171-Tests-for-clojure.core-instance-compiler-ma.patch     Text File 0002-CLJ-1171-Obey-lexical-scope-for-class-argument-in-in.patch     Text File 0003-CLJ-1171-Check-arity-in-instance-compiler-macro.patch    
Patch: Code and Test
Approval: Ok

 Description   

Summary

The compiler tries to emit jvm native instanceof expressions for direct clojure.core/instance? calls.
For that, it tries to resolve its first argument as a class name. However, it disregards lexical bindings when doing that.
This is incongruent to the default implementation in core.clj

Patches

[Stu] All three patches should be applied IMO.

  • 0002 makes instance? respect lexical bindings
  • 0003 makes instance?'s compiled form check arity, consistent with higher-order behavior
  • 0001 has a minimal test for both 0002 and 0003.

Data

Test case

user=> (let [Long String] (instance? Long "abc"))
false
;; expected true as in
user=> (let [Long String] (apply instance? [Long "abc"]))
true

Culprit method

https://github.com/clojure/clojure/blob/4ccb10edbe66eae81336a4c0972050d9759b0bf7/src/jvm/clojure/lang/Compiler.java#L3578

List Discussion

https://groups.google.com/d/topic/clojure/mf25OlFRpa8/discussion

Tangent

This was discovered because the same compiler macro also omits the arity check implicit in the default definition. This could also conveniently be fixed when touching that method:

user=> (instance? String)
false
;; expected
user=> (apply instance? [String])
ArityException Wrong number of args (1) passed to: core$instance-QMARK-  clojure.lang.AFn.throwArity (AFn.java:437)

EDIT elaborated on ticket title and description; added tangent



 Comments   
Comment by Herwig Hochleitner [ 27/Feb/13 8:11 PM ]

Attached patches test and fix issue + tangent

Comment by Herwig Hochleitner [ 04/Mar/13 3:51 PM ]

Note: Patch 0003 just adds the arity check, hence is optional, but if it's omitted from the patchset, the corresponding test from patch 0001 will fail.

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

Summarizing the decisions in these patches:

  • instance? and apply instance? should be consistent
  • they should check arity (matching apply instance? existing behavior)
  • they should allow lexical shadowing of the class argument (matching apply instance? existing behavior)

It is possible (although unlikely) that existing code relies on the current eccentric behavior of instance?. I think it would be fair to categorize programs relying on this behavior as buggy, but that is easy for me to say.





[CLJ-1164] typos in instant.clj Created: 14/Feb/13  Updated: 24/May/13  Resolved: 24/May/13

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: Steve Miner Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJ-1164-typos-instant.patch    
Patch: Code
Approval: Ok

 Description   

There are a few typographical mistakes in instant.clj.



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

Fixes a couple of typos. No code changes.

Comment by Gabriel Horner [ 10/May/13 11:25 AM ]

For anyone wondering about the UTC change see CLJ-928





[CLJ-1161] sources jar has bad versions.properties resource Created: 11/Feb/13  Updated: 25/Apr/14

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

Type: Defect Priority: Minor
Reporter: Steve Miner Assignee: Stuart Halloway
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-CLJ-1161-Remove-version.properties-from-sources-JAR.patch    
Patch: Code
Approval: Incomplete

 Description   

The "sources" jar (at least since Clojure 1.4 and including 1.5 RC) has a bad version.properties file in it. The resource clojure/version.properties is literally:

version=${version}

The regular Clojure jar has the correct version string in that resource.

I came across a problem when I was experimenting with the sources jar (as used by IDEs). I naively added the sources jar to my classpath, and Clojure died on start up. The bad clojure/versions.properties file was found first, which led to a parse error as the clojure version was being set.

Solution: patch leaves version.properties file out of sources JAR, where it causes problems for tools.



 Comments   
Comment by Steve Miner [ 11/Feb/13 10:04 AM ]

Notes from the dev mailing list:

The "sources" JAR is generated by another Maven plugin, configured here:
https://github.com/clojure/clojure/blob/clojure-1.5.0-RC15/pom.xml#L169-L181

The simplest solution might be to just exclude the file from the sources jar. It looks like maven-source-plugin has an excludes option which would do the trick:

http://maven.apache.org/plugins/maven-source-plugin/jar-mojo.html#excludes

Comment by Jeff Valk [ 21/Apr/14 8:20 AM ]

This issue is marked closed, but I'm still seeing it: the clojure-1.6.0-sources.jar, clojure-1.5.1-sources.jar, etc on Maven Central still contain the bad version.properties files. More specifically, it looks like the fix has been applied to builds in the SNAPSHOTS repository, but not to RELEASES.

Fix applied: https://oss.sonatype.org/content/repositories/snapshots/org/clojure/clojure/
Not fixed: https://oss.sonatype.org/content/repositories/releases/org/clojure/clojure/

Comment by Alex Miller [ 24/Apr/14 4:15 PM ]

Not sure what's needed here, but marking incomplete.

Comment by Jeff Valk [ 25/Apr/14 11:13 AM ]

Would a fix for this update existing sources jars (1.5.1, 1.6.0, etc) on Central? Or would any fix have to wait on the next Clojure release?

Comment by Alex Miller [ 25/Apr/14 12:37 PM ]

For all the same reasons that mutable state is undesirable, changing an existing release jar in the central Maven repository is also undesirable. While it's not technically impossible, we will not update existing releases and this will need to wait for the next. I've looked at this problem a little and I do not yet know enough to know how to fix it or why it even varies between snapshot and release. Help welcome!

In which tool do you see a resulting problem from this?

Comment by Jeff Valk [ 25/Apr/14 11:56 PM ]

Despite the way I phrased the question, I'd hoped that would be the answer. It's the right policy.

Unfortunately, this issue leaves the released sources jars essentially unusable from a tools standpoint. CIDER now has source code navigation from stacktraces – you can jump into both Clojure and Java function definitions from the error/trace. For the latter, the sources jar (for Clojure or any other Java library) needs to be on the classpath as a dev dependency. There's more host interop support in the works for CIDER too ("embrace the host platform"), but not being able to add a dependency on a stable Clojure sources jar presents a wrinkle.

Are the official Clojure releases built by Hudson? The Hudson build right before the 1.6.0 release (#532) and the one right after (#534) both show the exclusion fix, as does the git clojure-1.6.0 tag, which when I check out and build from source, is fine. The Hudson builds with release tags (e.g. 1.6 = #533, 1.6-RC1 = #512, etc), though, don't show any artifacts other than a pom.xml. This would seem to make it rather hard to audit builds...am I missing something?





[CLJ-1160] reducers/mapcat ignores Reduced Created: 11/Feb/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: Christophe Grand Assignee: Christophe Grand
Resolution: Completed Votes: 1
Labels: None

Attachments: File lazy-rmapcat2.diff     File lazy-rmapcat.diff    
Patch: Code and Test
Approval: Ok

 Description   

Problem: clojure.core.reducers/mapcat does not stop on reduced? values.

Demonstration:

(require '[clojure.core.reducers :as r])

(->> (concat (range 100) (lazy-seq (throw (Exception. "Too eager"))))
             (r/mapcat (juxt inc str))
             (r/take 5)
             (into []))
;; Exception Too eager

;; Expected return value: [1 "0" 2 "1" 3]

Cause: r/mapcat introduces an intermediate reduce which swallows the reduced value coming from r/take.

Patch: lazy-rmapcat2.diff



 Comments   
Comment by Stuart Sierra [ 02/Aug/13 2:31 PM ]

Cleaned up description.

Comment by Stuart Sierra [ 02/Aug/13 2:36 PM ]

Original patch lazy-rmapcat.diff had whitespace errors, which I have fixed in a new patch lazy-rmapcat2.diff. I also added the ticket number to the commit message.

Comment by Stuart Sierra [ 02/Aug/13 2:37 PM ]

Screened.





[CLJ-1154] Compile.java closes out preventing java from reporting exceptions Created: 31/Jan/13  Updated: 14/Aug/13  Resolved: 14/Aug/13

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

Type: Defect Priority: Major
Reporter: Robert (Bobby) Evans Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None

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

 Description   

Problem: I was trying to compile a project that has some native dependencies (using clojure-maven-plugin version 1.3.13 with Maven 2.0). I forgot to set java.library.path properly so that the native library could be found, and only got an error of

[INFO] ------------------------------------------------------------------------
[ERROR] BUILD ERROR
[INFO] ------------------------------------------------------------------------
[INFO] Clojure failed.
[INFO] ------------------------------------------------------------------------

Cause: Compile.java flushes and closes RT.out in the finally block. When out is closed it is unable to write out the stack trace for the UnsatisfiedLinkError that was being thrown. This made it very difficult to debug what was happening.

Solution: Flush, but do not close RT.out in Compile/main. Test is included, but may or may not be worth keeping.

Patch: CLJ-1154.patch



 Comments   
Comment by Stuart Halloway [ 01/Mar/13 10:45 AM ]

I have encountered this problem as well. Did not verify the explanation, but sounds reasonable.

Comment by Alex Miller [ 24/Jul/13 10:39 AM ]

Updated description a bit and added a patch. The code change is just to stop closing RT.out in Compile.main. The test to invoke Compile and check the stream has an annoying amount of setup and teardown so it's a bit messy. Would love to have more io facilities available (with managed cleanup)!

Comment by Stuart Sierra [ 02/Aug/13 8:34 AM ]

Screened. I confirmed with an independent test, by compiling code which spawned a future which would write to STDOUT:

(ns foo)

(future
  (Thread/sleep 1000)
  (.. System out (println "This is the future of Foo.")))

With or without the patch, creating the future prevents the compile process from terminating, but after the patch the output is visible.





[CLJ-1143] Minor correction to doc string of ns macro Created: 10/Jan/13  Updated: 24/May/13  Resolved: 24/May/13

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

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

Attachments: Text File clj-1143-ns-doc-string-correction-v1.txt    
Patch: Code
Approval: Ok

 Description   

The doc string of ns says "If :refer-clojure is not used, a default (refer 'clojure) is used." 'clojure should be replaced with 'clojure.core

Clojure group thread: https://groups.google.com/forum/?fromgroups=#!topic/clojure/rDjZodxOMh8



 Comments   
Comment by Andy Fingerhut [ 10/Jan/13 1:34 PM ]

clj-1143-ns-doc-string-correction-v1.txt dated Jan 10 2013 replaces (refer 'clojure) with (refer 'clojure.core) in the doc string of the ns macro.





[CLJ-1125] Clojure can leak memory when used in a servlet container Created: 11/Dec/12  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: Defect Priority: Critical
Reporter: Toby Crawley Assignee: Unassigned
Resolution: Completed Votes: 14
Labels: memory

Attachments: File threadlocal-removal-tcrawley-2012-12-11.diff     File threadlocal-removal-tcrawley-2013-06-14.diff     File threadlocal-removal-tcrawley-2013-11-24.diff    
Patch: Code
Approval: Ok

 Description   

When used within a servlet container (Jetty/Tomcat/JBossAS/Immutant/etc), the thread locals Var.dvals (used to store dynamic bindings) and LockingTransaction.transaction (used to store the currently active transaction(s)) prevent all of the classes loaded by an application's clojure runtime from being garbage collected, resulting in a memory leak.

Cause: The issue comes from threads living beyond the lifetime of a deployment - servlet containers use thread pools that are shared across all applications within the container. Currently, the dvals and transaction thread locals are not discarded when they are no longer needed, causing their contents to retain a hard reference to their classloaders, which, in turn, causes all of the classes loaded under the application's classloader to be retained until the thread exits (which is generally at JVM shutdown).

Solution: I've attached a patch that does the following:

  • Var.dvals is initialized to a canonical TOP Frame
  • Var.dvals is now removed when the thread bindings are popped to the TOP
  • The outer transaction in LockingTransaction.transaction now removes the thread local when it is finished

There is still the opportunity for memory leaks if agents or futures are used, and the executors used for them are not shutdown when the app is undeployed. That's a solvable problem, but should probably be solved by the containers themselves (and/or the war generation tools) instead of in clojure itself.

This patch has a small performance impact: its use of a try/finally around running transactions to remove the outer transaction adds 4-6 microseconds to each transaction call on my hardware.

Providing an automated test for this patch is difficult - I've tested it locally with repeated deployments to a container while monitoring GC and permgen. All of clojure's tests pass with it applied.

The above is a condensation of:
https://groups.google.com/d/topic/clojure-dev/3CXDe8_9G58/discussion

Patch: threadlocal-removal-tcrawley-2013-11-24.diff

Screened by: Alex Miller - the new patch (since prior screening) has no changes in the LockingTransaction code but has been updated in Var to address the regression logged in CLJ-1299.



 Comments   
Comment by Colin Jones [ 13/May/13 7:30 PM ]

This patch works great for me to avoid OOM/PermGen crashes from classloaders being retained [mine is a non-servlet use case].

Comment by Stuart Halloway [ 24/May/13 9:43 AM ]

Does Tomcat create warnings for Clojure, as described e.g. here?

If so, does this patch make the warnings go away?

Comment by Toby Crawley [ 24/May/13 9:56 AM ]

Stu: that's a good question. I'll take a look at Tomcat this afternoon.

Comment by Stuart Halloway [ 24/May/13 10:04 AM ]

The code that calls transaction.remove() seems unncessarily subtle. There are two exits from the method, and only one is protected by the finally block.

If the "outer" case was a top-level if, the logic would be more clear, and only the "outer" case would need try/finally, which might reduce the performance penalty in the case of deeply nested dosyncs.

Did your transaction overhead of 4-6 microseconds test only one level of dosync, or many?

Comment by Stuart Halloway [ 24/May/13 10:13 AM ]

Because the unwind code calls remove at the top (as opposed to set(null)), the code should now be safe for use with Clojure-defined ThreadLocal subclasses.

Therefore, Var's use of an initialValue should be irrelevant to this patch, and it should be possible to fix this bug with a patch half the size of the current patch, touching only LockingTransaction.runInTransaction and Var.popThreadBindings.

Comment by Toby Crawley [ 14/Jun/13 7:38 AM ]

re: Tomcat ThreadLocal warnings

With Clojure 1.5.1 using my test app (linked below), I see:

Jun 14, 2013 6:35:22 AM org.apache.catalina.loader.WebappClassLoader checkThreadLocalMapForLeaks
SEVERE: The web application [/leak] created a ThreadLocal with key of type [clojure.lang.Var$1] (value [clojure.lang.Var$1@4902919]) and a value of type [clojure.lang.Var.Frame] (value [clojure.lang.Var$Frame@147a2aa6]) but failed to remove it when the web application was stopped. Threads are going to be renewed over time to try and avoid a probable memory leak.
Jun 14, 2013 6:35:22 AM org.apache.catalina.loader.WebappClassLoader checkThreadLocalMapForLeaks
SEVERE: The web application [/leak] created a ThreadLocal with key of type [java.lang.ThreadLocal] (value [java.lang.ThreadLocal@608602ca]) and a value of type [clojure.lang.LockingTransaction] (value [clojure.lang.LockingTransaction@7e214d47]) but failed to remove it when the web application was stopped. Threads are going to be renewed over time to try and avoid a probable memory leak.

With the original patch (threadlocal-removal-tcrawley-2012-12-11.diff) and the one attached today (threadlocal-removal-tcrawley-2013-06-14.diff), I no longer see these warnings.

re: the LockingTransaction.runInTransaction changes

In today's patch (threadlocal-removal-tcrawley-2013-06-14.diff), I modified runInTransaction to have one exit point, and only wrap a call to run with a try/finally in the outer transaction case. It does introduce two locations where run can be called to preserve the case where an inner transaction has null info:

static public Object runInTransaction(Callable fn) throws Exception{
	LockingTransaction t = transaction.get();
        Object ret;
	if(t == null) {
            transaction.set(t = new LockingTransaction());
            try {
                ret = t.run(fn);
            } finally {
                transaction.remove();
            }
        } else {
            if(t.info != null) {
                ret = fn.call();
            } else {
                ret = t.run(fn);
            }
        }

        return ret;
}

However, this will likely not reduce the speed penalty I observed in my testing, as I was only using a single level of dosync when capturing timing data.

re: removing initialValue from dvals

My original solution kept initialValue, but I then apparently discovered cases where the leak still occurred (see the mailing list thread).

Unfortunately, I can neither recreate that case, nor find in my notes, test code, or the clojure code a reason why keeping initialValue would allow the ThreadLocals to leak when popThreadBindings is patched (assuming one doesn't call Var.getThreadBindings from Java without calling Var.popThreadBindings).

Therefore, I've attached a simpler patch (threadlocal-removal-tcrawley-2013-06-14.diff) that just patches LockingTransaction.runInTransaction and Var.popThreadBindings.

I've also created a project that demonstrates the leak with 1.5.1, and that the leak does not appear with this patch applied to 1.6.0-master. See its README for usage details.

The patched version of 1.6.0-master is available as [org.clojars.tcrawley/clojure "1.6.0-clearthreadlocals"] if anyone wants to give it a try in their own projects. Note that since its group isn't 'org.clojure', you may need to add exclusions to your project to prevent another version of clojure being included.

Comment by Andy Fingerhut [ 14/Jun/13 10:56 AM ]

Presumptuously changing ticket approval from Incomplete back to its former Vetted state, since Toby's comments and new patch seem to address the comments that led Stu to change it to Incomplete.

Comment by Toby Crawley [ 02/Aug/13 10:30 AM ]

Stu:

Is there anything else you need from me for this to be applied?

Comment by Chas Emerick [ 04/Aug/13 5:52 PM ]

FWIW, using Toby's Clojure dep with Immutant has eliminated the out-of-permgen errors I used to occasionally get after N app redeployments.

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

I looked at the updated patch and it seems good to me. In the LockingTransaction.runinTransaction code the cases are driven by where t=null and t.info=null. Of those 4 cases, I believe the same call is being made in all but the case of t == null (where a new LockingTransaction is created) and t.info != null. However, I believe since a new txn is created and t.info should start as null, this case does not actually exist in practice.

Greatly appreciate Chas's experience feedback and all of Toby and Stu's work to make this change solid!

Marking screened.

Comment by Alex Miller [ 22/Nov/13 7:59 PM ]

Reverted in 1.6.0-alpha3 based on CLJ-1299 report.

Comment by Toby Crawley [ 24/Nov/13 5:22 PM ]

I just attached a new patch (threadlocal-removal-tcrawley-2013-11-24.diff) that achieves the same ThreadLocal removal as the previous patch, but addresses the issues with binding conveyance reported in CLJ-1299.





[CLJ-1122] Add contributing.md file to github repository (shows clear message on issues/pull request create form) Created: 09/Dec/12  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: Enhancement Priority: Minor
Reporter: Max Penet Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File contributing.patch     Text File contributing-v2.patch    
Patch: Code
Approval: Ok

 Description   

Problem: New contributors are often unaware of the process for submitting tickets and patches for Clojure. In particular, almost daily a contributor sends a pull request to Clojure or a contrib library and the repo owner must them notify them that we don't accept patches.

Solution: Github supports a special contributors markdown file (see https://github.com/blog/1184-contributing-guidelines) that will be shown when users want to create a pull request or issue. The patch provides this file and invites the user to read the guidelines. The content of the file is just a markdown version of http://clojure.org/contributing

This patch covers clojure, but similar could be done on all contrib libraries as well.

Preview (I think this is first patch) here: https://github.com/mpenet/clojure/blob/aef170ca5eca1b71a2eb1ef320223d1277df0e5e/CONTRIBUTING.md

Patch: contributing-v2.patch
Screened by: Alex Miller



 Comments   
Comment by Stuart Halloway [ 02/Jan/13 6:31 AM ]

Please change this to link to the definitive prose, so we don't have to maintain that in two places.

Comment by Max Penet [ 02/Jan/13 6:51 AM ]

Feel free to correct the wording, I used something simple.

Comment by Gabriel Horner [ 17/May/13 9:04 AM ]

Stu, he linked to clojure.org as you requested so I'm moving this along.

Comment by Alex Miller [ 28/Jul/13 9:43 PM ]

Reworded the description to make it follow our guidelines better and point to the patch to look at.





[CLJ-1121] -> and ->> have unexpected behavior when combined with unusual macros Created: 06/Dec/12  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: Minor
Reporter: Gary Fredericks Assignee: Stuart Halloway
Resolution: Completed Votes: 4
Labels: None

Attachments: Text File 0001-CLJ-1121-Reimplement-and-without-recursion.patch     Text File CLJ-1121-v002.patch    
Patch: Code and Test
Approval: Ok

 Description   

My intuitive understanding of the classic threading macros is that the meaning of forms like (-> a b c) can be understood syntactically independent of the meaning of the symbols involved or the fact that the two threading macros are defined recursively. However the recursive definition breaks that expectation. After

(macroexpand-1 (macroexpand-1 '(-> a b c)))

=> (c (-> a b))

c is now in control if it is a macro, and is now seeing the argument (-> a b) rather than (b a) as would be the case if we had written (c (b a)) originally.

Admittedly I do not know of a realistic example where this is an important distinction (I noticed this when playing with a rather perverse use of ->> with macros from korma), but at the very least it means that the behavior of the threading macros isn't quite as easy to accurately explain as I thought it was.



 Comments   
Comment by Gary Fredericks [ 07/Dec/12 9:19 AM ]

I just realized that my patch also implements CLJ-1086.

Comment by Stuart Halloway [ 22/Dec/12 9:48 AM ]

Would be nice if tests also demonstrated that metadata is preserved correctly.

Comment by Gary Fredericks [ 26/Dec/12 8:16 PM ]

New patch in response to stuarthalloway feedback.

Comment by Tim McCormack [ 09/Jan/13 6:47 PM ]

This patch also prevents an infinite loop in the macroexpander when fed the following expression:

(->> a b (->> c d))

Edit: Far simpler example.





[CLJ-1118] inconsistent numeric comparison semantics between BigDecimal and other Numerics Created: 30/Nov/12  Updated: 22/Nov/13  Resolved: 22/Nov/13

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: Major
Reporter: Arthur Ulfeldt Assignee: Unassigned
Resolution: Completed Votes: 3
Labels: math

Attachments: Text File clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v4.txt     Text File clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v5.txt     Text File clj-1118-v6.txt     Text File clj-1118-v7.patch    
Patch: Code and Test
Approval: Ok

 Description   

BigDecimal does not have consistent comparison semantics with other numeric types.

user> *clojure-version*
{:major 1, :minor 5, :incremental 1, :qualifier nil}
user> (== 2.0 2.0M)
true
user> (== 2 2.0M)
false                  <-- this one is not like the others
user> (== 2 2.0)
true
user> (== 2N 2.0)
true
user> (== 2 (double 2.0M))
true
user> (== 1.0M 1.00M)
false    ;; potentially surprising

Patch: clj-1118-v7.patch

Approach: Change equiv for BigDecimals so that instead of using BigDecimal.equals(), it uses BigDecimal.compareTo() and checks the return value is equal to 0.

The javadoc for these methods explicitly states that BigDecimal.equals() will treat values that are otherwise equal numerically, but differ in scale, as not equal. They also say that BigDecimal.compareTo() will return 0 for such BigDecimals.

Doing this also changes the behavior of = when comparing BigDecimal values to other numbers. hash should be consistent with =, so now hash should return the same value for all numerically equal BigDecimal values. This patch modifies hasheq() to return the same value for all numerically equal BigDecimal values, by calling BigDecimal.stripTrailingZeros() on them before hashing. This change also will make 1.0M == 1.00M which was not true before.

Screened by: Alex Miller



 Comments   
Comment by Arthur Ulfeldt [ 30/Nov/12 1:51 PM ]

I understand that the definition of equality between bigDecimals is dependent on both value and scale as in this case:

user> (== 0.000000M 0.0M)
false

I just want to make sure the decission to propagate that semantic across types is intentional. If this is on purpose than this is not a bug.

Comment by Arthur Ulfeldt [ 30/Nov/12 2:03 PM ]

this could be fixed by calling stripTrailingZeros on bigDecimals before comparing them to Longs or BigInts.

(== 2 (double (. 2.0M stripTrailingZeros)))
true

Edited by Andy Fingerhut: Unfortunately that fails for BigDecimal values equal to 0, unless they happen to have a scale that matches what you are comparing it to.

I think a more complete solution is to use BigDecimal's compareTo method, e.g.:

(zero? (.compareTo 2.0M (bigdec 2)))
true

Comment by Timothy Baldridge [ 03/Dec/12 11:31 AM ]

It seems we need some more eyes on this issue, can you bring this up on clojure-dev and see what they think?

Comment by Andy Fingerhut [ 14/Apr/13 4:03 AM ]

Patch clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v1.txt dated Apr 14 2013 changes equiv for BigDecimals so that instead of using BigDecimal.equals(), it uses BigDecimal.compareTo() and checks the return value is equal to 0.

The Java docs for these methods explicitly state that BigDecimal.equals() will treat values that are otherwise equal numerically, but differ in scale, as not equal.

They also say that BigDecimal.compareTo() will return 0 for such BigDecimals.

I'm not sure if this is the preferred behavior for Clojure, but if it is, this patch should do it.

Comment by Andy Fingerhut [ 15/Apr/13 12:18 AM ]

clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v2.txt dated Apr 14 2013 is same as clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v1.txt described in previous comment, except it also has some new tests included.

Comment by Andy Fingerhut [ 15/Apr/13 9:07 PM ]

clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v3.txt dated Apr 15 2013 is the same as the the previous patch clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v2.txt, except for the following:

By changing == behavior for BigDecimal by modifying the BigDecimalOps.equiv() method, that also changes the behavior of = when comparing BigDecimal values to other numbers. hash should be consistent with =, so now hash should return same value for all numerically equal BigDecimal values. This patch should achieve that.

Comment by Andy Fingerhut [ 14/Aug/13 7:29 PM ]

Patch clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v4.txt dated Apr 15 2013 is simply updating the stale -v3.txt patch. The only reason it no longer applied cleanly with git is because a few lines of context changed in the test code.

Comment by Andy Fingerhut [ 27/Sep/13 11:55 AM ]

Quick comment that some of the tests in patch clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v4.txt were written before CLJ-1036 was declined as out of scope for Clojure, and should probably be cut down a bit, at least to eliminate biginteger and float occurrences. I will wait and see if there are any other comments from screening before creating any new patches.

Comment by Alex Miller [ 27/Sep/13 12:21 PM ]

Andy, the tests in the patch don't actually pass for me?

Many failures in the generative tests, but for example:

[java] {:clojure.test/vars (equality-tests),
     [java]  :thread/name "main",
     [java]  :pid 5719,
     [java]  :thread 1,
     [java]  :type :assert/fail,
     [java]  :message
     [java]  "Test that 0 (class java.lang.Byte) #'clojure.core/== 0.0 (class java.math.BigDecimal)",
     [java]  :level :warn,
     [java]  :test/actual false,
     [java]  :test/expected (equal-var val1 val2),
     [java]  :line 58,
     [java]  :tstamp 1380298603830,
     [java]  :file "numbers.clj"}
Comment by Andy Fingerhut [ 27/Sep/13 12:26 PM ]

Could you tell me the OS, version, and JDK version, and the command you used to run this test? The reason I ask is that this patch has been tested on 3 OS/JDK version combos via my prescreen testing, using 'ant', and it passes on all of them, so if it fails on some OS/JDK I am not testing I would like to figure out why.

Comment by Alex Miller [ 27/Sep/13 12:42 PM ]

Sorry - user error. They're passing.

Comment by Alex Miller [ 27/Sep/13 3:12 PM ]

Slightly tweaked patch to remove some of the float/double stuff that I'm not expecting to change.

Marking screened...

Comment by Andy Fingerhut [ 27/Sep/13 7:54 PM ]

Patch clj-1118-v6.txt is tiny modification of Alex Miller's clj-1118-make-double-equals-true-for-more-bigdecimals-patch-v5.txt

It removes hash consistency tests for floats and BigIntegers, with a comment explaining why they should not be there.

Comment by Rich Hickey [ 22/Oct/13 7:55 AM ]

Please don't use .txt as an extension for diffs, it keeps them from loading with appropriate mode in various editors.

v5 was screened but v6 alters it?

Comment by Alex Miller [ 22/Oct/13 8:01 AM ]

Andy's last patch just added a comment. I updated a new version that is identical to v6 but has a .patch extension instead. Marking Screened again.

Comment by Andy Fingerhut [ 22/Oct/13 8:37 AM ]

It was not only adding a comment, but removing tests that should not have been there and could potentially mislead people into thinking that Clojure guarantees hash being consistent with = for BigInteger and Float/Double, which by CLJ-1036 it does not.

Regarding the .txt file extensions, first I've heard of the preference. I can make sure future screened tickets don't have this, but changing currently screened tickets would likely lead to confusion about which ones are screened. M-x diff-mode in emacs will force the mode regardless of file name.

Comment by Alex Miller [ 22/Oct/13 8:53 AM ]

Oh right, that is ok so can stay screened.





[CLJ-1105] clojure.walk should support records Created: 08/Nov/12  Updated: 14/Feb/14  Resolved: 22/Nov/13

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

Type: Enhancement Priority: Minor
Reporter: Jouni K. Seppänen Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: walk

Attachments: Text File 0001-CLJ-1105-Support-records-in-clojure.walk.patch    
Patch: Code and Test
Approval: Ok

 Description   

Problem: clojure.walk throws exceptions if used on records.

user=> (defrecord Foo [x])
user.Foo
user=> (def f (Foo. :x))
#'user/f
user=> (use 'clojure.walk)
nil
user=> (postwalk identity {:foo f})
UnsupportedOperationException Can't create empty: user.Foo  user.Foo (NO_SOURCE_FILE:1)

Current Patch: 0001-CLJ-1105-Support-records-in-clojure.walk.patch adds a special case for records.

See also: CLJ-1239 "faster, more flexible dispatch for clojure.walk" which could supersede this ticket.

Screened by: Alex Miller - I think this will likely be superseded by CLJ-1239 in the future, but this is a reasonable short-term step.



 Comments   
Comment by Nicola Mometto [ 08/Nov/12 2:35 PM ]

maybe clojure should follow clojurescript's footsteps and move empty out of IPersistentCollection and create an
interface IEmptyableCollection extends IPersistentCollection { IEmptyableCollection empty(); }

Comment by Stuart Halloway [ 25/Nov/12 6:39 PM ]

Can whoever claims this please consider walk's behavior in the face of all different collection types? I think it also fails with Java collections.

Also, the collection partitioning code in clojure.data may be of use.

Comment by Stuart Sierra [ 26/Jul/13 7:39 AM ]

clojure.walk can be made to support records directly, see https://github.com/stuartsierra/clojure.walk2

Comment by Alex Miller [ 26/Jul/13 10:18 AM ]

Update title and adjust description slightly to focus this ticket on being an enhancement to make clojure.walk work with records.

Comment by Stuart Sierra [ 29/Jul/13 12:15 PM ]

Attachment 0001-CLJ-1105-Support-records-in-clojure.walk.patch is a minimal patch adding support for records without altering the design of clojure.walk.

Comment by Nicola Mometto [ 29/Jul/13 12:45 PM ]

clojure.walk is not required during bootstrapping, so I don't see why we couldn't just substitute clojure.walk with clojure.walk2 given that it only changes the internal implementation to a faster one

Comment by Chouser [ 14/Feb/14 1:50 PM ]

The way this patch behaves can be surprising compared to regular maps:

(clojure.walk/prewalk-replace {[:a 1] nil} {:a 1, :b 2})
;=> {:b 2}

(defrecord Foo [a b])
(clojure.walk/prewalk-replace {[:a 1] nil} (map->Foo {:a 1, :b 2}))
;=> #user.Foo{:a 1, :b 2}

Note how the [:a 1] entry is removed from the map, but not from the record.

I believe CLJ-1239 also demonstrates this behavior.

Comment by Andy Fingerhut [ 14/Feb/14 1:54 PM ]

This comment would probably get more visibility on CLJ-1239, given that this one is closed and CLJ-1239 is still open.





[CLJ-1102] Better handling of exceptions with empty stack traces Created: 04/Nov/12  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: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: errormsgs

Attachments: File clj-1102-improve-empty-stack-trace-handling-v2.diff     Text File clj-1102-improve-empty-stack-trace-handling-v2.txt    
Patch: Code and Test
Approval: Ok

 Description   

REPL session demonstrating clojure.stacktrace/print-stack-trace and clojure.test/file-and-line throwing exceptions when given Throwable with an empty stack trace:

user=> (require '[clojure.stacktrace :as s])
nil
user=> (def empty-stack (into-array (Class/forName "java.lang.StackTraceElement") []))
#'user/empty-stack
user=> (def t (doto (Throwable.) (.setStackTrace empty-stack)))
#'user/t
user=> (def msg (with-out-str (s/print-stack-trace t)))

NullPointerException   clojure.lang.Reflector.invokeNoArgInstanceMember (Reflector.java:296)
user=> msg
#<Unbound Unbound: #'user/msg>
user=> (require 'clojure.test)
nil
user=> (def m1 (#'clojure.test/file-and-line t 0))

ArrayIndexOutOfBoundsException   java.lang.reflect.Array.get (Array.java:-2)
user=> m1
#<Unbound Unbound: #'user/m1>

I have seen this cause confusing output when exceptions with empty stack traces are thrown while running tests on a project. According to the Java docs for Throwable, it is permissible for getStackTrace to do this:

http://docs.oracle.com/javase/6/docs/api/java/lang/Throwable.html#getStackTrace%28%29

Approach:

I found all places in the Clojure code that call getStackTrace. Among them, two did not handle an empty stack trace correctly.

Output of tests above with this patch applied:

...

user=> (def msg (with-out-str (s/print-stack-trace t)))
#'user/msg
user=> (print msg)
java.lang.Exception: null
 at [empty stack trace]
nil

...

user=> (def m1 (#'clojure.test/file-and-line t 0))
#'user/m1
user=> m1
{:line nil, :file nil}

Patch: clj-1102-improve-empty-stack-trace-handling-v2.diff

Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 04/Nov/12 5:07 PM ]

clj-1102-improve-empty-stack-trace-handling-v1.txt dated Nov 4 2012 improves the handling of .getStackTrace returning a length 0 array in two places. I checked all other places .getStackTrace was called and they seem to already handle this case gracefully.

Comment by Timothy Baldridge [ 30/Nov/12 2:57 PM ]

Vetting.





[CLJ-1101] *default-data-reader-fn* should be set!-able in REPL Created: 03/Nov/12  Updated: 24/May/13  Resolved: 24/May/13

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

Type: Enhancement Priority: Minor
Reporter: Steve Miner Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJ-1101-make-default-data-reader-fn-set-able-in-REPL.patch    
Patch: Code
Approval: Ok

 Description   

In CLJ-927, Nicola Mometto pointed out that *default-data-reader-fn* should be set!-able. The fix needs to be added to clojure.main/with-bindings.



 Comments   
Comment by Steve Miner [ 03/Nov/12 9:32 AM ]

Add *default-data-reader-fn* to the special bindings in main.clj so that it is set!-able in the REPL

Comment by Steve Miner [ 03/Dec/12 10:07 AM ]

This is a one-liner that makes *default-data-reader-fn* convenient to use in the REPL, similar to how *data-readers* works. I'd like to have this fix in Clojure 1.5.

Comment by Steve Miner [ 12/Mar/13 8:10 PM ]

work-around for REPL:

(alter-var-root #'clojure.core/*default-data-reader-fn* (constantly my-default-reader))




[CLJ-1090] Indirect function calls through Var instances fail to clear locals Created: 22/Oct/12  Updated: 25/Oct/13  Resolved: 25/Oct/13

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

Type: Defect Priority: Minor
Reporter: Spencer Tipping Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: performance
Environment:

Probably all, but observed on Ubuntu 12.04, OpenJDK 6


Attachments: File var-clear-locals.diff     File var-clear-locals-patch-v2.diff     Text File var-clear-locals-patch-v2.txt    
Patch: Code
Approval: Ok

 Description   

If you make a function call indirectly by invoking a Var object (which derefs itself and invokes the result), the invocation parameters remain in the thread's local stack for the duration of the function call, even though they are needed only long enough to be passed into the deref'd function. As a result, passing a lazy seq into a function invoked in its Var form may run out of memory if the seq is forced inside that function. For example:

(defn total [xs] (reduce + 0 xs))
(total (range 1000000000))   ; this works, though takes a while
(#'total (range 1000000000)) ; this dies with out of memory error

Solution: Similar to RestFn, wrap each argN in Var inside a Util.ret1(argN, argN = null).

Patch: var-clear-locals-patch-v2.diff

Screened by: Alex Miller



 Comments   
Comment by Spencer Tipping [ 23/Oct/12 1:37 PM ]

Sorry, I typo'd the example. (defn total ...) should be (defn sum ...).

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

fixed typeo in example

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

Couldn't reproduce the exception, but the 2nd example did chew through about 4x the amount of memory. Vetting.

Comment by Timothy Baldridge [ 29/Nov/12 2:57 PM ]

adding a patch. Since most of Clojure ends up running this code in one way or another, I'd assert that tests are included as part of the normal Clojure test process.

Patch simply calls Util.ret1(argx, argx=null) on all invoke calls.

Comment by Timothy Baldridge [ 29/Nov/12 3:17 PM ]

And as a note, both examples in the original report now have extremely similar memory usages.

Comment by Spencer Tipping [ 30/Nov/12 2:22 PM ]

Sounds great, and the patch looks good too. Let me know if I need to do anything else.

Comment by Alex Miller [ 22/Aug/13 10:35 PM ]

Switching back to Triaged as afaict Rich has not Vetted this one.

Comment by Andy Fingerhut [ 05/Sep/13 6:18 PM ]

Patch var-clear-locals-patch-v2.txt is identical to var-clear-locals.diff (and preserves credit to its author), except it eliminates trailing whitespace in some added lines that cause git to give warnings when applying the patch.





[CLJ-1083] Incorrect ArityException message for function names containing -> Created: 09/Oct/12  Updated: 25/Oct/13  Resolved: 25/Oct/13

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

Type: Defect Priority: Minor
Reporter: Alex Nixon Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: errormsgs

Attachments: File better-throw-arity-messages.diff     Text File clj-1083-better-throw-arity-messages-patch-v3.txt     Text File clj-1083-better-throw-arity-messages-patch-v5.txt     File clj-1083-better-throw-arity-messages-patch-v6.diff     Text File clj-1083-better-throw-arity-messages-patch-v6.txt     File clj-1083-better-throw-arity-messages-patch-v7.diff    
Patch: Code and Test
Approval: Ok

 Description   

Problem: Error messages show munged symbol names.

user=> (defn a->b [])
#'user/a->b
user=> (a->b 1)
ArityException Wrong number of args (1) passed to: user$a  clojure.lang.AFn.throwArity (AFn.java:437)

Note that the reported function name in the stack trace is "user$a", where it should be "user$a->b" (or some mangled variant thereof?)

Patch: clj-1083-better-throw-arity-messages-patch-v7.diff

Approach:

Demunge the name to print a better exception message. Note: demunging is not reversible if the original symbol contains a munged word (GT, LT, PLUS, etc). These cases are rare in actual code.

To avoid introducing a dependency on Clojure code from Java code, add new demunge() method to class Compiler, near the existing munge() method. Also replace the two existing Clojure implementations of demunge with a call to this new Java demunge(). Test contains both a couple example tests and a generative test for name munge/demunge roundtrip.

See discussion here: https://groups.google.com/d/msg/clojure/PVNoLclhhB0/_NWqyE0cPAUJ

Screened by: Alex Miller



 Comments   
Comment by Timothy Baldridge [ 26/Nov/12 10:27 AM ]

Fix for this defect.

Comment by Timothy Baldridge [ 26/Nov/12 10:30 AM ]

The throwArity now attempts to locate and call clojure.main/demunge. If it finds the function it invokes it and uses the returned string in the error. Otherwise it just throws the actual class name. This results in the following behaviour:

user=> (defn a->b [])
#'user/a->b
user=> (a->b 32)
ArityException Wrong number of args (1) passed to: user/a->b clojure.lang.AFn.throwArity (AFn.java:449)
user=>

Comment by Stuart Halloway [ 24/May/13 11:35 AM ]

Timothy: Why the empty catch block? I don't see anything in the try block whose failure we would want to ignore.

Comment by Timothy Baldridge [ 30/May/13 2:02 PM ]

The demunge function is created inside of a .clj file. So it is possible that we could hit this exception before demunge exists. The try simply says "if we can get a better error message, use it. Otherwise, fall back to the old (half-broken) method of getting method names"

Comment by Andy Fingerhut [ 07/Jun/13 2:01 PM ]

Presumptuously changing approval from Incomplete back to its former value of Vetted after Timothy responded to what I believe is the reason it was marked Incomplete (Stu's question).

Comment by Alex Miller [ 02/Jul/13 6:47 PM ]

There are some unnecessary whitespace changes in the patch.

Is it ok to add a dependency from AFn (even if an optional one) to clojure.main/demunge? (The same code is repeated in clojure.repl/demunge btw.) Seems like it would be better for something more common to have a demunge function that all of these could call.

Shouldn't we have a test in the patch?

Changing back to Incomplete.

Comment by Andy Fingerhut [ 23/Aug/13 12:44 PM ]

Is Rich the only person that can authoritatively answer Alex's question "Is it ok to add a dependency from AFn (even if an optional one) to clojure.main/demunge?"

I am not sure, but it seems the only way to avoid a dependency from AFn to Clojure code in this case would be to write a version of demunge in Java.

Comment by Andy Fingerhut [ 23/Aug/13 1:19 PM ]

Patch clj-1083-better-throw-arity-messages-patch-v2.txt dated Aug 23 2013 includes all of Timothy Baldridge's patch better-throw-arity-messages.diff dated Nov 26 2012 except it leaves out the unnecessary whitespace changes. It also adds a new test that fails without his patch, and passes with it.

It still has a dependency from AFn to clojure.main/demunge, so if that is not acceptable, something else will be needed.

Comment by Andy Fingerhut [ 23/Aug/13 7:52 PM ]

Patch clj-1083-better-throw-arity-messages-patch-v3.txt dated Aug 23 2013 includes all of Timothy Baldridge's patch better-throw-arity-messages.diff dated Nov 26 2012 except it leaves out the unnecessary whitespace changes. It also adds a new test that fails without his patch, and passes with it, and fixes a bug with both copies of the demunge implementation that caused it to transform some munged names incorrectly.

It still has a dependency from AFn to clojure.main/demunge, so if that is not acceptable, something else will be needed.

Comment by Alex Miller [ 26/Aug/13 5:23 PM ]

I still think AFn calling into clojure.main/demunge is weird.

Some other alternatives:

1) Move demunge to clojure.stacktrace (which is also not demunging). Still weird to be calling from the Java portions into the Clojure portions (this is not done elsewhere).

2) Another alternative would be to implement demunge() in another class (putting it in Compiler introduces another weird cycle), perhaps in Util?

Comment by Andy Fingerhut [ 26/Aug/13 7:44 PM ]

Alex, sorry if I'm being a bit dense here, but how does adding a Java version of demunge() into class Compiler introduce a cycle of dependencies? I think all Java source files are compiled before any Clojure source files, so I am guessing you are referring to a dependency between different Java classes? To be clear, when I suggested writing a Java version of demunge(), I mean a "pure Java" implementation that does not refer to anything in the Clojure source files.

My main reason for suggesting adding it to class Compiler is simply to keep the related munge() and proposed new demunge() methods next to each other, and the fields they use.

Comment by Alex Miller [ 27/Aug/13 6:33 AM ]

Sorry, I was thinking that it might be weird for AFn to depend on Compiler, but maybe that doesn't matter. Putting it next to munge() makes a lot of sense.

Comment by Andy Fingerhut [ 27/Aug/13 8:33 AM ]

Patch clj-1083-better-throw-arity-messages-patch-v4.txt dated Aug 27 2013 adds a Java implementation of a demunge method to the Compiler class, and uses it in place of previously-existing Clojure implementations of demunge, as well as in the code for throwing arity exceptions.

It introduce a dependency on the Compiler class from class AFn, but not any dependencies on Clojure code from Java code.

Comment by Andy Fingerhut [ 27/Aug/13 9:54 AM ]

Patch clj-1083-better-throw-arity-messages-patch-v5.txt dated Aug 27 2013 is identical to previously attached -v4 described above, except it leaves out an unintentional change to the duration of Clojure's generative tests.

Comment by Alex Miller [ 02/Sep/13 10:43 PM ]

I reworked Andy's patch to clean up the Java code in Compiler. The logic seems correct. I added a generative test that checks for roundtripping a symbol name through munge/demunge. The only tricky part of that is avoiding symbol names which cannot properly roundtrip.

Marking screened.

Comment by Andy Fingerhut [ 03/Sep/13 6:24 AM ]

Thanks, Alex. I don't want to derail all of our work because of what is described in this comment, but wanted to record it somewhere after tracking it down.

The fact that demunge as it existed before this patch (and as it still behaves after this patch), can produce names that are not the original function name makes me finally realize why Chas Emerick made this comment in the email thread linked in the description:

"This is perhaps another case where pushing var metadata (or some subset thereof) down to the function being defined in defns would be beneficial; AFunction could then override throwArity to just use the :name of the function being called, thus avoiding any confusion introduced by munged or un-munging names."

It seems that if Chas's idea were implemented, it could be used to improve the output of throwArity to give the correct original Clojure function name in all cases. However, even then the demunge'ing of StackTraceElement class names could not be similarly improved (e.g. in the output of clojure.repl/pst), because all they contain are class names as strings, so functions b? and b_QMARK_ will always appear identical in a StackTraceElement.

Given all of that, I'd say take the best patch for this ticket, and simply advise people not to use substrings like "QMARK" or "COLON" in their function names. If they do, they should be prepared for confusion due to demunge'ing when examining stack traces with functions like clojure.repl/pst

Comment by Alex Miller [ 03/Sep/13 7:47 AM ]

I think the current approach is fine. Munge never made any promises about being reversible.

Given typical naming styles in Clojure, this should rarely be an issue. The consequence is that now you'll get a wrongly demunged (instead of wrongly munged) version; in either case you're not getting the original function name. I enjoyed discovering this detail via the generative tests!!

I do agree that having some way to recover the original function name would be a better solution.

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

I've not looked into details yet, but screened patch clj-1083-better-throw-arity-messages-patch-v6.diff fails to apply cleanly as of Oct 25, 2013 latest Clojure master.

Comment by Alex Miller [ 25/Oct/13 1:12 PM ]

Updated patch to apply cleanly to master as of Oct 25, 2013.





[CLJ-1082] Subvecs of primitive vectors cannot be reduced Created: 05/Oct/12  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: Defect Priority: Major
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: collections
Environment:

1.7.0-08, OS X 10.8


Attachments: Text File clj-1082.patch     File clj-1082-patch-v2.diff     Text File clj-1082-patch-v2.txt     File clj-1082-patch-v3.diff    
Patch: Code and Test
Approval: Ok

 Description   

Reduce doesn't work on subvecs of primitive vectors.

(let [prim-vec (into (vector-of :long) (range 10000))]
  (reduce + (subvec prim-vec 1 500)))

->> ClassCastException clojure.core.Vec cannot be cast to clojure.lang.PersistentVector  clojure.lang.APersistentVector$SubVector.iterator (APersistentVector.java:523)

If reduce on a subvec doesn't work then neither will nice ops like fold.

Cause: RT.subvec() creates an APersistentVector$SubVector. SubVector.iterator() assumes the source vector is a PersistentVector, however a primitive vector is a Vec (which is not a PersistentVector). This causes a class cast exception as observed on any attempt to iterate over the subvector.

Approach:
1. Provide a generic ranged iterator for APersistentVector, that can be used by SubVector
2. Make the iterator() method for APersistentVector$SubVector use this new iterator where possible (i.e. whenever the source vector is an APersistentVector). If not, the generic super.iterator() method is used (which is slower, but safe for any source vector that implements IPersistentVector)

Patch: clj-1082-patch-v3.diff

Screened by: Alex Miller



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

Confirmed to be broken on master. Vetting. Not sure what it's going to take to fix this, however. If this is of intrest for you, you might want to help push it along by providing a patch.

Comment by Andy Fingerhut [ 27/Nov/12 12:09 PM ]

There is no code or ticket for this yet, but I wanted to mention that I've begun working on an implementation of RRB-Tree (Relaxed Radix Balanced Tree) vectors for Clojure (see discussion at https://groups.google.com/forum/?fromgroups=#!topic/clojure-dev/xnbtzTVEK9A). Assuming it is no big deal to get reducers to work on such vectors, subvec could be implemented in O(log n) time in such a way that the result was the same concrete type of vector as you started with, and thus reducers would work on them, too.

It would mean O(log n) time for subvec instead of today's O(1), but this would likely fix other limitations that exist today with subvec's, e.g. CLJ-761.

Comment by Mike Anderson [ 20/Jan/13 5:14 AM ]

I have a fix for this and a regression test in the tree below:

https://github.com/mikera/clojure/tree/winfix

Not sure how best to make this into a patch though - I also have the pprint fix in here (CLJ-1076)

Comment by Mike Anderson [ 20/Jan/13 6:41 PM ]

Attached a patch that I created with:

git format-patch winfix --stdout HEAD~3..HEAD > clj-1082.patch

Does this do the trick? I had to use the HEAD~3..HEAD to just get the most recent 3 commits and exclude the pprint changes that I needed in order to build on Windows.

Comment by Mike Anderson [ 20/Jan/13 6:42 PM ]

Patch for CLJ-1082, containing 3 commits

Comment by Andy Fingerhut [ 21/Jan/13 1:11 AM ]

Mike, your patch clj-1082.patch applies cleanly to latest master for me, so looks like you found one way to do it.

Another would be as follows, and closer to the directions on the JIRA workflow page: http://dev.clojure.org/display/design/JIRA+workflow (but not identical). Note that these commands would work on Mac OS X or Linux. I'm not sure what the correct corresponding command would be on Windows for the "git am" step below, unless that just happens to work because Windows and/or git implement the input redirection with "<" somehow.

  1. Check out a fresh repo
    $ git clone git://github.com/clojure/clojure.git
    $ cd clojure
  1. Apply the patch for CLJ-1076 to the master branch. This step isn't on the JIRA workflow page.
    $ git am --keep-cr -s < clj-1076-fix-tests-on-windows-patch-v1.txt
  1. Create a branch for yourself
    $ git checkout -b fix-clj-1082
  1. After editing to make your changes, commit them to the current fix-clj-1082 branch
    $ git commit -a -m "fixed annoying bug, refs #42"

From there on down it is the same as the instructions on the JIRA workflow page. The "git format-patch master --stdout > file.patch" will create a patch for the changes you have made in the current branch fix-clj-1082 starting from the master branch, which has the CLJ-1076 fix because of the 'git am' command above.

Comment by Alex Miller [ 22/Aug/13 10:36 PM ]

Moving back to Triaged as Rich has not vetted.

Comment by Andy Fingerhut [ 05/Sep/13 6:12 PM ]

clj-1082-patch-v2.txt is identical to Mike Anderson's clj-1082.patch, preserving his authorship, except it eliminates a carriage return added at the end of one line, which also causes git to issue a warning when applying the patch.

Comment by Rich Hickey [ 22/Nov/13 7:24 AM ]

This diff remains inscrutable. It seems to have two patches in it, one a first idea and another a modification to that? Patches should be direct enhancements to the trunk code. Also, what is endIndex for and why is it mutable? Why not just use end? And, the code doesn't agree with the plan, which says "Check the vector type and if it is an APersistentVector, use the existing logic. Otherwise, fallback to a new rangedIterator() implementation in APersistentVector that iterates using nth." while the code seems to do the opposite:

+ if (v instanceof APersistentVector) { + return ((APersistentVector)v).rangedIterator(start,end); + }
+ return super.iterator();

Comment by Mike Anderson [ 22/Nov/13 11:00 AM ]

Hi Rich,
1. As per comments, Andy made a small change to the original patch. v2 supersedes the original patch.
2. endIndex is part of the iterator implementation: I believe this must be mutable to provide the required Java Iterator behaviour
3. I think the approach is misworded (it was added long after the patch), I shall try to improve this.

Comment by Mike Anderson [ 22/Nov/13 12:32 PM ]

I don't seem to have the ability to edit the description. Here's what I think it should say:

Cause: RT.subvec() creates an APersistentVector$SubVector. SubVector.iterator() assumes the source vector is a PersistentVector, however a primitive vector is a Vec (which is not a PersistentVector). This causes a class cast exception as observed on any attempt to iterate over the subvector.

Approach:
1. Provide a generic ranged iterator for APersistentVector, that can be used by SubVector
2. Make the iterator() method for APersistentVector$SubVector use this new iterator where possible (i.e. whenever the source vector is an APersistentVector). If not, the generic super.iterator() method is used (which is slower, but safe for any source vector that implements IPersistentVector)

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

Updated description per Mike's comment.

Comment by Alex Miller [ 24/Nov/13 2:15 PM ]

Added new v3 patch that a) combines the previous commits into a single patch and b) removes endIndex and uses end instead. As far as I know this + the description change address all of Rich's questions. Marking re-screened.





[CLJ-1076] pprint tests fail on Windows, expecting \n Created: 26/Sep/12  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: Critical
Reporter: Ivan Kozik Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: print
Environment:

Windows 7


Attachments: Text File clj-1076-fix-tests-on-windows-patch-v1.txt     Text File clj-1076-fix-tests-on-windows-patch-v2.txt     Text File clj-1076-v3.txt     Text File pprint_test_failures_01b4cb7156.txt    
Patch: Code and Test
Approval: Ok

 Description   

New pprint tests were committed recently, but they fail on Windows because the tests check for \n, while pprint seems to output \r\n. A log with the test failures is attached.

The first failing commit is https://github.com/clojure/clojure/commit/4ca0f7ea17888ba7ed56d2fde0bc2d6397e8e1c0

Patch: clj-1076-v3.txt

Approach: Before comparing output of pprint against a string in the unit test, split each of those strings into sequences of lines using clojure.string/split-lines, which removes occurrences of the regex #"\r?\n" between lines, and can thus safely be used to compare multiline strings between platforms that use only a newline, and those that use carriage return plus newline.

Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 29/Sep/12 2:27 PM ]

Patch clj-1076-fix-tests-on-windows-patch-v1.txt dated Sep 29 2012 when applied to the particular commit that Ivan mentions causes the tests to pass when I run "ant" on a Windows 7 machine for me, and it continues to pass all tests on Mac OS X 10.6.8, too.

I may be doing something wrong, but when I try to run "ant" to build and test on Windows 7 with latest Clojure master, with or without this patch, it fails right at the beginning of the tests because it can't find clojure.test.generative. I'm probably doing something wrong somewhere. Ivan, would you be able to test this patch on Windows with the latest Clojure master to see if all tests pass for you now?

Comment by Ivan Kozik [ 29/Sep/12 2:59 PM ]

All tests pass on Windows 7 here with the patch.

Ant can't find my test.generative either because it isn't in my "${maven.test.classpath}". I put it in CLASSPATH and modified my build.xml like this:

<java classname="clojure.main" failonerror="false" fork="true">
<classpath>
+ <pathelement path="${java.class.path}"/>
<pathelement path="${maven.test.classpath}"/>

Comment by Andy Fingerhut [ 10/Dec/12 1:33 PM ]

Just as a rough idea of how often people are hitting this issue, CLJ-1123 was opened on Dec 9 2012 and then closed when the ticket creator realized it was a duplicate of this one.

Comment by Mike Anderson [ 18/Jan/13 7:44 PM ]

Hi there is this likely to get fixed soon?

I'd like to help contribute some more patches to Clojure but it's tricky to do when I can't get the build to work

Comment by Andy Fingerhut [ 18/Jan/13 8:39 PM ]

I do not know if or when this patch will be committed to Clojure.

I can tell you that you can apply the patch to your own local copy of the Clojure source code, and then develop new Clojure patches based upon that version. The patch that fixes this problem only affects one test file, so it is unlikely to conflict with any changes you develop and submit.

Comment by Mike Anderson [ 21/Jan/13 6:36 AM ]

I can confirm this patch works fine for me on Windows with Maven/Eclipse

Suggest this patch gets pushed through approval and applied ASAP? It's a pretty obvious fix that is breaking the build....

Comment by Stuart Halloway [ 01/Mar/13 12:44 PM ]

This patch is sloppy – it makes unnecessary whitespace changes in several places.

Would it be better to make the tests trailing whitespace agnostic? Otherwise this feels like poking and prodding until the build box is happy.

Comment by Andy Fingerhut [ 02/Mar/13 2:50 PM ]

Patch clj-1076-fix-tests-on-windows-patch-v2.txt dated Mar 2, 2013 fixes pprint tests on Windows in a different way: Removing all occurrences of carriage return (\r) characters in the output of pprint before comparing it to the expected string.

I tried simply doing str/trim-newline to remove newlines and carriage returns at the end of the string, but that does not make the tests pass. They still fail due to carriage returns in the middle of the string.

Comment by Andy Fingerhut [ 02/Mar/13 2:51 PM ]

Presumptuously changing Approval from Incomplete back to None, since there is a new patch attached that should address the reason it was marked Incomplete.

Comment by Alex Miller [ 02/Sep/13 7:37 PM ]

Is this still happening on Windows?

Comment by Andy Fingerhut [ 02/Sep/13 8:27 PM ]

Yes, the latest version of Clojure still fails tests for the same reason as reported in this ticket, and both of the patches still allow the tests to pass on Windows as well as Linux and OS X.

I have thought of yet another way to allow the tests to pass on all of these platforms, which is to call clojure.string/split-lines on the multi-line strings to be compared, and then compare the resulting sequences of single-line strings for equality. That is just a slightly different way of eliminating the line terminator difference between platforms. Let me know if you would prefer a patch like that over the patch clj-1076-fix-tests-on-windows-patch-v2.txt

Comment by Alex Miller [ 02/Sep/13 10:44 PM ]

Marking triaged.

Comment by Alex Miller [ 13/Sep/13 8:50 AM ]

Andy, what if we changed the current patch to replace "\r\n" -> "\n" instead of "\r" -> ""? That way we wouldn't be accidentally removing \r inside lines (which admittedly would be very unusual). I'd be good to go with that change (unless there is a good reason not to do that).

Comment by Andy Fingerhut [ 13/Sep/13 9:16 AM ]

clj-1076-v3.txt is like the other patches, except it uses split-lines on the two multi-line strings to be compared in the test, and compares the result sequences of strings. split-lines splits strings on the regex #"\r?\n", so only where there are newline characters, and optionally a carriage return preceding the newline.





[CLJ-1072] Replace old metadata reader macro syntax Created: 21/Sep/12  Updated: 24/May/13  Resolved: 24/May/13

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

Type: Enhancement Priority: Trivial
Reporter: Sam Aaron Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-CLJ-1072-Replace-old-metadata-reader-macro-syntax.patch    
Patch: Code
Approval: Ok

 Description   

5 files still have old metadata reader syntax hash-caret instead of just caret:

src/clj/clojure/core.clj
src/clj/clojure/gvec.clj
src/clj/clojure/java/browse_ui.clj
src/clj/clojure/java/io.clj
src/clj/clojure/repl.clj



 Comments   
Comment by Stuart Sierra [ 21/Sep/12 7:56 AM ]

Modified this ticket to cover all remaining cases of old metadata syntax. Added patch.





[CLJ-1058] StackOverflowError on exception in reducef for PersistentHashMap fold Created: 02/Sep/12  Updated: 07/Feb/14  Resolved: 07/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: Tom Jack Assignee: Bruce Adams
Resolution: Completed Votes: 1
Labels: reducers
Environment:

Clojure 1.5.0-alpha4, Sun Java 1.6.0_35, with [org.codehaus.jsr166-mirror/jsr166y "1.7.0"]


Attachments: File sneakyThrow-clj-1058.diff    
Patch: Code and Test
Approval: Ok

 Description   

If reducef throws an exception, the exception is swallowed sometimes (that is: not propagated up to the caller of r/fold). This can lead to infinite recursion, causing a stack overflow which masks the problem.

user> (require '[clojure.core.reducers :as r])
nil
user> (r/fold (fn ([]) ([ret k v] (+ 3 "foo") ret)) (into {} (map (juxt identity identity) (range 10000))))
StackOverflowError   clojure.lang.Numbers.add (Numbers.java:126)

This results in a stack like: https://raw.github.com/gist/3bab917287a7fd635a84/f38bfe3e270556e467f3fc02062af7ea10781390/gistfile1.txt

Cause: The problem area in the code catches Exception and swallows it - this is commented "aargh" in PersistentHashMap.java line 444 (as of 412a51d).
https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/PersistentHashMap.java#L444

Approach: Throw the Exception and allow it to propagate instead of swallowing it.

Patch: sneakyThrow-clj-1058.diff

Screened by: Alex Miller



 Comments   
Comment by Timothy Baldridge [ 30/Nov/12 3:40 PM ]

Verified as a bug.

Comment by Dave Sann [ 30/Jul/13 3:41 AM ]

Some comments here:

https://groups.google.com/d/msg/clojure-dev/kwJEw9YjeGc/VKxlixGVnMIJ

in short
//aargh

can be replaced by

throw new RuntimeException(e);

or maybe: Util.sneakyThrow(e);

which will surface the exception rather than swallow it.

I have questions about clean-up - in the thread - I am not very familiar with the fork join framework.

Comment by Bruce Adams [ 17/Sep/13 7:12 AM ]

Don't ignore exceptions thrown by tasks

Comment by Bruce Adams [ 18/Sep/13 11:24 AM ]

This change removes the problematic
try ... catch
and enhances "foldTasks" and "fold" method declarations by adding "throws Exception".

Comment by Andy Fingerhut [ 26/Sep/13 9:28 PM ]

Bruce, others can comment better than I can on this comment, but a large number of places in the Clojure implementation avoid the need for declaring checked exceptions by using Util.sneakyThrow. Search for that in the Java code for examples, and how it is implemented if you are curious. It might be that this is another place this should be done.

Comment by Bruce Adams [ 28/Sep/13 8:38 PM ]

I don't yet know the Clojure code base very well. Doing a quick count, there are slightly more methods with a declared "throws" clause then there are "sneakyThrows" calls. I am assuming (based on other Java experience) that sneakyThrows is only used when declaring a throws clause is problematic. In my proposed fix, throws declarations are needed only on four methods, all in the same module. The actual exceptions appear to be dealt with cleanly.

What I haven't yet figured out is how to write relevant tests for this. The clearly broken thing about the existing code is falling out of the if count==1 block into code that assumes count>1. Even just returning a null at least avoids the infinite recursion problem (and also swallows the exception, which seems wrong). So a test for once-and-only-once seems relevant. Also, a test for exception behavior seems called for.

(Also, I don't know protocol for assigning bugs. I'm tempted to hit the "Assign To Me" button in JIRA, but I'm not sure what it means in the larger scheme of things.)

Comment by Alex Miller [ 11/Oct/13 4:21 PM ]

"Assign to Me" means that you are currently working on something for that ticket and others should probably check with you before investing time. From my perspective, anyone is welcome to assign to themselves with the caveat that someone else may still create a patch and/or interpret non-progress as an invitation to work on it.

Is this ticket in a state where it should be screened or do you feel it is incomplete? I am trying to judge whether I should invest time in screening it.

Comment by Bruce Adams [ 11/Oct/13 4:42 PM ]

I believe that declare-throws-clj-1058.diff correctly deals with the problem. It certainly behaves better than code it replaces. I can imagine you screening, and then Rich accepting, it as is.

However, I think we will all be happier with tests demonstrating behavior. I've been struggling with writing tests, especially tests that cleanly fail on the old code, instead of hard crashing the test suite with a stack overflow.

Comment by Bruce Adams [ 17/Oct/13 1:21 PM ]

The code fix in "patch-with-tests-clj-1058.diff" is identical to my earlier "declare-throws-clj-1058.diff" This new patch includes an additional commit adding a unit test.

This is ready for screening.

Comment by Alex Miller [ 17/Dec/13 11:05 PM ]

I don't think we want to throw Exception out of (at least) fold() and possibly the others as well. Can you rework with sneakyThrow?

Comment by Bruce Adams [ 18/Dec/13 9:21 PM ]

I can use sneakyThrow. The patch I uploaded in September 17 did exactly that. I have since deleted that patch.

I've now had two people I respect, you and Andy Fingerhut, suggest a need for using sneakyThrow in this code. Each of you know the Clojure code far better than I do. I'm very inclined to trust your intuition that sneakyThrow is the right thing here, but I would really like to understand why. In my testing, checked exceptions (non-RuntimeExceptions) out of this code already get wrapped in a RuntimeException. I assume this wrapping happens higher up the Java stack and is implemented via sneakyThrow. Why do we want to add yet another layer of exception wrapping? It adds complexity to this Java code.

I am happy to add tests demonstrating the existing checked exception wrapping behavior.

Can you help me understand why we want sneakyThrow here? I would really appreciate it.

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

Most of the internal Clojure code eschews checked exceptions (they are an artifact of Java, not a JVM thing) and I think it's unlikely that Rich would accept a patch that explicitly declared it in this case. My expectation is that wrapping the exception at the beginning via sneakyThrow will prevent it from being wrapped again farther up, so I don't know that this change will nest things any more deeply.

Comment by Bruce Adams [ 11/Jan/14 10:57 AM ]

My earlier patch had an extra, unneeded "throws Exception" declaration on a publicly visible "fold" method. This new patch, declarative-clj-1058.diff, only adds "throws Exception" to methods used within src/jvm/clojure/lang/PersistentHashMap.java.

To make this locality clear, I've attached another patch, private-interface-INode.diff, which declares the internal INode interface as "private". I don't think the "private" declaration needs to go into the Clojure code.

These patches do not taking your advice to use sneakyThrow here. Using sneakyThrow still feels wrong to me. I will (attempt to) start a conversation on the Clojure-dev mailing asking people for thoughts about the two approaches.

Comment by Bruce Adams [ 13/Jan/14 5:16 AM ]

I forgot to thank Alex for expressing concerns with my earlier patchset. That got me to think harder about what exactly is happening here and to learn more about the Java code implementing Clojure. I enjoy thinking and learning.

Thank you, Alex!

Comment by Bruce Adams [ 14/Jan/14 5:49 PM ]

Thanks to Kevin Downey on the Clojure-dev mailing list, I now understand sneakyThrow better.

This patch uses sneakyThrow to surface exceptions.

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

Switching to Vetted so this is available to be screened.

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

Bruce, what's with the k-fail and def stuff in the tests?

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

Moving back to Incomplete - I think the tests should be simplified - I don't see why all the internal def stuff is necessary to demonstrate this.

Comment by Bruce Adams [ 17/Jan/14 11:09 AM ]

Yes, I agree. I'll update the patch with simpler tests. It'll take a few days.

Comment by Bruce Adams [ 05/Feb/14 8:10 PM ]

Same fix as before, using sneakyThrow, but with a single, much simpler test.





[CLJ-1056] defprotocol: invalid method overload syntax getting accepted Created: 01/Sep/12  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: Víctor M. Valenzuela Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: errormsgs, protocols

Attachments: Text File clj-1056-1.txt     File clj-1056-2.diff     Text File clj-1056-2.txt    
Patch: Code and Test
Approval: Ok

 Description   

The compiler accepts this erroneous form:

user=> (defprotocol Bar (m [this]) (m [this arg]))
Bar

Analysis: defprotocol silently assoc's the last list of signatures found for any particular method name, without checking whether the method name was given earlier.

Patch: clj-1056-2.diff

Approach: Modify defprotocol to check whether each method name has already been encountered earlier, and throw an exception if so. The patch also updates the error message for the case of a protocol function with no args specified.

Behavior with patch clj-1056-2.txt:

user=> (defprotocol Bar (m [this]) (m [this arg]))
IllegalArgumentException Function m in protocol Bar was redefined. Specify all arities in single definition.
user=> (defprotocol Foo (m []))
IllegalArgumentException Definition of function m in protocol Foo must take at least one arg.

Screened by: Stuart Halloway



 Comments   
Comment by Timothy Baldridge [ 29/Nov/12 4:02 PM ]

Can not reproduce the fist error:

user=> (defprotocol Foo (f ([this]) ([this arg])))
CompilerException java.lang.IllegalArgumentException: Parameter declaration missing, compiling:(NO_SOURCE_PATH:5:1)

But the 2nd one I can reproduce:

user=> (defprotocol Bar (m [this]) (m [this arg]))
Bar
user=> Bar
{:on-interface user.Bar, :on user.Bar, :sigs {:m {:doc nil, :arglists ([this arg]), :name m}}, :var #'user/Bar, :method-map {:m :m}, :method-builders {#'user/m #<user$eval71$fn_72 user$eval71$fn_72@1a2b53fb>}}
user=>

Notice that :arglists only has one entry

Vetting

Comment by Alex Miller [ 22/Aug/13 10:36 PM ]

Moving back to Triaged as Rich has not vetted.

Comment by Andy Fingerhut [ 10/Sep/13 11:24 PM ]

Patch clj-1056-1.txt changes defprotocol to throw an exception if the same method name appears more than once.

Without this patch, the last set of signatures for a method name "wins", silently overriding any earlier ones.

Comment by Alex Miller [ 18/Oct/13 11:08 AM ]

Updated patch to improve error messages and to switch from CompilerException (which is not used outside the Compiler) back to IllegalArgumentException.

I think it would be a good idea to have a general Exception that could carry file/line/col info that could be used by Compiler but also other errors (the EdnReader and LispReader both have their own variants). But I think that should be a separate enhancement, which I have filed as CLJ-1280.





[CLJ-1053] Locals still cleared too aggressively on delay in specific cases Created: 30/Aug/12  Updated: 03/Sep/13  Resolved: 03/Sep/13

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

Type: Defect Priority: Minor
Reporter: Jean Niklas L'orange Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

This seems to be an extension of #CLJ-232. Locals are still cleared too aggressively in some specific instances: If the delay throws an exception when realized, and you dereference a local within the delay, the local may or may not be cleared depending on its need in the tail positions (without any obvious pattern). Examples of functions which works as intended and doesn't work as intended follows.

(def d (let [y (delay 0)]
         (delay (if (zero? @y) (/ 0 0) (/ @y 1))))) ; works properly

(def d (let [y (delay 0)]
         (delay (if (zero? @y) (/ 0 0) (/ 1 1))))) ; does not work properly

(def d (let [y (delay 0)]
         (delay (if (zero? @y) (/ @y 0) (/ 1 1))))) ; does not work properly

(def d (let [y (delay 0)]
         (delay (if (zero? @y) (/ @y 0) (/ @y 1))))) ; does not work properly

(def d (let [y (delay 0)]
         (delay @y (/ 0 0)))) ; does not work properly

(def d (let [y (delay 0)]
         (delay @y (/ 0 0) @y))) ; works properly

By "works", d will throw "ArithmeticException Divide by zero" every single time it is dereferenced. By "does not work", d will throw "ArithmeticException Divide by zero" on first dereferencing, but from second and onwards it will throw "NullPointerException [trace missing]".



 Comments   
Comment by Jean Niklas L'orange [ 09/Apr/13 11:55 AM ]

With Clojure 1.5.1, the trace is given and returns NullPointerException clojure.core/deref-future (core.clj:2NullPointerException clojure.core/deref-future (core.clj:2108)108), which refers to a .get call. The stacktrace reveals the following:

NullPointerException 
	clojure.core/deref-future (core.clj:2108)
	clojure.core/deref (core.clj:2129)
	user/fn--1/fn--4 (NO_SOURCE_FILE:2)
	clojure.lang.Delay.deref (Delay.java:33)
	clojure.core/deref (core.clj:2128)
	user/eval13 (NO_SOURCE_FILE:-1)
	clojure.lang.Compiler.eval (Compiler.java:6619)
	clojure.lang.Compiler.eval (Compiler.java:6582)
	clojure.core/eval (core.clj:2852)
	clojure.main/repl/read-eval-print--6588/fn--6591 (main.clj:259)
	clojure.main/repl/read-eval-print--6588 (main.clj:259)
	clojure.main/repl/fn--6597 (main.clj:277)
Comment by Alex Miller [ 03/Sep/13 12:17 PM ]

This was fixed by CLJ-1175 which is now in 1.6.0-master. All of the given examples now work there.





[CLJ-1018] range's behavior is inconsistent Created: 29/Jun/12  Updated: 24/May/13  Resolved: 24/May/13

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

Type: Defect Priority: Minor
Reporter: Devin Walters Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch, range

Attachments: File inconsistent_range_fix.diff    
Patch: Code and Test
Approval: Ok

 Description   

Problem statement: The current behavior of range is inconsistent. (range 0 9 0) has always produced (). (range 0 9 -1) has always produced (). (range 9 0 1) has always produced (). However, (range 9 0 0) produces (9 9 9 9 ...), and (range 0 0 0) produces '(0 0 0 0 ...)

Proposal: Make the behavior of range consistent when using a step of 0 to make it produce an empty list.

Please see attached code and patch.



 Comments   
Comment by Mike Anderson [ 01/Jul/12 4:08 AM ]

Agree it is good to fix the inconsistency, but I think an infinite sequence of zeros is the correct result, as implied by the current docstring definition.

It's also mathematically cleanest: range should produce an arithmetic progression until the end value is equalled or exceeded.

Empty lists only seem to make sense as a return value when start >= end.

Comment by Devin Walters [ 01/Jul/12 12:36 PM ]

Hi Mike,

Could you explain how you think the docstring definition implies this behavior? Maybe I'm missing something, but I was surprised. For instance, in the case of (range 3 9 0), if start were truly inclusive as the docstring says, the result would be (3), not ().

You mentioned that you think the infinite sequence of 0's is consistent and in keeping with the definition of range. I'm not sure I agree. (0,0] is an empty set of length zero, and [0,0) is an empty set of length zero.

You state that empty list only makes sense for start >= end, except this is not the current behavior. Could you help me understand what you believe the appropriate behavior would be in each of the following three cases? (range 0 10 0), (range 10 0 0), and (range 0 0 0)?

A few options to consider:
1) Fix the docstring to reflect the actual behavior of range.
2) Handle the case of (range 9 3 0) => (9 9 9 ...) to make it consistent with the behavior of (range 3 9 0) => ()
3) Stop allowing a step of zero altogether.

Editing to Add (Note: I don't think the way someone else did something is always the right way, just wanted to do some digging on how other people have handled this in the past):
http://docs.python.org/library/functions.html#range (0 step returns ValueError)
http://www2.tcl.tk/10797 (range returns empty list for a zero step)
http://www.scala-lang.org/api/2.7.7/scala/Range.html (zero step is not allowed)

Comment by Dimitrios Piliouras [ 05/Jul/12 4:13 PM ]

It does make sense NOT to allow a step of zero (at least to me)... I wasn't going to say anything about this but if other popular languages do not allow 0, then I guess it makes more sense than I originally gave myself credit for... However, if we want to be mathematically correct then the answer would be to return an infinte seq with the start value like below. After all, what is a step of 0? does it make any difference how many steps you take if with every step you cover 0 distance? obviously not...you start from x and you stay at x forever...we do have repeat for this sort of thing though...

(take 5 (range 3 9 0)) => (3 3 3 3 3)

+1 for not allowing 0 step

Comment by Mike Anderson [ 08/Jul/12 8:49 AM ]

@Devin quite simple: a lazy sequence of nums starting from x with step 0.0 until it reaches an end value of y (where y > x) is an infinite sequence of x.

Consider the case where start is 0 and end is infinity (the default), you would expect sequences to go as follows:

step +2 : 0 2 4 6 8....
step +1 : 0 1 2 3 4....
step +0 : 0 0 0 0 0....

It would be inconsistent to make 0 a special case, all of these are valid arithmetic progressions. And all of these are consistent with the current docstring.

If you make 0 a special case, then people will need to write special case code to handle it. Consider the code to create a multiplication table for example:

(for [x (range 10)]
(take 10 (range 0 Long/MAX_VALUE x)))

This works fine if you produce an infinite sequence of zeros for step 0, but fails if you create an empty list as a special case for step 0.

As a related issue, I'd personally also favour negative step sizes also producing an infinite sequence. If we don't want to allow this though, then at least the docstring should be changed to say "a lazy seq of non-decreasing nums" and a negative step should throw an error.

Comment by Devin Walters [ 09/Jul/12 7:09 PM ]

Carrying over a message from the clojure-dev list by Stuart Sierra:

  • I called the ticket a defect. Does that seem reasonable?

yes

  • Does anyone actually use the 0 step behavior in their programs?

not if they have any sense

  • Has anyone been bitten by this in the past?

not me

  • Is this behavior intentional for some historical reason I don't know about or understand?

I doubt it.

  • Has this been brought up before? I couldn't find any reference to it via Google.

Not that I know of.

  • Are there performance implications to my patch?

I doubt it. Lazy seqs are not ideal for high-performance code anyway.

  • Am I addressing a symptom rather than the problem?

I think the problem is that the result of `range` with a step of 0 was never specified. Don't assume that the tests are specifications. Many of the tests in Clojure were written by over-eager volunteers who defined the tests based on whatever the behavior happened to be. The only specification from the language designer is the docstring. By my reading, that means that `range` with a step of 0 should return an infinite seq of the start value, unless the start and end values are equal.

-S

Comment by Devin Walters [ 09/Jul/12 7:10 PM ]

Carrying over a message by Michal Marczyk:

With a negative step, the comparison is reversed, so that e.g.

(range 3 0 -1)

evaluates to

(3 2 1)

I think this is the useful behaviour; the docstring should perhaps be
adjusted to match it.

Agreed on zero step.

Cheers,
Michał

Comment by Devin Walters [ 20/Jul/12 5:10 PM ]

Adding a new patch after hearing comments. This patch makes (range 9 3 0) => (9 9 9 9 ...), (range 3 9 0) => (3 3 3 3 ...), and () will be returned when (= start end). Also updated the docstring.

Comment by Timothy Baldridge [ 27/Nov/12 3:01 PM ]

Marking as vetted

Comment by Timothy Baldridge [ 27/Nov/12 3:04 PM ]

Patch applies cleanly. We've discussed this issue to death (for as simple as it is). I think it's time to mark it as screened.

Comment by Timothy Baldridge [ 27/Nov/12 3:06 PM ]

For some reason I'm not allowed to edit the attachments list. When you apply the patch, please apply inconsistent_range_fix.diff as that is the most recent version of the fix.

Comment by Rich Hickey [ 09/Dec/12 6:44 AM ]

As someone who has to read these tickets, I'd really appreciate it if you could keep the description up to date and accurate, with examples of the current and intended behavior and the strategy to be used in fixing. I can't follow every thread to see what the latest thinking is, especially on a patch like this where the original mission was changed.

Thanks

Comment by Devin Walters [ 10/Dec/12 3:53 PM ]

@Tim: I've removed the other attachments.

@Rich: Understood. I will update the description this evening.





[CLJ-983] proxy-super does not restore original binding if call throws exception Created: 05/May/12  Updated: 31/Jan/14  Resolved: 31/Jan/14

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

Type: Defect Priority: Major
Reporter: Valentin Mahrwald Assignee: Unassigned
Resolution: Completed Votes: 4
Labels: interop

Attachments: Text File clj-983-2.patch     Text File proxy_super.patch    
Patch: Code and Test
Approval: Ok

 Description   

The code for proxy-call-with-super internally used by proxy-super does not handle exceptions from the call method:

(defn proxy-call-with-super [call this meth]
 (let [m (proxy-mappings this)]
    (update-proxy this (assoc m meth nil))
    (let [ret (call)]
      (update-proxy this m)
      ret)))

As a result the following sequence behaves unexpectedly:

(def obj (proxy [java.lang.ClassLoader] []
		      (loadClass [cl] (println "enter")
				 (proxy-super loadClass cl))))
(.loadClass obj "nonexistent")
;; prints enter, then throws ClassNotFoundException
(.loadClass obj "nonexistent")
;; does not print anything before throwing cnfe

Patch: clj-983-2.patch

Approach: Use a try-finally block around the invocation to perform the after invocation action regardless of exceptions.

Screened by: Stuart Sierra



 Comments   
Comment by Valentin Mahrwald [ 05/May/12 9:04 AM ]

Test & fix patch on latest Clojure github branch.

Comment by Alex Miller [ 26/Dec/13 7:24 AM ]

Patch provider does not have a signed CA so can't be accepted as is.

Comment by Alex Miller [ 03/Jan/14 12:33 PM ]

Marked vetted in 1.6 per Rich.

Comment by Alex Miller [ 06/Jan/14 1:07 PM ]

Incomplete - needs clean patch.

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

Added new clean patch for screening consideration.

Comment by Stuart Sierra [ 10/Jan/14 4:22 PM ]

Screened OK.





[CLJ-980] Documentation for extend-type falsely implies that & is allowed in protocol fn signatures Created: 03/May/12  Updated: 18/Oct/13  Resolved: 18/Oct/13

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

Type: Defect Priority: Trivial
Reporter: Charles Duffy Assignee: Stuart Halloway
Resolution: Declined Votes: 0
Labels: docstring

Attachments: File clojure--extend-type-doc-fix.diff     Text File extended-type-doc-fix-v2.patch    
Patch: Code
Approval: Vetted

 Description   

The documentation for extend-type contains the following example:

(extend-type MyType 
    Countable
      (cnt [c] ...)
    Foo
      (bar [x y] ...)
      (baz ([x] ...) ([x y & zs] ...)))

However, [x y & zs] is not a valid parameter list for a protocol fn. The documentation should be appropriately amended.



 Comments   
Comment by John Szakmeister [ 25/May/12 4:00 AM ]

v2 of the patch is simply converts the patch to git format. I made sure that Mr. Duffy got proper attribution. I also took a stab at a simple log message. Any problems with the latter are mine, and I'm happy to fix it up if necessary.

Comment by Aaron Bedra [ 15/Aug/12 10:08 PM ]

This doesn't update the entire doc string. The expansion is updated, but the signature section isn't. Here's what shows up.

user=> (doc extend-type)
-------------------------
clojure.core/extend-type
([t & specs])
Macro
  A macro that expands into an extend call. Useful when you are
  supplying the definitions explicitly inline, extend-type
  automatically creates the maps required by extend.  Propagates the
  class as a type hint on the first argument of all fns.

  (extend-type MyType 
    Countable
      (cnt [c] ...)
    Foo
      (bar [x y] ...)
      (baz ([x] ...) ([x y & zs] ...)))

  expands into:

  (extend MyType
   Countable
     {:cnt (fn [c] ...)}
   Foo
     {:baz (fn ([x] ...) ([x y ...] ...))
      :bar (fn [x y] ...)})
Comment by Tassilo Horn [ 25/Aug/12 6:44 AM ]

This is very much related to http://dev.clojure.org/jira/browse/CLJ-1024.

NOTE: While varargs are not supported in protocol declarations, dynamic extension of a protocol via extend (extend-type, extend-protocol) does allow for varargs and also destructuring, cause the method impls are actually normal clojure functions.

So if a Foo protocol declares a (foo [this a b c]) method, you can extend/extend-type/extend-protocol it dynamically using (foo [this & more] (do-magick)) where `a b c` are conflated into the `more` parameter.

However, that doesn't work for method implementations defined by deftype, defrecord, and reify.

So with respect to the facts, the example in the docstring is actually correct, so I'm not sure if it should be changed. However, what's supported in which cases should be documented better as it is right now.

Comment by Alex Miller [ 28/Jul/13 10:13 PM ]

Putting back in triage to go through release assignment again.

Comment by Stuart Halloway [ 18/Oct/13 9:31 AM ]

I agree with Tassilo's comment, the documented behavior works. You can't use varargs in defprotocol but you can use them in extend-type.





[CLJ-949] let undeclared exceptions continue unchecked Created: 07/Mar/12  Updated: 22/Nov/13  Resolved: 22/Nov/13

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

Type: Enhancement Priority: Minor
Reporter: Brian Taylor Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-let-undeclared-exceptions-continue-unchecked.patch     File clj949-patch-v2.diff     Text File clj949-patch-v2-ignore-whitespace.txt     Text File clj949-patch-v2.txt    
Patch: Code
Approval: Ok

 Description   

The recent modifications regarding checked exceptions have eliminated the need for several try/catch blocks. This commit removes the blocks that no longer serve a purpose.

Patch: clj949-patch-v2.diff

The attachment clj949-patch-v2-ignore-whitespace.txt is not the intended patch. It was created to be slightly easier to read and review that clj949-patch-v2.txt. It was created by ignoring white space changes using 'git diff -w ...'



 Comments   
Comment by Andy Fingerhut [ 09/Mar/12 9:06 AM ]

Patch 0001-let-undeclared-exceptions-continue-unchecked.patch applies cleanly to latest master as of March 9, 2012, and build and test without errors or warnings. One author, Brian Taylor, has signed CA.

Comment by Stuart Sierra [ 02/Aug/13 1:45 PM ]

Marked incomplete. Code cleanup is good, but I don't think this patch includes all the cases where we can remove try/catch. For example, I found these two by searching for "sneakyThrow" and I expect there are more. If we can, I'd like to get all the possible changes in one patch. Thanks, -S

diff --git a/src/jvm/clojure/lang/EdnReader.java b/src/jvm/clojure/lang/EdnReader.java
index 494c0b4..545fba0 100644
--- a/src/jvm/clojure/lang/EdnReader.java
+++ b/src/jvm/clojure/lang/EdnReader.java
@@ -62,12 +62,7 @@ static boolean nonConstituent(int ch){

 static public Object readString(String s, IPersistentMap opts){
        PushbackReader r = new PushbackReader(new java.io.StringReader(s));
-       try {
        return read(r, opts);
-       }
-       catch(Exception e) {
-       throw Util.sneakyThrow(e);
-       }
 }

 static boolean isWhitespace(int ch){
diff --git a/src/jvm/clojure/lang/Ref.java b/src/jvm/clojure/lang/Ref.java
index cf7ffa7..f687575 100644
--- a/src/jvm/clojure/lang/Ref.java
+++ b/src/jvm/clojure/lang/Ref.java
@@ -241,14 +241,7 @@ public Object call() {
 }

 public void run(){
-       try
-               {
-               invoke();
-               }
-       catch(Exception e)
-               {
-               throw Util.sneakyThrow(e);
-               }
+       invoke();
 }

 public Object invoke() {
Comment by Andy Fingerhut [ 30/Aug/13 8:59 PM ]

Patch clj949-patch-v2.txt dated Aug 30 2013 contains 2 commits. The first commit is the same as Brian Taylor's 0001-let-undeclared-exceptions-continue-unchecked.patch. The second commit removes all remaining occurrences of "throw sneakyThrow(e)" that can be removed without causing Java compilation errors.

clj949-patch-v2-ignore-whitespace.txt is nearly identical, and is included only for ease of review. It was produced with no lines containing only whitespace changes, using the command below, since there are many lines that were in try blocks that were only changed by removing a leading tab character:

git format-patch -w master --stdout

Comment by Alex Miller [ 02/Sep/13 11:41 PM ]

clj949-patch-v2.txt had warnings when I applied it:

$ git am --keep-cr -s --ignore-whitespace < ~/Downloads/clj949-patch-v2.txt
Applying: let undeclared exceptions continue unchecked
Applying: CLJ-949: let more undeclared exceptions continue unchecked
/Users/alex/code/clojure/.git/rebase-apply/patch:189: trailing whitespace.
					gen.visitInsn(I2L);
/Users/alex/code/clojure/.git/rebase-apply/patch:194: trailing whitespace.
					gen.visitInsn(F2D);
/Users/alex/code/clojure/.git/rebase-apply/patch:204: trailing whitespace.
					gen.visitInsn(D2F);
warning: 3 lines add whitespace errors.
Comment by Andy Fingerhut [ 03/Sep/13 12:12 AM ]

I have attached a slightly modified version of clj949-patch-v2.txt that should not have those warnings when applied.

FYI, the warnings were because I removed one leading tab from some Java source lines that had several tabs after the code on that line, just before the newline. Those tabs were there in the original Compiler.java lines being modified. git doesn't have any problem with a deleted line having such trailing whitespace, but it warns about adding lines with trailing whitespace.

Comment by Andy Fingerhut [ 03/Sep/13 1:34 AM ]

Alex, do you know whether you will want all screened patches to be free of trailing whitespace warnings, and other such whitespace warnings?

If so, I can try to be proactive in finding and fixing such patches before you come across them. It is a small change to my code for checking whether patches are prescreened, to also check whether they have these warnings when the patch is applied.

Alternately, it is also easy to disable trailing whitespace warnings in git. If you think it is OK for patches to add lines with trailing whitespace, I can document that on the wiki.

Comment by Alex Miller [ 03/Sep/13 7:50 AM ]

Honestly, I can't say I understood all the details of what the warning meant. Seeing your explanation, it doesn't seem like a big deal but it certainly seems better to clean up the whitespace as we patch. Warnings are something that slows down evaluation of the patch b/c you have to understand whether the warning is ok, so in general I'd prefer not to see them.

Comment by Andy Fingerhut [ 04/Sep/13 6:40 AM ]

Got it, Alex. My program for prescreening patches now includes some output that tells me which patches have warnings when they are applied with the "git am ..." command. For each of those, it gives a different status depending upon how far the ticket is in the workflow: "fix now" for patches on tickets that are already Vetted and scheduled for the next release, "fix soon" for tickets that are only Vetted, and "fix later" for all pre-Vetted tickets.

Thus even though there are a couple dozen patches with these warnings now, almost all of them are for pre-Vetted tickets, so not urgent. I can be reminded of the patches needing cleaning-up gradually, as they are Vetted or I have spare time.

Comment by Alex Miller [ 27/Sep/13 5:14 PM ]

Moving to Vetted for re-screening.

Comment by Stuart Sierra [ 11/Oct/13 6:27 PM ]

Screened.





[CLJ-944] Compiler sometimes gives constant collections types which mismatch with their runtime values Created: 04/Mar/12  Updated: 06/Mar/14  Resolved: 06/Mar/14

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

Type: Defect Priority: Major
Reporter: Alf Kristian Støyle Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: compiler
Environment:

Clojure 1.3 om Mac OS 10.7, Clojure 1.5.0 alpha1 on Linux x86_64 (OpenJDK 1.7.0 b147)


Attachments: Text File 0001-Fix-for-CLJ-944.patch     Text File 0002-Fix-for-CLJ-944.patch     Text File clj944-plus-tests.patch    
Patch: Code and Test
Approval: Ok

 Description   
(.containsKey {:one 1} :one)
;=> ClassCastException clojure.lang.PersistentArrayMap cannot be cast to clojure.lang.PersistentHashMap

The map is a clojure.lang.PersistentArrayMap, which obviously has a containsKey method (https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/PersistentArrayMap.java#L95).

Casting it works fine though:

(.containsKey ^clojure.lang.PersistentArrayMap {:one 1} :one)
;=> true

The problem is not present in Clojure 1.2.1.

Cause: In the first example, the Compiler will parse the MapExpr (which reports a Java class of IPersistentMap) into a ConstantExpr whose value is a PersistentHashMap (and reports type as same). The .containsKey() call is a HostExpr which parses to an InstanceMethodExpr.

When emitted to bytecode, the ConstantExpr becomes a call to RT.map() to generate a map. RT.map() produces either a PersistentArrayMap or PersistentHashMap depending on size. The InstanceMethodExpr becomes a call to PersistentHashMap.containsKey() (based on the reported type of it's expression). In the case where RT.map() will produce a PersistentArrayMap at runtime (small maps), this causes a mismatch.

Note also that this same scenario occurs for constant sets, vectors, etc - all of those cases produce ConstantExpr's with the type of the concrete data structure, instead of the more general IPersistentMap, IPersistentSet, etc. All of those scenarios should be addressed in the same way. But, those interfaces do not capture the full capabilities of the returned object. The compiler and the marshaller need to agree on the types to produce.

Approach: One approach (A) explored previously was to have MapExpr always produce a ConstantExpr containing a value of the same type that will be produced at runtime by RT.map(). While this works, it is also fragile, and sidesteps the real issue that MapExpr creates a ConstantExpr with a type that is too specific. Rich said this is the wrong approach.

(B) Another approach is to create an anonymous subclass of ConstantExpr in MapExpr that overrides getJavaClass() to report a more general type. This isn't done anywhere else and it may have negative ramifications (but I have not found any).

(C) Another approach is to follow the path of example #2 above. That example wraps the map in a MetaExpr which reports getJavaClass() returning the hinted type. In the case above, it hints the concrete type that happens to match the return of RT.map(). Instead, we could wrap the ConstantExpr in a MetaExpr hinted on the abstract type IPersistentMap (the return type of RT.map()).

RH - a more general (interface) type will not give full capabilities. Perhaps APersistent*

Patch:



 Comments   
Comment by Nicola Mometto [ 30/Oct/12 5:02 PM ]

The attached patch fixes the issue, by emitting IPersistentMap instead of Persistent{Hash|Array}Map as class type for maps literals

Comment by Nicola Mometto [ 01/Nov/12 3:48 PM ]

I uploaded another patch fixing the same problem in a different way.
While 0001-Fix-for-CLJ-944.patch makes clojure.lang.Complier.ConstantExpr#getJavaClass return clojure.lang.IPersistentMap for both clojure.lang.PersistentHashMap and clojure.lang.PersistentArrayMap, 0002-Fix-for-CLJ-944.patch makes clojure.lang.Compiler.MapExpr#parse return a PersistentArrayMap if the length is <= HASHTABLE_THRESHOLD, instead of always returning a PersistentHashMap.

This approach is more consistent, making the type of the compiler's internal representation of a map literal equal to the one of the reader.

Note that this second approach while being more consistent, breaks some tests that assume some operations on maps (specifically `seq` and `print`) to be order dependent, and written with the hash-map return order implementation in mind.

That should not be the case and if the second patch is preferred over the first one, I'll gladly fix those tests.

Comment by Stuart Halloway [ 01/Mar/13 12:09 PM ]

Approach #2, relying on consistent choice of concrete map class by size throughout, feels quite fragile.

Approach #1 seems to abuse the method name getJavaClass(), now having it return "get the base type I would need for cast".

Maybe there needs to be a different thing entirely?

Comment by Nicola Mometto [ 01/Mar/13 2:17 PM ]

Patch #2 should get merged (IMHO) regardless of the fragility of its approach to fixing this ticket's bug, since it fixes another bug:

prior to the patch:

user=> (class {:a 1})
clojure.lang.PersistentArrayMap
user=> (def a {:a 1})
#'user/a
user=> (class a)
clojure.lang.PersistentHashMap

after the patch:

user=> (class {:a 1})
clojure.lang.PersistentArrayMap
user=> (def a {:a 1})
#'user/a
user=> (class a)
clojure.lang.PersistentArrayMap

This should also lead to some minor performance enhancement since prior to this moment, every map def'ed would be a HashMap instead of an ArrayMap

So, I think patch #2 should be applied if not for this ticket's bug, at least for the reason stated above.
If somebody has any proposal for making this patch more solid regarding this ticket's bug, any help is welcome

Comment by Rich Hickey [ 13/Apr/13 9:41 AM ]

This should not have passed screening. There are two issues, should be separate. I have no idea what has been screened nor what will be applied should it be approved. There's contention in the discussion but no resolution.

Comment by Nicola Mometto [ 13/Apr/13 12:06 PM ]

I don't think that there are two issues here.
The issue is only one: the compiler doesn't emit maps in a way consistent with what the reader returns and with how the compiler itself uses maps.
The symptoms are two: some interop calls fail, and def'ed vars with a literal map as value never use a PersistentArrayMap.

The underlying cause of those two symtoms is fixed by the patch 002 that i submitted (incorporated in Stu's clj944-plus-tests patch.

Stuart said that this approach feels fragile but the bug is caused by the fact that everywhere else clojure returns a PersistentArrayMap when the element count is <= than the PersistentHashMap threshold, and when emitting maps, it doesn't.

Making clojure emit maps consistently with how clojure does internally everywhere else looks to me like the only solution, and I don't really see how making clojure consistent is a fragile approach.

But again, if somebody can suggest a better solution to this problem, I'll gladly submit another patch.

Comment by Rich Hickey [ 14/Aug/13 8:36 AM ]

"Compiler makes different concrete maps then the reader" is not inherently a problem. But the code posted is a problem:

(.containsKey {:one 1} :one)
;=> ClassCastException clojure.lang.PersistentArrayMap cannot be cast to clojure.lang.PersistentHashMap

i.e. .containsKey isn't working in this scenario. Stating exactly why is the path to a good discussion and fix.

Comment by Nicola Mometto [ 14/Aug/13 10:01 AM ]

I don't know how you're getting that exception.

This patch removes exactly that problem, I just tried it out on a fresh clojure git repo with only that patch applied and it works:
Clojure 1.6.0-master-SNAPSHOT
user=> (.containsKey {:one 1} :one)
true

Comment by Alex Miller [ 16/Aug/13 10:49 AM ]

Nicola, I believe Rich is saying that:

1) the posted original example is showing a real problem
2) the title of the ticket and the suggested cause (compiler makes different concrete maps than the reader) is incorrect and that it is ok for them to make different concrete maps
3) the original error message (.containsKey isn't working) is the path to a correct diagnosis.

Rich is NOT saying that the patch doesn't resolve the error but rather that it resolves it in an undesired way due to a misdiagnosis of cause.

Comment by Nicola Mometto [ 17/Aug/13 7:38 AM ]

I'm going to try to describe as clearly as possible what's the problem and what could be the possible solutions (and thus what made me chose this as the best possible fix)

Problem: .containsKeys does not work on some maps

Diagnosis:
Constant maps are expressed as a ConstantExprs by the compiler; the ConstantExpr#getJavaClass method is implemented as a call to .getClass() on the constant object.

Since MapExpr#parse in case of a constant map always delegated to ConstantExpr a PersistentHashMap, it's clear that .getJavaClass on a ConstantExpr-wrapped map will always return PersistentHashMap.

This means that interop-calls will try to resolve PersistentHashMap methods instead of IPersistentMap (since they call .getJavaClass on the object to determine what to target).

When and why does this bug not manifest?

  • if the map is not constant:
    user=> (.containsKey {:one (Object.)} :one)
    true
  • if the map gets compiled as opposed to being .eval'ed
    usser=> (def a (.containsKey {:one 1} :one))
    #'user/a
    user=> a
    true

This also mean that currently, maps that are hold in a Var or that are AOT compiled are always PHMs and never PAMs (removing a possible optimization)

What are the possible solutions?

  • Don't wrap constant maps in ConstantExprs so we always use MapExpr and remove the problem.
  • Special-case ConstantExpr#getJavaClass to return IPersistentMap for PHMs and PAMs (approach #1)
  • Be consistent in MapExpr#parse and create PAMs when the element number is above the PHM threshold as we do EVERYWHERE but in there. (approach #2)

Of the three possible solutions, the third seems to me the best one.

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

Thanks for this - it was a very useful summary for me as I'm still trying to get my head around all the details of this ticket. Stepping back a bit, I'd say in your diagnosis that the point at which I see something wrong is when MapExpr#parse returns something that depends on a particular implementation type (via getJavaClass).

Idea: what if MapExpr#Expr instead returned a anonymous subclass of ConstantExpr that overrode getJavaClass() to return IPersistentMap in this case?

return new ConstantExpr(m) { 
  public Class getJavaClass() {
    return IPersistentMap.class;
} };

That way, the ConstantExpr only commits to the abstraction. This seems to also fix the problem to me.

Separately, I agree that it may be useful as an optimization to emit a PAM instead of a PHM in this case (maybe even in all constant cases?). But I think we should not rely on two different pieces of code producing the identical map impl - that should be hidden behind the abstraction (IPersistentMap). I think I would like to break that out as an independent ticket though.

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

I see that my suggestion is effectively similar to the first patch on this ticket but (imho) cleaner in that it puts the change at the point where the knowledge exists and can thus be a simpler check.

Comment by Andy Fingerhut [ 08/Nov/13 10:19 AM ]

Patch clj944-plus-tests.patch no longer applies cleanly to latest master, probably due to changed context lines in a test file, but I haven't checked. Not worth updating until/unless there is agreement on what should be changed.

Comment by Nicola Mometto [ 08/Nov/13 1:27 PM ]

I would very like to see this fixed and will happily update my patches, however I'd first like some feedback on the correct approach here before making yet another patch.

Should we go with one of my two approaches or should I implement what Alex suggested?
Each approach fixes this bug, I stated before what's causing this and I have explained my solutions, Alex's one looks reasonable too, can we please get an opinion on what's the best move to get a satisfying fix for this?

Thanks

Comment by Alex Miller [ 08/Nov/13 1:45 PM ]

Sorry it's not visible on the ticket, but I have actually spent several hours looking at this since my last comment but have not reached a final conclusion. I also would really like to see it fixed and have not given up on it yet.

Comment by Andy Fingerhut [ 08/Nov/13 1:53 PM ]

The most direct cause of the exception in the example of how to reproduce it is due to the compiler emitting a type check on a type that is different than the constant emitted.

This is just tossing out ideas, perhaps brain-dead ones, but eliminating that type check would eliminate the exception. Also, changing the emitted type check to be for a more general type than than the constant, e.g. IPersistentMap, should also eliminate the exception.

Comment by Nicola Mometto [ 08/Nov/13 2:27 PM ]

Alex, there's no need to be apologetic, I was in no way trying to imply a lack of interest, just asking for directions

Wrapping up for screeners/Rich:

Here is a diagnosis of the bug: http://dev.clojure.org/jira/browse/CLJ-944?focusedCommentId=31694&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-31694

To me now it looks like the best solutions right now are either:

  • My #2 solution: making MapExpr#parse return a PAM or a PHM consistently with RT/map
  • Alex's solution: making MapExpr#Expr return an anonymous subclass of ConstantExpr overriding getJavaClass() to return IPersistentMap

Right now my patch does not apply to master, and there is no patch implementing Alex's solution, once I get a feedback on which approach is preferable i can: update my patch to apply to master or implement Alex's solution (Alex, if you want to do it yourself I will not step in)

Comment by Alex Miller [ 08/Nov/13 2:33 PM ]

I have actually built a couple of variant patches locally but still have not convinced myself that any of them are the right patch to recommend. I hope to look at it again in the next couple weeks. I think the ticket itself is in the correct state (Incomplete). Ideally I would have a few minutes to look at it with Rich for further direction.

Comment by Nicola Mometto [ 20/Feb/14 6:50 PM ]

Any update on this? this is still marked for clojure 1.6

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

Actually yes! I spent some time on this today and have something to attach.

Comment by Nicola Mometto [ 22/Feb/14 12:28 PM ]

A problem I see with approaches b and c is that they don't address the issue I raised about (def a {:a 1}) here: http://dev.clojure.org/jira/browse/CLJ-944?focusedCommentId=29885&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-29885

Rich stated that it's a different issue than the one being discussed in this ticked, I don't agree, to me it's simply another symptom of the same bug.

I want to suggest a fourth alternative approach to this that combines approach -a with whichever is preferred between -b and -c.

Comment by Alex Miller [ 22/Feb/14 1:53 PM ]

Nicola, I'm not getting your point here (the linked comment doesn't seem to match up to the comments about (def a {:a 1}) so that's confusing me). In the case of (def a {:a 1}), my reading of the bytecode is that RT.map() (which returns IPersistentMap) will be invoked at runtime to generate the map. There is more than one potential concrete class for a (depending on size), but nothing is expecting it to be a particular concrete type here. I do not expect literal maps to retain order or be ArrayMaps (you should call array-map if you want that).

I'm hoping to get some consensus from Rich about the newly updated problem, cause, and approach before I move further. I'd currently classify the problem as the failed .containsKey call, the cause as the compiler replacing the MapExpr with an expression that specifies its type more narrowly than it should, and B and C as some candidate solutions.

If we want the compiler to generate concrete maps that match RT.map(), I think that's ok too, but I don't see it as the cause right now.

Comment by Nicola Mometto [ 22/Feb/14 2:25 PM ]

Alex, I'm sorry here's the correct link: http://dev.clojure.org/jira/browse/CLJ-944?focusedCommentId=30684&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-30684

Your analysis of the emitted bytecode is correct, however when the code is loaded JIT, calls to `def` will be interpreted https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L409-L434 as a result, if the initexpr is a ConstantExpr, the in-memory value of that ConstantExpr will be used and if it's the case that the expression is a map, that will always be a PHM.

I understand that switching between PAMs and PHMs is merely an optimization but having code behave differently if it's AOT compiled rather than JIT loaded is undesiderable IMHO.

Comment by Nicola Mometto [ 22/Feb/14 2:41 PM ]

Immediatelly after posting the previous response it occurred to me that approaches b and c have an inherent problem:
with that approach it's no longer possible to access public fields/methods in instances of PAMs or PHMs that do not belong in IPersistentMap without incurring in reflection.

This means that for example calls to java.lang.Map instance methods would require type-hinting the map literal while now it can be resolved at compile-time without the need of type-hinting.

Here's an example using a first patch that implements the fix as described by approaches -b or -c

user=>  (set! *warn-on-reflection* true)
true

;; pre patch
user=>  (.isEmpty {:a 1})
false

;; after patch
user=>  (.isEmpty {:a 1})
Reflection warning, NO_SOURCE_PATH:2:2 - reference to field isEmpty on clojure.lang.IPersistentMap can't be resolved.
false
Comment by Alex Miller [ 05/Mar/14 11:44 AM ]

Remove from 1.6 release. Still needs more analysis.

Comment by Alex Miller [ 06/Mar/14 7:53 AM ]

Reopened just to fix the ticket fields.





[CLJ-939] Exceptions thrown in the top level ns form are reported without file or line number Created: 24/Feb/12  Updated: 22/Nov/13  Resolved: 22/Nov/13

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

Type: Defect Priority: Major
Reporter: Hugo Duncan Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: errormsgs

Attachments: File 0001-report-load-exceptions-with-file-and-line.diff     File 0002-report-load-exceptions-with-file-and-line.diff     Text File clj-939-report-load-exceptions-with-file-and-line-patch-v2.txt     File clj-939-report-load-exceptions-with-file-and-line-patch-v3.diff     Text File clj-939-report-load-exceptions-with-file-and-line-patch-v3.txt     File clj-939-v4.diff     File screen-clj-939.org    
Patch: Code
Approval: Ok

 Description   

If there is an error in the `ns` form, an exception is thrown, which is not caught in `load`.

For example, with an invalid :only clause;

(ns clj14.myns
  (:use
   [clojure.core :only seq]))

With the latest Clojure master as of Aug 24 2013, this generates the following exception, with no source file or line number except the one shown in clojure.core:

Exception :only/:refer value must be a sequential collection of symbols  clojure.core/refer (core.clj:3854)

You can find a source file in your project if you painstakingly search through the stack trace, but it would be nice if it jumped out at you in the exception itself.

Patch: clj-939-v4.diff

Approach: The latest patch does not modify the behavior of any other exceptions thrown by the compiler. The throw-if function is only used in the load-related functions: this patch changes it to throw CompilerException instead of Exception. As a result, exceptions which occur while loading files will be decorated with file names and line/column numbers. The line/column numbers are not always accurate (see notes in attachment screen-clj-939.org) but the file names are correct.

This is an incremental improvement to compile-time error messages, but it does not solve some of the fundamental problems with reporting correct line/column numbers from errors thrown by the compiler.

Screened by: Stuart Sierra (re-screened by Alex for the specific comments by Rich)



 Comments   
Comment by Hugo Duncan [ 25/Feb/12 8:26 AM ]

Corrected patch

Comment by Andy Fingerhut [ 09/Mar/12 9:26 AM ]

Patch 0001-report-load-exception-with-file-and-line.diff fails build. Patch 0002-report-load-exception-with-file-and-line.diff applies, builds, and tests cleanly as of March 9, 2012. Hugo has signed a CA.

Comment by Andy Fingerhut [ 05/Oct/12 8:13 AM ]

clj-939-report-load-exceptions-with-file-and-line-patch-v2.txt dated Oct 5 2012 is intended to be an update to Hugo Duncan's patch 0002-report-load-exceptions-with-file-and-line.diff dated Feb 25 2012. Because of Brandon Bloom's recently commited patch adding column numbers in addition to line numbers, this is not simply updating some lines of context, but I think it is correct. It would be good if Hugo could take a look at it and confirm.

Comment by Stuart Sierra [ 09/Nov/12 9:38 AM ]

Screened.

The error messages are better than what we had before. The line/column numbers are not particularly informative, probably because ns is a macro.

Comment by Rich Hickey [ 13/Nov/12 3:37 PM ]

This patch doesn't change the reporting on any other (e.g. nested) exceptions? It looks like it might.

Comment by Alex Miller [ 28/Jul/13 10:12 PM ]

This issue has somewhat strange history. Starting over with Triaged so it can go through release assignment and patch process again.

Comment by Andy Fingerhut [ 25/Aug/13 6:01 AM ]

Patch clj-939-report-load-exceptions-with-file-and-line-patch-v3.txt dated Aug 25 2013 started based upon Hugo Duncan's
clj-939-report-load-exceptions-with-file-and-line-patch-v2.txt patch.

It was enhanced to avoid wrapping a CompilerException within another CompilerException first. That always gave line 1 and column 1 for all load/require/use exceptions, so I modified clojure.core/throw-if to throw a CompilerException instead. The line/column numbers with this approach are now usually those of the beginning of the enclosing ns form, if these operations are caused by an ns form.

I also added a little extra info to one exception message, and a new condition in which an exception is thrown if a require/use operand is neither a libspec nor a prefix list.

Comment by Stuart Sierra [ 27/Sep/13 9:45 AM ]

Screening in progress.

Comment by Stuart Sierra [ 27/Sep/13 12:57 PM ]

Screened. Notes in attachment screen-clj-939.org

Comment by Rich Hickey [ 25/Oct/13 7:06 AM ]

this (surreptitiously? adds a new test for vector or list. Please submit patches that address one specific issue. There's no reason for this concrete check, but if you think there is make another ticket.

Comment by Andy Fingerhut [ 25/Oct/13 9:06 AM ]

To: Operative Sierra
From: Chaos

Agent Hickey thwarted world domination plans yet again. Recommend plan omicron. InfoSec dept provided clj-939-v4.diff - should enable our insidious changes to slip past his Detect-O-Tron. Good luck, operative. Message ends.

Comment by Alex Miller [ 25/Oct/13 9:58 AM ]

Seems like the (declare list?) can go away too?

Comment by Andy Fingerhut [ 25/Oct/13 10:49 AM ]

Good catch, Alex. More coffee required here. Uploaded new clj-939-v4.diff in place of the previous one.

Comment by Alex Miller [ 25/Oct/13 10:59 AM ]

Verified questioned bits have been removed and patch still applies cleanly and tests pass. Re-marking Screened with new patch.





[CLJ-937] cl-format prints ratio arguments with bad format for E, F, G directives Created: 21/Feb/12  Updated: 25/Oct/13  Resolved: 25/Oct/13

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

Type: Defect Priority: Trivial
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: print

Attachments: Text File cl-format-efg-coerce-ratios-to-doubes-patch1.txt     File clj-937-cl-format-coerces-ratios-patch2.diff     Text File clj-937-cl-format-coerces-ratios-patch2.txt    
Patch: Code and Test
Approval: Ok

 Description   

Before:

user=> (use 'clojure.pprint)
nil
user=> (cl-format false "~e" 4/5)
"4./5E+2"
user=> (cl-format false "~f" 4/5)
"4/5.0"
user=> (cl-format false "~g" 4/5)
"4/5.    "

After:

user=> (cl-format false "~10,3f" 4/5)
"     0.800"
user=> (cl-format false "~e" 4/5)
"8.0E-1"
user=> (cl-format false "~f" 4/5)
"0.8"
user=> (cl-format false "~g" 4/5)
"0.8    "

Approach: Patch changes cl-format so that when E, F, or G directive is used, the corresponding arg is coerced from a clojure.lang.Ratio to a double before formatting, if it fits in a double, otherwise a BigDecimal if the extra precision is needed.

Patch: clj-937-cl-format-coerces-ratios-patch2.diff

Screened by: Alex Miller



 Comments   
Comment by Tom Faulhaber [ 29/Mar/12 8:48 PM ]

I have reviewed this patch and recommend that it be applied.

(This one has actually been on my to do list for about 4 years. Thanks, Andy!)

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

Patch clj-937-cl-format-coerces-ratios-patch2.txt dated Apr 8 2013 supersedes the previous patch cl-format-efg-coerce-ratios-to-doubes-patch1.txt dated Feb 21 2013.

The newer patch works with ratios that cannot be represented as a double, using BigDecimal in those cases. The older patch would print garbage from the string "Infinity" if the ratios were larger than Double/MAX_VALUE, or 0 if the ratio was between 0 and Double/MIN_VALUE.

Comment by Andy Fingerhut [ 03/Sep/13 1:57 PM ]

Replace patch clj-937-cl-format-coerces-ratios-patch2.txt with another one of the same name. The only change is to eliminate trailing whitespace from some of the lines added by the patch, which eliminates the warnings that occur when git is used to apply the patch.





[CLJ-935] clojure.string/trim uses different defn of whitespace as triml, trimr Created: 21/Feb/12  Updated: 31/Jan/14  Resolved: 31/Jan/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: string

Attachments: Text File clj935-2.patch     Text File clj935-3.patch     Text File fix-trim-fns-different-whitespace-patch.txt    
Patch: Code and Test
Approval: Ok

 Description   

clojure.string/triml and trimr use Character/isWhitespace to determine whether a character is whitespace, but trim uses some other definition of white space character. For example:

user=> (use 'clojure.string)
nil
user=> (def s "  \u2002  foo")
#'user/s
user=> (trim s)
"?  foo"
user=> (triml s)
"foo"

Cause: triml and trimr use Character/isWhitespace. trim uses String/trim which seems to define whitespace as any character less than or equal '\u0020'. The isWhitespace() definition is slightly different and includes other Unicode space characters.

Approach: The attached patch changes trim to use Character/isWhitespace. The isWhitespace version seems generally newer and more Unicode considerate so this was chosen over changing triml and trimr to match trim.

A few alternative implementations were considered with respect to longs, ints, etc. The patch opts to use the simplest possible code, eschewing any extreme performance measures. See the comments for more info if desired.

The patch also changes triml to only call .length on s once.

Patch: clj935-3.patch

Screened by: Stuart Sierra



 Comments   
Comment by Stuart Halloway [ 08/Jun/12 10:22 AM ]

Question for Rich: do we want to be consistent across the different trim fns (per this patch) or consistent with Java, which uses one definition of whitespace in Character/isWhitespace, and a different definition in String/trim?

Comment by Stuart Halloway [ 08/Jun/12 10:22 AM ]

Question for Andy: why the (int ...) forms, when Clojure only works with primitive longs?

Comment by Andy Fingerhut [ 08/Jun/12 10:33 AM ]

Answer for Stuart: Looks like I was preserving the (int ...) form that was in triml before, perhaps somewhat mindlessly. Depending on Rich's answer, I can update the patch if desired.

Comment by Aaron Bedra [ 11/Apr/13 5:34 PM ]

Bump on the discussion. This ticket seems blocked until we make a decision.

Comment by Alex Miller [ 30/Aug/13 3:47 PM ]

In response to the questions regarding the "int" calls in the prior patch, I did some analysis and looked at alternative impls.

Prior patch: the calls to (int) were indeed causing primitive ints to be used in some cases (like letting the length). However, the loop index/rindex is always a prim long at best (and there is no way to type hint or alter that afaict). Because of this, casts get inserted to upcast the length to a long for comparison anyways. Using criterium and calling ltrim on a string of 1M spaces, I got a mean of 2.9 ms.

I tried a dirty trick of using a mutable int array for the index and that (while ugly) was 2.25 ms, so definitely faster. It did not seem fast enough to justify the dirtiness.

I also benchmarked a pure Java int version:

static public CharSequence triml(CharSequence s) {
  int len = s.length();
    for(int index=0; index < len; index++) {
      if(! Character.isWhitespace(s.charAt(index)))
        return s.subSequence(index, len).toString();
    }
  return "";
}

This code is actually smaller and faster - 0.92 ms.

I pondered this and in the end decided to leave it as the most straightforward Clojure impl. I think for the vast majority of trim calls, the difference will be negligible. For users needing higher trim performance, they should probably write an optimized version and tune their whitespace set. My second choice would be adding the Java implementations.

Marking screened.

Comment by Rich Hickey [ 25/Oct/13 7:42 AM ]

did you try

(set! *unchecked-math* true)
?

Comment by Alex Miller [ 02/Dec/13 10:55 PM ]

Testing perf of ltrim on a 1M character blank string (tested on criterium w/ good JVM settings):

clojure.string/ltrim = 2.785597 ms
clj935-2.patch ltrim = 2.880528 ms
same as clj935-2 with unchecked-inc = 2.225098 ms
same as clj935-2 with unchecked-inc-int = 2.198700 ms

I tested the last 3 with unchecked-math both true and false and saw no difference between them.

Based on this, I will update the patch to use unchecked-inc as that seems to give even better performance than the prior impl.

Comment by Rich Hickey [ 03/Jan/14 9:28 AM ]

Is there a gist or anything of the perf tests?





[CLJ-908] Functions with metadata print poorly Created: 10/Jan/12  Updated: 24/May/13  Resolved: 24/May/13

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

Type: Enhancement Priority: Minor
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-Print-metadata-and-anonymous-classes-better.patch     Text File clj-908-Print-metadata-and-anonymous-classes-better-patch2.txt    
Patch: Code
Approval: Ok
Waiting On: