<< Back to previous view

[CLJ-1237] reduce gives a SO for pathological seqs Created: 27/Jul/13  Updated: 28/May/15

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

Type: Defect Priority: Major
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: None
Environment:

1.5.1


Attachments: Text File CLJ-1237c.patch     Text File CLJ-1237d.patch    
Patch: Code and Test
Approval: Incomplete

 Description   

reduce gives a StackOverflowError on long sequences that contain many transitions between chunked and unchunked:

(->> (repeat 50000 (cons :x [:y]))
     (apply concat)
     (reduce (constantly nil)))
;; throws StackOverflowError

Such a sequence is well behaved under most other sequence operations, and its underlying structure can even be masked such that reduce succeeds:

(->> (repeat 50000 (cons :x [:y]))
     (apply concat)
     (take 10000000)
     (reduce (constantly nil)))
;; => nil

I don't think Clojure developers normally worry about mixing chunked and unchunked seqs, so the existence of such a sequence is not at all unreasonable (and indeed this happened to me at work and was very difficult to debug).

It seems obvious what causes this given the implementation of reduce – it bounces back and forth between the chunked impl and the unchunked impl, consuming more and more stack as it goes. Without proper tail call optimization, it's not obvious to me what a good fix would be.

Presumed bad solutions

Degrade to naive impl after first chunk

In the IChunkedSeq implementation, instead of calling internal-reduce when the
sequence stops being chunked, it could have an (inlined?) unoptimized implementation,
ensuring that no further stack space is taken up. This retains the behavior that a
generic seq with a chunked tail will still run in an optimized fashion, but a seq with
two chunked portions would only be optimized the first time.

Use clojure.core/trampoline

This would presumably work, but requires wrapping the normal return values from all
implementations of internal-reduce.

Proposed Solution

(attached as CLJ-1237c.patch)

Similar to using trampoline, but create a special type (Unreduced) that signals
an implementation change. The two implementation-change points in internal-reduce
(in the IChunkedSeq impl and the Object impl) are converted to return an instance
of Unreduced instead of a direct call to internal-reduce.

Then seq-reduce is converted to check for instances of Unreduced before returning,
and recurs if it finds one.

Pros

  • Only requires one additional check in most cases
  • Reduces stack usage for existing heterogeneous reductions that weren't extreme enough to crash
  • Should be compatible with 3rd-party implementations of internal-reduce, which can still use the old style (direct recursive calls to internal-reduce) or the optimized style if desired.

Cons

  • internal-reduce is slightly more complicated
  • There's an extra check at the end of seq-reduce – is that a performance worry?


 Comments   
Comment by Gary Fredericks [ 25/Aug/13 4:13 PM ]

Added patch.

Comment by Gary Fredericks [ 24/Mar/15 11:33 AM ]

My coworker just ran into this, independently.

Comment by Jason Wolfe [ 22/May/15 5:13 PM ]

In Clojure 1.7, this is now happening for more cases, such as:

(->> (repeat 10000 (java.util.ArrayList. (range 10)))
     (apply concat) 
     (reduce (constantly nil)))
Comment by Alex Miller [ 22/May/15 7:39 PM ]

Pulling into 1.7 just so we don't lose it.

Comment by Alex Miller [ 27/May/15 12:16 PM ]

Rich would prefer to degrade to naive impl for this.

Comment by Gary Fredericks [ 27/May/15 4:32 PM ]

Is this still aimed at 1.7? I'm happy to work on a patch, just wanted to know if it was relatively urgent (compared to 1.8).

Comment by Alex Miller [ 27/May/15 6:12 PM ]

Yes, would like a patch.

Comment by Gary Fredericks [ 27/May/15 10:27 PM ]

Attached CLJ-1237d.patch, which contains the naive-impl-degradation implementation, and two tests – one for my original case, and the other as reported by Jason Wolfe on the mailing list in response to more recent clojure changes.

Comment by Alex Miller [ 28/May/15 7:28 AM ]

I think we also need to do this same change in the Object case of InternalReduce (the other place where coll-reduce is called) which you can trip simply with something like:

(reduce + (concat (repeat 2 2) (range 5)))

I couldn't quickly come up with an easy large case that demonstrated this though. It needs to be some interleaving of different kinds of non-chunked sequences I think?

Comment by Gary Fredericks [ 28/May/15 9:43 AM ]

Yeah, I had thought to try to analyze the call graph to see if there are other routes to a stackoverflow, but hadn't done that yet. I hereby begin intending to do that more vigorously.

Comment by Alex Miller [ 28/May/15 10:34 AM ]

That Object case is the only other place that loops back to coll-reduce, creating the potential for arbitrary stack usage.





[CLJ-1739] Broken set equality for sets of equal sets Created: 28/May/15  Updated: 28/May/15  Resolved: 28/May/15

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

Type: Defect Priority: Major
Reporter: Tassilo Horn Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: collections, interop


 Description   

With both clojure 1.6.0 and 1.7.0-RC1 I get the following inconsistent behavior.

Different kinds of sets are equal which is expected:

(= #{1 2 3} (flatland.ordered.set/ordered-set 1 2 3)) ;=> true
(= #{1 2 3} (java.util.HashSet. [1 2 3]))             ;=> true

However, sets containing equal sets are not equal:

(= #{#{1 2 3}} #{(flatland.ordered.set/ordered-set 1 2 3)}) ;=> false
(= #{#{1 2 3}} #{(java.util.HashSet. [1 2 3])})             ;=> false

This is similar to http://dev.clojure.org/jira/browse/CLJ-1649 and probably caused by http://dev.clojure.org/jira/browse/CLJ-1372.



 Comments   
Comment by Alex Miller [ 28/May/15 6:14 AM ]

This is a duplicate version of the problem in CLJ-1372. = will check that the persistent collection contains each element from the other collection, but because the hash codes are different for the Java version of the set, the element is not found and equality fails.

user=> (contains? #{#{1 2 3}} #{1 2 3})
true
user=> (contains? #{#{1 2 3}} (java.util.HashSet. [1 2 3]))
false
Comment by Andy Fingerhut [ 28/May/15 10:13 AM ]

There could be an issue where flatland.ordered.set/ordered-set is still using a pre-Clojure-1.6.0 hash function, and should be updated.





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

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

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

Approval: Triaged

 Description   

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

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

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

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

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



 Comments   
Comment by Valerie Houseman [ 24/Mar/15 6:11 PM ]

Please reroute to the CLJ project, as I lack access to do so - my bad.

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

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

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

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

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

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

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

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





[CLJ-1738] Chunked iterator-seq incompatible with Java iterators that return the same mutable object every time Created: 27/May/15  Updated: 27/May/15

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

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

1.7.0-RC1


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

 Description   

Clojure code that uses iterator-seq to wrap Java iterators that return the same mutable object on every call are broken by the chunked iterator-seq changes from CLJ-1669.

Some examples where this occurs:

  • Hadoop ReduceContextImpl$ValueIterator
  • Mahout DenseVector$AllIterator/NonDefaultIterator
  • LensKit FastIterators

Cause: In 1.6, the iterator-seq wrapper could be used with these to consume a sequence over these iterators element-by-element. In 1.7 RC1, iterator-seq produces a chunked sequence. Because next() is called 32 times on the iterator before the first value can be retrieved from the seq, and the same mutable object is returned every time, code doing this now receives different (incorrect) results.

Approach: Switch iterator-seq back to non-chunked and change eduction to use the chunking iterator-seq strategy as that was the original target. We can also retain the use of the chunked iterator seq in sequence over the TransformerIterator.

Patch: clj-1738.patch



 Comments   
Comment by Alex Miller [ 27/May/15 7:00 AM ]

I spot-checked some of the perf timings from CLJ-1669 and didn't see anything unexpected.

Comment by Marshall T. Vandegrift [ 27/May/15 7:38 AM ]

In order to maintain compatibility it is also necessary to change `clojure.lang.RT/seqFrom` back to creating non-chunked `IteratorSeq`s. I've verified that these changes are sufficient to restore compatibility for my cases.

Comment by Marshall T. Vandegrift [ 27/May/15 10:05 AM ]

Added updated version of proposed patch which covers RT Iterable->seq coercion and includes a test case.





[CLJ-1103] Make conj assoc dissoc and transient versions handle args similarly Created: 04/Nov/12  Updated: 26/May/15

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

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: collections, transient

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

 Description   

Examples that work as expected:

Clojure 1.7.0-master-SNAPSHOT
user=> (dissoc {})
{}
user=> (disj #{})
#{}
user=> (conj {})
{}
user=> (conj [])
[]

Examples that do not work as desired, but are changed by the proposed patch:

user=> (assoc {})
ArityException Wrong number of args (1) passed to: core/assoc  clojure.lang.AFn.throwArity (AFn.java:429)
user=> (assoc! (transient {}))
ArityException Wrong number of args (1) passed to: core/assoc!  clojure.lang.AFn.throwArity (AFn.java:429)
user=> (dissoc! (transient {}))
ArityException Wrong number of args (1) passed to: core/dissoc!  clojure.lang.AFn.throwArity (AFn.java:429)

I looked through the rest of the code for similar cases, and found that there were some other differences between them in how different numbers of arguments were handled, such as:

+ conj handles an arbitrary number of arguments, but conj! does not.
+ assoc checks for a final key with no value specified (CLJ-1052), but assoc! did not.

History/discussion: A discussion came up in the Clojure Google group about conj giving an error when taking only a coll as an argument, as opposed to disj which works for this case:

https://groups.google.com/forum/?fromgroups=#!topic/clojure/Z9mFxsTYTqQ



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

clj-1103-make-conj-assoc-dissoc-handle-args-similarly-v1.txt dated Nov 4 2012 makes conj conj! assoc assoc! dissoc dissoc! handle args similarly to each other.

Comment by Brandon Bloom [ 09/Dec/12 5:30 PM ]

I too ran into this and started an additional discussion here: https://groups.google.com/d/topic/clojure-dev/wL5hllfhw4M/discussion

In particular, I don't buy the argument that (into coll xs) is sufficient, since into implies conj and there isn't an terse and idiomatic way to write (into map (parition 2 keyvals))

So +1 from me

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

Patch clj-1103-2.diff is identical to the previous patch clj-1103-make-conj-assoc-dissoc-handle-args-similarly-v1.txt except it applies cleanly to latest master. The only changes were some changed context lines in a test file.

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

Patch clj-1103-3.diff is identical to the patch clj-1103-2.diff, except it applies cleanly to latest master. The only changes were some doc strings for assoc! and dissoc! in the context lines of the patch.

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

Patch clj-1103-4.diff is identical to the previous clj-1103-3.diff, except it updates some context lines so that it applies cleanly to latest Clojure master as of today.

Comment by Alex Miller [ 05/Jun/14 9:29 PM ]

Can someone update the description with some code examples? Or drop them here at least?

Comment by Brandon Bloom [ 05/Jun/14 9:35 PM ]

What do you mean code examples?

These currently work as expected:
(dissoc {})
(disj #{})

The following fail with arity errors:
(assoc {})
(conj {})

Similarly for the transient ! versions.

This is annoying if you ever try to do (apply assoc m keyvals)... it works at first glance, but then one day, bamn! Runtime error because you tried to give it an empty sequence of keyvals.

Comment by Andy Fingerhut [ 06/Aug/14 5:05 PM ]

Patch clj-1103-5.diff dated Aug 6 2014 applies cleanly to latest Clojure master as of today, whereas the previous patch did not. Rich added 1-arg version of conj to 1.7.0-alpha1, so that change no longer is part of this patch.

Comment by Andy Fingerhut [ 29/Aug/14 6:04 PM ]

Patch clj-1103-6.diff dated Aug 29 2014 is identical to the former patch clj-1103-5.diff (which will be deleted), except it applies cleanly to the latest Clojure master.

Comment by Andy Fingerhut [ 26/May/15 9:00 PM ]

Patch clj-1103-7.diff dated May 25 2015 is identical to the former patch clj-1103-6.diff (which will be deleted), except it applies cleanly to the latest Clojure master.





[CLJ-1722] Typo in the doc string of `with-bindings` Created: 03/May/15  Updated: 26/May/15

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

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

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

 Description   

The doc string says "the execute body". It should say "then execute body".



 Comments   
Comment by Andy Fingerhut [ 26/May/15 8:47 AM ]

Alex, this one 'falls off the JIRA state chart' since Rich hasn't assigned it a Fix Version. Should Approval be Triaged instead?





[CLJ-1402] sort-by calls keyfn more times than is necessary Created: 11/Apr/14  Updated: 26/May/15

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

Type: Enhancement Priority: Minor
Reporter: Steve Kim Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: performance

Attachments: Text File CLJ-1402-v1.patch     Text File CLJ-1402-v2.patch    
Patch: Code

 Description   

clojure.core/sort-by evaluates keyfn for every pairwise comparison. This is wasteful when keyfn is expensive to compute.

user=> (def keyfn-calls (atom 0))
#'user/keyfn-calls
user=> (defn keyfn [x] (do (swap! keyfn-calls inc) x))
#'user/keyfn
user=> @keyfn-calls
0
user=> (sort-by keyfn (repeatedly 10 rand))
(0.1647483850582695 0.2836687590331822 0.3222305842748623 0.3850390922996001 0.41965440953966326 0.4777580378736771 0.6051704988802923 0.659376178201709 0.8459820304223701 0.938863131161208)
user=> @keyfn-calls
44


 Comments   
Comment by Steve Kim [ 11/Apr/14 11:46 AM ]

CLJ-99 is a similar issue

Comment by Michael Blume [ 09/Feb/15 3:03 PM ]

Avoid using for before it's defined

Comment by Andy Fingerhut [ 25/May/15 5:13 PM ]

Michael, does your patch CLJ-1402-v2.patch intentionally modify the doc string of sort-by, because the sentence you are removing is now obsolete? If so, that would be good to mention explicitly in the comments here.

Comment by Michael Blume [ 26/May/15 2:41 PM ]

Yep, the patch changes sort-by so that it maps over the collection and then performs a sort on the resulting seq. This means arrays will be unmodified and a new seq created instead.





[CLJ-1659] compile leaks files Created: 16/Feb/15  Updated: 26/May/15

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

Type: Defect Priority: Major
Reporter: Ralf Schmitt Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, newbie

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

 Description   

clojure's compile function leaks file descriptors, i.e. it relies on garbage collection to close the files. I'm trying to use boot [1] on windows and ran into the problem, that files could not be deleted intermittently [2]. The problem is that clojure's compile function, or rather clojure.lang.RT.lastModified relies on garbage collection to close files. lastModified looks like:

static public long lastModified(URL url, String libfile) throws IOException{
	if(url.getProtocol().equals("jar")) {
		return ((JarURLConnection) url.openConnection()).getJarFile().getEntry(libfile).getTime();
	}
	else {
		return url.openConnection().getLastModified();
	}
}

Here's the stacktrace from file leak detector [3]:

#205 C:\Users\ralf\.boot\tmp\Users\ralf\home\steinmetz\2mg\-x24pa9\steinmetz\fx\config.clj by thread:clojure-agent-send-off-pool-0 on Sat Feb 14 19:58:46 UTC 2015
    at java.io.FileInputStream.(FileInputStream.java:139)
    at java.io.FileInputStream.(FileInputStream.java:93)
    at sun.net.www.protocol.file.FileURLConnection.connect(FileURLConnection.java:90)
    at sun.net.www.protocol.file.FileURLConnection.initializeHeaders(FileURLConnection.java:110)
    at sun.net.www.protocol.file.FileURLConnection.getLastModified(FileURLConnection.java:178)
    at clojure.lang.RT.lastModified(RT.java:390)
    at clojure.lang.RT.load(RT.java:421)
    at clojure.lang.RT.load(RT.java:411)
    at clojure.core$load$fn__5066.invoke(core.clj:5641)
    at clojure.core$load.doInvoke(core.clj:5640)
    at clojure.lang.RestFn.invoke(RestFn.java:408)
    at clojure.core$load_one.invoke(core.clj:5446)
    at clojure.core$compile$fn__5071.invoke(core.clj:5652)
    at clojure.core$compile.invoke(core.clj:5651)
    at pod$eval52.invoke(NO_SOURCE_FILE:0)
    at clojure.lang.Compiler.eval(Compiler.java:6703)
    at clojure.lang.Compiler.eval(Compiler.java:6693)
    at clojure.lang.Compiler.eval(Compiler.java:6666)
    at clojure.core$eval.invoke(core.clj:2927)
    at boot.pod$eval_in_STAR_.invoke(pod.clj:203)
    at clojure.lang.Var.invoke(Var.java:379)
    at org.projectodd.shimdandy.impl.ClojureRuntimeShimImpl.invoke(ClojureRuntimeShimImpl.java:88)
    at org.projectodd.shimdandy.impl.ClojureRuntimeShimImpl.invoke(ClojureRuntimeShimImpl.java:81)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:483)
    at clojure.lang.Reflector.invokeMatchingMethod(Reflector.java:93)
    at clojure.lang.Reflector.invokeInstanceMethod(Reflector.java:28)
    at boot.pod$eval_in_STAR_.invoke(pod.clj:206)
    at boot.task.built_in$fn__1417$fn__1418$fn__1421$fn__1422.invoke(built_in.clj:433)
    at boot.task.built_in$fn__1443$fn__1444$fn__1447$fn__1448.invoke(built_in.clj:446)
    at boot.task.built_in$fn__1190$fn__1191$fn__1194$fn__1195.invoke(built_in.clj:232)
    at boot.core$run_tasks.invoke(core.clj:663)
    at clojure.lang.Var.invoke(Var.java:379)
    at boot.user$eval297$fn__298.invoke(boot.user4212477544188689077.clj:33)
    at clojure.core$binding_conveyor_fn$fn__4145.invoke(core.clj:1910)
    at clojure.lang.AFn.call(AFn.java:18)
    at java.util.concurrent.FutureTask.run(FutureTask.java:266)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
    at java.lang.Thread.run(Thread.java:745)

So, it looks like getLastModified opens an InputStream internally. On Stackoverflow [4] there's a discussion on how to close the URLConnection correctly.

On non-Windows operating systems this shouldn't be much of a problem. But on windows this hurts very much, since you can't delete
files that are opened by some process.

I've tested with clojure 1.6.0, but I assume other version are also affected.

[1] http://boot-clj.com/
[2] https://github.com/boot-clj/boot/issues/117
[3] http://file-leak-detector.kohsuke.org/
[4] http://stackoverflow.com/questions/9150200/closing-urlconnection-and-inputstream-correctly



 Comments   
Comment by Pietro Menna [ 06/May/15 11:10 AM ]

First attempt Patch

Comment by Pietro Menna [ 06/May/15 11:13 AM ]

Hi Alex,

This is my first patch to Clojure and to any OSS. So maybe I will need a little guidance. I follow the steps on how to generate the patch and just uploaded the patch to this thread.

The link from Stack Overflow was good, but unfortunately it is not possible to cast to HttpURLConnection in order to have the .disconnect() method.

Please, let me know if I should attempt anything else.

Kind regards,

Pietro

Comment by Andy Fingerhut [ 06/May/15 11:20 AM ]

Seems that creating a test for this to be run on every build might be difficult.

Have you verified that on Windows it has the desired effect?

Comment by Ralf Schmitt [ 06/May/15 4:49 PM ]

I don't understand how the patch solves that issue. It just sets connection to null. Or am I missing something?

You can test for the file leak with the following program. This works on windows and Linux for me.

leak-test.clj
;; test file leak
;; on UNIX-like systems set hard limit on open files via
;; ulimit -H -n 200 and then run
;; java -jar clojure-1.6.0.jar leak-test.clj

(let [file (java.io.File. "test-leak.txt")
      url (.. file toURI toURL)]
  (doseq [x (range 2000)]
    (print x " ")
    (flush)
    (spit file "")
    (clojure.lang.RT/lastModified url nil)
    (assert (.delete file) "delete failed")))
Comment by Ralf Schmitt [ 06/May/15 5:21 PM ]

dammit, the formatting is wrong. but this patch seems to fix the problem for me (tested on linux).

Comment by Ralf Schmitt [ 06/May/15 6:16 PM ]

indentation fixed

Comment by Andy Fingerhut [ 07/May/15 8:45 AM ]

Ralf, thanks for the patch. I can't say if or when this ticket will be considered for a change to Clojure, but I do know that patches are only considered if they were written by someone who has signed a CA. Were you considering doing so? You can do it on-line here if you wish: http://clojure.org/contributing

Also, patches should be in a slightly different format that include the author's name, email, date of change, etc. Instructions for creating a patch in that format are here: http://dev.clojure.org/display/community/Developing+Patches

Comment by Ralf Schmitt [ 07/May/15 8:50 AM ]

Thanks for the explanation. I've signed the CA and I will update the patch.

Comment by Ralf Schmitt [ 07/May/15 9:34 AM ]

patch vs current master, created with git format-patch

Comment by Andy Fingerhut [ 26/May/15 8:41 AM ]

Ralph, it would be good if all attached files on a ticket have different names, for clarity when referring to them. Could you remove your clj-1659.patch and upload it with a different name, e.g. clj-1659-v2.patch ?

Comment by Ralf Schmitt [ 26/May/15 8:49 AM ]

upload my last patch with non-conflicting filename

Comment by Ralf Schmitt [ 26/May/15 8:52 AM ]

yes, sorry about that. I don't know how to remove my previous patches. (ok, found it now)





[CLJ-1657] proxy creates bytecode that calls super methods of abstract classes Created: 08/Feb/15  Updated: 26/May/15

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

Type: Defect Priority: Minor
Reporter: Alexander Yakushev Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None
Environment:

Everywhere, but so far relevant only on Android 5.0


Attachments: File CLJ-1657-patch.diff    
Patch: Code

 Description   

When proxy is used to extend abstract classes (e.g. java.io.Writer), the bytecode it produces include the call to non-existing super methods. For example, here's decompiled method from clojure/pprint/column_writer.clj:

public void close()
    {
        Object obj;
label0:
        {
            obj = RT.get(__clojureFnMap, "close");
            if(obj == null)
                break label0;
            ((IFn)obj).invoke(this);
            break MISSING_BLOCK_LABEL_31;
        }
        JVM INSTR pop ;
        super.close();
    }

As you can see on the last line, super.close() tries to call a non-defined method (because close() is abstract in Writer).

This hasn't been an issue anywhere until Android 5.0 came out. Its bytecode optimizer is very aggressive and rejects such code. Google guys claim that it is a bug in their code, which they already fixed[1]. Still I wonder if having faulty bytecode, that is not valid by Java standards, might cause issues in future (not only on Android, but in other enviroments too).

[1] https://code.google.com/p/android/issues/detail?id=80687



 Comments   
Comment by Alexander Yakushev [ 18/Mar/15 5:31 AM ]

I attached a patch that resolves the issue. The change makes `generate-proxy` treat abstract methods like interface methods. Which means, if the implementation for the method is not provided, it will throw unsupported exception rather than try to call the parent method (which doesn't exist).

Comment by Michael Blume [ 18/Mar/15 12:50 PM ]

Alexander: Awesome, thanks =)

Note: If you use git format-patch after making a commit, you can generate a patch file with your name/e-mail and a commit message that a clojure maintainer can apply directly to clojure as a new commit.

Comment by Alex Miller [ 18/Mar/15 12:53 PM ]

The patch process is documented here: http://dev.clojure.org/display/community/Developing+Patches

Comment by Alexander Yakushev [ 18/Mar/15 4:38 PM ]

Sorry, I should have checked the guidelines first. I uploaded a new patch, hope it is correct now.





[CLJ-1598] Make if forms compile directly to the appropriate branch expression if the test is a literal Created: 24/Nov/14  Updated: 26/May/15

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

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: compiler, performance, primitives

Attachments: Text File 0001-if-test-expr-of-an-if-statement-is-a-literal-don-t-e.patch    
Patch: Code
Approval: Triaged

 Description   

This allows expressions like `(cond (some-expr) 1 :else 2)` to be eligible for unboxed use, which is not currently possible since the cond macro always ends up with a nil else branch that the compiler currently takes into account.

With the attached patch, the trailing (if :else 2 nil) in the macroexpansion will be treated as 2 by the Compiler, thus allowing the unboxed usage of the cond expression.






[CLJ-1607] docstring for clojure.core/counted? should be more specific Created: 29/Nov/14  Updated: 26/May/15

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

Type: Enhancement Priority: Minor
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: docstring

Attachments: Text File CLJ-1607-p1.patch    
Patch: Code
Approval: Triaged

 Description   

The docstring for counted? currently says:

Returns true if coll implements count in constant time

This tempts the user into thinking they can use this function to determine whether or not calling count on any collection is a constant-time operation, when in fact it only reflects whether or not an object implements the clojure.lang.Counted interface. Since count special-cases a handful of platform types, there are common cases such as Arrays and Strings that are constant time but will return false from counted?.

Proposed:

Returns true if coll, a Clojure collection, implements count in constant time. Note that this function will return false for host types even if the count function can return their size in constant time (as with arrays and strings).



 Comments   
Comment by Gary Fredericks [ 29/Nov/14 9:01 AM ]

Attached CLJ-1607-p1.patch with my first draft of a better docstring.

Comment by Gary Fredericks [ 29/Nov/14 9:08 AM ]

What would be the most accurate language to describe the exceptions? I used "some collections" in the first patch but perhaps "native collections" or "host collections" would be more helpful?

Comment by Alex Miller [ 29/Nov/14 9:44 AM ]

While I understand where you're coming from, I think the intent of "counted?" is not to answer the question "is this thing countable in constant time" for all possible types, but specifically for collections that participate in the Clojure collection library. This includes both internal collections like PHM, PHS, PV, etc but also external collections that mark their capabilities using those interfaces.

I believe count handles more cases than just collections that are counted in constant time (like seqs) so is not intended to be symmetric with counted?.

Comment by Gary Fredericks [ 29/Nov/14 9:55 AM ]

Sure, I wasn't suggesting changing what the function does – just changing the docstring to make it less likely to be misleading.

Comment by Gary Fredericks [ 29/Nov/14 10:00 AM ]

What about this sort of wording?

Returns true if coll, a Clojure collection, implements count in constant time.
Note that this function will return false for host types even if the count 
function can return their size in constant time (as with arrays and strings).
Comment by Alex Miller [ 30/Nov/14 9:52 PM ]

I think it's unlikely to pass vetting, but that's just my guess.

Comment by Gary Fredericks [ 01/Dec/14 8:53 AM ]

I'm trying to figure out where the disagreement is here; are you arguing any of these points, or something different?

  1. The docstring is not likely to confuse people by making them think it gives meaningful responses for host collections
  2. It's not a problem for us to solve if the docstring confuses people
  3. It is a problem we should solve, but the changes I've suggested are a bad solution
Comment by Alex Miller [ 01/Dec/14 9:18 AM ]

In general, the docstrings prefer concision and essence over exhaustive cases or examples. My suspicion is that the docstring says what Rich wants it to say and he would consider the points you've added to be implicit in the current docstring, and thus unnecessary. Specifically, "coll" is used pretty consistently to mean a Clojure collection (or sequence) across all of the docstrings. And there is an implicit else in the docstring that counted? will return false for things that are not Clojure collections. The words that are there (and not there) are carefully chosen.

I agree with you that more words may be necessary to describe fully what to expect from this or any other function in core. My experience from seeing Rich's response on things like this is that he may agree with that too, but he would prefer it to live somewhere outside the doc string in reference material or other sources. Not to say that we don't update docstrings, as that does happen pretty regularly; I just don't think this one will be accepted. I've asked Stu to give me a second set of eyes too.

Comment by Gary Fredericks [ 01/Dec/14 9:36 AM ]

That was helpful detail, thanks!

Comment by Reid McKenzie [ 01/Dec/14 12:42 PM ]

I think this one is fine as-is, because the docstring for count explicitly notes "Also works on ..." which are implied not to be counted?.





[CLJ-1292] print docstring should specify nil return value Created: 01/Nov/13  Updated: 26/May/15

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

Type: Enhancement Priority: Trivial
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, print

Attachments: File clj-1292.diff    
Patch: Code

 Description   

The docstring for print does not mention its return value. The docstring should clarify whether print dependably returns nil or shouldn't be depended on to (lest, for example, something leak out as the inadvertent return value of print's caller).



 Comments   
Comment by Édipo L Féderle [ 06/Nov/14 7:46 PM ]

Hi, I just add to docstring the mention to fact that it return nil





[CLJ-1733] Tagged literals throw on records containing sorted-set Created: 19/May/15  Updated: 26/May/15

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

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

Clojure 1.6.0
Clojure 1.7.0-alpha5
Clojure 1.7.0-beta3

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


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

 Description   

This is a deadly combination for some reason:

1. Take sorted-set
2. Put it to a record
3. Return record from tagged literal reader fn

(ns tonsky)

(defrecord A [a])

(defn a [arg] (A. (sorted-set arg)))

(set! *data-readers* {'tonsky/tag tonsky/a})

(prn (a "123")) ;; => #tonsky.A{:a #{"123"}}
(prn #tonsky/tag "123") ;; =>
Caused by: java.lang.ClassCastException: Cannot cast clojure.lang.PersistentVector to clojure.lang.ISeq
	at java.lang.Class.cast(Class.java:3258)
	at clojure.lang.Reflector.boxArg(Reflector.java:427)
	at clojure.lang.Reflector.boxArgs(Reflector.java:460)
	at clojure.lang.Reflector.invokeMatchingMethod(Reflector.java:58)
	at clojure.lang.Reflector.invokeStaticMethod(Reflector.java:207)
	at clojure.lang.Reflector.invokeStaticMethod(Reflector.java:200)
	at clojure.lang.LispReader$EvalReader.invoke(LispReader.java:1048)
	at clojure.lang.LispReader$DispatchReader.invoke(LispReader.java:616)
	at clojure.lang.LispReader.read(LispReader.java:183)
	at clojure.lang.RT.readString(RT.java:1785)
	at tonsky$eval34.<clinit>(tonsky.clj:10)
	... 17 more


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

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

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

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

(defrecord Rec [f])

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

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





[CLJ-1467] Implement Comparable in PersistentList Created: 17/Jul/14  Updated: 26/May/15

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

Type: Enhancement Priority: Major
Reporter: Pascal Germroth Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: collections

Attachments: Text File 0001-first-try-for-adding-compare.patch    
Patch: Code and Test

 Description   

PersistentVector implements Comparable already.



 Comments   
Comment by Bart Kastermans [ 13/Nov/14 11:17 AM ]

Patch for this issue; done with Jeroen van Dijk and Razvan Petruescu at a clojure meetup. Any feedback welcome; the learning for me here is not the fix, but learning how to deal with ant and jira etc.

Comment by Andy Fingerhut [ 13/Nov/14 12:31 PM ]

Looks like you have navigated the steps for creating a patch in the desired format, and attaching it to a JIRA ticket, just fine. I see your name on the list of contributors, which is a precondition before a patch can be committed to Clojure or a contrib library.

You've gotten past what are actually the easier parts. There is still the issue of whether this ticket is even considered by the Clojure core team to be an enhancement worth making a change to Clojure. Take a look at the JIRA workflow here if you haven't seen it already and are curious: http://dev.clojure.org/display/community/JIRA+workflow

If you like Pascal think that this is a change you really want to see in Clojure, you may vote on this or any other JIRA ticket (except ones you create yourself – the creator is effectively the 0th voter for a ticket). Log in and click on the Vote link near the top right, and/or Watch to get email updates of changes.

Comment by Bart Kastermans [ 14/Nov/14 3:12 AM ]

Andy, thanks for the info. I was not aware of the JIRA workflow.





[CLJ-1611] clojure.java.io/pushback-reader Created: 08/Dec/14  Updated: 26/May/15

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

Type: Enhancement Priority: Major
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: io, reader

Attachments: Text File drupp-clj-1611-2.patch     Text File drupp-clj-1611.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Whereas

  • clojure.core/read and clojure.edn/read require a PushbackReader;
  • clojure.java.io/reader produces a BufferedReader, which isn't compatible;
  • the hazard has tripped folks up for years[1];
  • clojure.java.io is pure sugar anyway (and would not be damaged by the addition of a little bit more);
  • clojure.java.io's very existence suggests suitability and fitness for use (wherein by the absence of a read-compatible pushback-reader it falls short);

i.e., in the total absence of clojure.java.io it would not seem "hard" to use clojure.edn, but in the presence of clojure.java.io and its "reader" function, amidst so much else in the API that does fit together, one keeps thinking one is doing it wrong;

and

  • revising the "read" functions to make their own Pushback was considered but rejected [2];

Therefore let it be suggested to add clojure.java.io/pushback-reader, returning something consumable by clojure.core/read and clojure.edn/read.

[1] The matter was discussed on Google Groups:

(2014, "clojure.edn won't accept clojure.java.io/reader?") https://groups.google.com/forum/#!topic/clojure/3HSoA12v5nc

with a reference to an earlier thread

(2009, "Reading... from a reader") https://groups.google.com/forum/#!topic/clojure/_tuypjr2M_A

[2] CLJ-82 and the 2009 message thread



 Comments   
Comment by David Rupp [ 10/Jan/15 4:05 PM ]

Attached patch drupp-clj-1611.patch implements clojure.java.io/pushback-reader as requested.

Comment by David Rupp [ 10/Jan/15 4:07 PM ]

Note that you can always import java.io.PushbackReader and do something like (PushbackReader. (reader my-thing)) yourself; that's really all the patch does.

Comment by Phill Wolf [ 11/Jan/15 7:54 AM ]

clojure.java.io/reader is idempotent, while the patch of 10-Jan-2015 re-wraps an existing PushbackReader twice: first with a new BufferedReader, then with a new PushbackReader.

Leaving a given PushbackReader alone would be more in keeping with the pattern of clojure.java.io.

It also needs a docstring. If pushback-reader were idempotent, the docstring's opening phrase could echo clojure.java.io/reader's, e.g.: Attempts to coerce its argument to java.io.PushbackReader; failing that, (bla bla bla).

Comment by David Rupp [ 11/Jan/15 11:14 AM ]

Adding drupp-clj-1611-2.patch to address previous comments.





[CLJ-1380] Three-arg ExceptionInfo constructor permits nil data Created: 13/Mar/14  Updated: 25/May/15

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

Type: Defect Priority: Minor
Reporter: Gordon Syme Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: checkargs

Attachments: File clj-1380.diff    
Patch: Code and Test

 Description   

The argument check in the two-arg clojure.lang.ExceptionInfo constructor isn't present in the three-arg constructor so it's possible to create an ExceptionInfo with arbitrary (or nil) data.

E.g.:

user=> (clojure-version)
"1.5.1"

user=> (ex-info "hi" nil)
IllegalArgumentException Additional data must be a persistent map: null  clojure.lang.ExceptionInfo.<init> (ExceptionInfo.java:26)

user=> (ex-info "hi" nil (Throwable.))
NullPointerException   clojure.lang.ExceptionInfo.toString (ExceptionInfo.java:40)


 Comments   
Comment by Gordon Syme [ 13/Mar/14 10:47 AM ]

Sorry, didn't meant to classify as "major" and I don't have permissions to edit.

Comment by Gordon Syme [ 13/Mar/14 11:11 AM ]

Patch + tests

I'm not at all familiar with the project so may have put tests in the wrong language and/or wrong place.

The ex-info-works test is a bit dorky but shows that both constructors are equivalent (and passes without the patch to ExceptionInfo).

Comment by Alex Miller [ 13/Mar/14 12:18 PM ]

No worries on the classification - I adjust most incoming tickets in some way or another.

Thanks for the patch, however it cannot be considered unless you complete the Clojure Contributor's Agreement - http://clojure.org/contributing. This is an important step in the process that keeps the Clojure codebase on a sound legal basis.

Someone else could develop a clean room patch implementation for this ticket later, but of course it would be ideal if you could become a contributor!

Comment by Gordon Syme [ 13/Mar/14 1:15 PM ]

Hi Alex,

sure, that makes sense. I'll get the contributor's agreement in the post. It may take a while to arrive since I'm based in Europe.

Comment by Gordon Syme [ 25/Mar/14 10:03 AM ]

I just checked http://clojure.org/contributing, looks like my CCA made it through

Comment by Andy Fingerhut [ 01/Oct/14 6:48 PM ]

Gordon, I do not know if your patch is of interest to the Clojure developers, so I can't comment on that aspect of this ticket.

Instructions for creating a patch in the expected format is given on the wiki page below. Your patch is not in the expected format.

http://dev.clojure.org/display/community/Developing+Patches

Comment by Gordon Syme [ 27/Oct/14 5:30 AM ]

Whoops, sorry Andy.

I've rebased against master and added a correctly formatted patch.





[CLJ-1595] Nested doseqs leak with sequence of huge lazy-seqs Created: 20/Nov/14  Updated: 25/May/15

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

Type: Defect Priority: Minor
Reporter: Andrew Rudenko Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File doseq_leaks.diff    
Patch: Code

 Description   

Hello!

This little snippet demonstrates the problem:

(doseq [outer-seq (list (range)) inner-seq outer-seq])

That's it. It is not just eats my processor, but also eats all available memory. Practically it can affect (and it is) at consuming of complex lazy structures like huge XML-documents.

I think this is at least non trivial behaviour.

It can be fixed by this small patch. We can get next element before current iteration, not after, so outer loop will not hold reference to the head of inner-seq.

This patch doesn't solve all problems, for example this code:

(doseq [outer-seq [(range)] inner-seq outer-seq])

leaks. Because chunked-seqs (vector in this case) holds current element by design.



 Comments   
Comment by Andy Fingerhut [ 25/May/15 3:15 PM ]

Andrew, sorry but I do not know whether this ticket is of interest to the Clojure core team.

I do know that patches are only considered for inclusion in Clojure if the submitter has signed the contributor agreement (CA). If you were interested in doing that, you can do it fairly quickly on-line here: http://clojure.org/contributing





[CLJ-1647] infinite loop in 'partition' and 'partition-all' when 'step' or 'n' is not positive Created: 20/Jan/15  Updated: 25/May/15

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

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

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

 Description   

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

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

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

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



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

Also see CLJ-764

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

Needs a perf check when done.

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

patch file to fix clj-1647

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

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

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

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

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

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

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

I will supply a patch file without the whitespace changes.

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

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

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

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

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

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

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

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

Any help would be greatly appreciated.

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

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

Did you mvn clean? Or rm target?

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

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

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

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

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





[CLJ-1737] Omit java exception class from CompilerException message Created: 23/May/15  Updated: 23/May/15

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

Type: Enhancement Priority: Minor
Reporter: John Hume Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, errormsgs, patch, usability

Attachments: Text File clearer-CompilerException-messase.patch     File compiler_exception_examples.clj    
Patch: Code

 Description   

A CompilerException is always created with a cause exception. Currently the message is built using cause.toString(), which for all examples I've examined is the cause class, followed by a colon, followed by the cause message. In all those examples, the message of the cause is informative, and the class name provides no additional help.

I propose to switch to using cause.getMessage() rather than cause.toString(). This would make it easier for tools to present compiler errors that don't leak implementation details that may confuse a new user. The cause class would still be shown in the stack trace.

Here are the examples I looked at, with the output from before the attached patch:

Example source '(ns foo)

(def'
Exception message:
 java.lang.RuntimeException: EOF while reading, starting at line 3, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:3:1)

Example source ':foo}'
Exception message:
 java.lang.RuntimeException: Unmatched delimiter: }, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:6)

Example source 'foo'
Exception message:
 java.lang.RuntimeException: Unable to resolve symbol: foo in this context, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:14:1)

Example source 'clojure.core/firstt'
Exception message:
 java.lang.RuntimeException: No such var: clojure.core/firstt, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:15:1)

Example source '(nil 1)'
Exception message:
 java.lang.IllegalArgumentException: Can't call nil, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:1)

Example source '("hi" 1)'
Exception message:
 java.lang.ClassCastException: java.lang.String cannot be cast to clojure.lang.IFn, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:1)

Example source '{:foo}'
Exception message:
 java.lang.RuntimeException: Map literal must contain an even number of forms, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:7)

Example source '1st'
Exception message:
 java.lang.NumberFormatException: Invalid number: 1st, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:1)

And the output with the attached patch applied:

Example source '(ns foo)

(def'
Exception message:
 EOF while reading, starting at line 3, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:3:1)

Example source ':foo}'
Exception message:
 Unmatched delimiter: }, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:6)

Example source 'foo'
Exception message:
 Unable to resolve symbol: foo in this context, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:14:1)

Example source 'clojure.core/firstt'
Exception message:
 No such var: clojure.core/firstt, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:15:1)

Example source '(nil 1)'
Exception message:
 Can't call nil, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:1)

Example source '("hi" 1)'
Exception message:
 java.lang.String cannot be cast to clojure.lang.IFn, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:1)

Example source '{:foo}'
Exception message:
 Map literal must contain an even number of forms, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:7)

Example source '1st'
Exception message:
 Invalid number: 1st, compiling:(/Users/jhume/Projects/clojure/clojure/temp.clj:1:1)





[CLJ-1315] Don't initialize classes when importing them Created: 28/Dec/13  Updated: 22/May/15  Resolved: 07/Oct/14

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

Type: Enhancement Priority: Critical
Reporter: Aaron Cohen Assignee: Unassigned
Resolution: Completed Votes: 9
Labels: aot, compiler, interop

Attachments: Text File 0001-Don-t-initialize-classes-during-import.patch    
Patch: Code
Approval: Ok

 Description   

Problem: When classes are imported in Clojure, the class is loaded using Class.forName(), which causes its static initialisers to be executed. This differs from Java where compilation does not cause classes to be loaded.

Motivation: In many cases when those classes are normally loaded by Java code during execution of a framework of some kind (IntelliJ in my case, and RoboVM is another culprit mentioned in that thread) the initialiser expects some infrastructure to be in place and will fail when it's not. This means that it's impossible to AOT compile namespaces importing these classes, which is a fairly serious limitation.

Approach: Modify ImportExpr to call RT.classForNameNonLoading() instead of Class.forName(), which will load the class but not initialise it. This change causes the Clojure test suite to fail, since clojure.test-clojure.genclass imports a gen-class'ed class which no longer loads its namespace on initialisation. I'm not sure if this is considered an incorrect use of such a class (IIRC with records it's required to import the class and require its namespace), but given that it's in the Clojure test case it's reasonable to assume that this fix would be a breaking change for more code out there. This test failure is also corrected in the attached patch.

Patch: 0001-Don-t-initialize-classes-during-import.patch

Screened by: Alex Miller - I have tested many open source Clojure projects with this change (particularly seeking out large, complicated, or known users of genclass/deftype/etc) and have found no projects adversely impacted. I know that Cursive has been running with this modification for a long time with no known issues. I am ok with unconditionally enabling this change (re the comment below). The impact is described in more detail in the suggested changelog diff in the comments below.

Alternative: This patch enables the change unconditionally, but depending on the extent of breakage it causes, it might need to be enabled with a configuration flag. I propose we make it unconditional in an early 1.7 beta and monitor the fall-out.

Background: This issue has been discussed in the following threads
https://groups.google.com/d/topic/clojure/tWSEsOk_pM4/discussion
https://groups.google.com/forum/#!topic/clojure-dev/qSSI9Z-Thc0



 Comments   
Comment by Alex Miller [ 29/Dec/13 12:23 PM ]

From original post:

This issue was originally reported by Zach Oakes and Colin Fleming and this patch was also tested by Colin.

I'm duplicating here my suggested release notes for this issue, which includes my current thoughts on potential breakage (it's also in the commit message of the patch):

    "import" no longer causes the imported class to be initialized. This
    change better matches Java's import behavior and allows the importing of
    classes that do significant work at initialization time which may fail.
    This semantics change is not expected to effect most code, but certain
    code may have depended on behavior that is no longer true.

    1) importing a Class defined via gen-class no longer causes its defining
    namespace to be loaded, loading is now deferred until first reference. If
    immediate loading of the namespace is needed, "require" it directly.
    2) Some code may have depended on import to initialize the class before it
    was used. It may now be necessary to manually call (Class/forName
    "org.example.Class") when initialization is needed. In most cases, this
    should not be necessary because the Class will be initialized
    automatically before first use.
Comment by Greg Chapman [ 13/May/14 6:25 PM ]

I'm not sure if this should also be fixed, but it would be nice if you could emit the code for a proxy of one of these non-initialized classes without forcing initialization. For example, the following raises an exception (I'm using Java 8):

Clojure 1.6.0
user=> (def cname "javafx.scene.control.ListCell")
#'user/cname
user=> (let [cls (Class/forName cname false (clojure.lang.RT/baseLoader))] (.importClass *ns* cls))
javafx.scene.control.ListCell
user=> (defn fails [] (proxy [ListCell] [] (updateItem [item empty] (proxy-super item empty))))
CompilerException java.lang.ExceptionInInitializerError, compiling:(NO_SOURCE_PATH:3:16)

The exception was ultimately caused by "IllegalStateException Toolkit not initialized", which javafx throws if you attempt to initialize a Control class outside of Application.launch.

Comment by Michael Blume [ 22/May/15 2:58 PM ]

Not sure if this should properly be considered a bug in Cloverage, but since this patch landed, I've been unable to get coverage in gen-class methods.

https://github.com/lshift/cloverage/issues/74





[CLJ-1736] Tweaks to changelog for 1.7 RC2 Created: 22/May/15  Updated: 22/May/15

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

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

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

 Description   

Just some minor tweaks to the changelog.



 Comments   
Comment by Nicola Mometto [ 22/May/15 11:37 AM ]

https://github.com/clojure/clojure/commit/69afe91ae07a4c75c34615a4af14327f98d78510#commitcomment-10670998





[CLJ-1735] Throwable->map is missing docstring and since Created: 22/May/15  Updated: 22/May/15

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

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

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

 Description   

Throwable->map is missing docstring and since






[CLJ-1360] clojure.string/split strips trailing delimiters Created: 18/Feb/14  Updated: 21/May/15

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

Type: Defect Priority: Major
Reporter: Tim McCormack Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None


 Description   

clojure.string/split and clojure.string/split-lines inherit the bizarre default behavior of java.lang.String#split(String,int) in stripping trailing consecutive delimiters:

(clojure.string/split "banana" #"an")
⇒ ["b" "" "a"]
(clojure.string/split "banana" #"na")
⇒ ["ba"]
(clojure.string/split "nanabanana" #"na")
⇒ ["" "" "ba"]

In the case of split-lines, processing a file line by line and rejoining results in truncation of trailing newlines in the file. In both cases, the behavior is surprising and cannot be inferred from the docstrings.

This behavior should either be fixed or documented.



 Comments   
Comment by Andy Fingerhut [ 18/Feb/14 10:51 AM ]

Probably documenting would be safer than changing the behavior at this point, given that some people may actually rely on the current behavior after testing, deploying, etc.

I don't currently have a suggestion for a modified doc string, but note that there are examples of this behavior and how one can use an extra "-1" limit argument at the end to get all split strings: http://clojuredocs.org/clojure_core/clojure.string/split

Comment by Crispin Wellington [ 21/May/15 10:46 PM ]

This bug just bit me. +1 to be fixed. If we just document and leave the behavior as is, then we have a surprising and inconsistent behaving split (why are inner empty values kept, but outer ones stripped?) that is different to every other split you've ever used. The optional -1 limit argument looks hacky but a fix could keep this -1 argument working.

EDIT: this looks to be java's string class behavior: http://stackoverflow.com/questions/2170557/split-method-of-string-class-does-not-include-trailing-empty-strings
Would be nice if limit defaulted to -1 on that type of clojure.string/split call.





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

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

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


 Description   

The following expression prints 1234 and returns 1:

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

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

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

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


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

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

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

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

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

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

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

Updated join code to be actually valid.

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

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

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




[CLJ-1706] top level conditional splicing ignores all but first element Created: 15/Apr/15  Updated: 21/May/15  Resolved: 21/May/15

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: reader

Attachments: Text File 0001-CLJ-1706-Make-top-level-reader-conditional-splicing-.patch     Text File clj-1706-2.patch     Text File clj-1706-3.patch     Text File clj-1706-make-error.patch    
Patch: Code and Test
Approval: Ok

 Description   
user=> (def a (java.io.PushbackReader. (java.io.StringReader. "#?@(:clj [1 2])")))
#'user/a
user=> (read {:read-cond :allow} a)
1
user=> (read {:read-cond :allow} a)
RuntimeException EOF while reading  clojure.lang.Util.runtimeException (Util.java:221)

Currently the reader is stateless (read is a static call) but utilizes a stateful reader (and has a few hooks into compiler/runtime state for autoresolving keywords, etc). If the call into the reader at the top level calls a splicing reader conditional, then only the first one will be returned. The remaining forms are stranded in the pendingForms list and will be lost for subsequent reads.

Approach: Make top level reader conditional splicing an error:

user=> (read-string {:read-cond :allow} "#?@(:clj [1 2])")
IllegalStateException Reader conditional splicing not allowed at the top level.  clojure.lang.LispReader.read (LispReader.java:200)

Patch: clj-1706-2.patch

Alternatives:

1. Make top-level reader conditional splicing an error and throw an exception. (SELECTED)

2. Allow the caller to pass in a stateful collection to catch or return the pendingForms. This changes the effective calling API for the reader. You would only need to do this in the cases where reader conditionals were allowed/preserved.

3. Add a static (or threadlocal?) pendingForms attribute to the reader to capture the pendingForms across calls. A static field would have concurrency issues - anyone using the reader across threads would get cross-talk in this field. The pendingForms could be threadlocal which would probably achieve separation in the majority of cases, but also creates a number of lifecycle questions about those forms. When do they get cleared or reset? What happens if reading the same reader happens across threads? Another option would be an identity map keyed by reader instance - would need to be careful about lifecycle management and clean up, as it's basically a cache.

4. Add more state into the reader itself to capture the pendingForms. The reader interfaces and hierarchy would be affected. This would allow the reader to stop passing the pendingForms around inside but modifies the interface in other ways. Again, this would only be needed for the specific case where reader conditionals are allowed so other uses could continue to work as is?

5. If read is going to exit with pendingForms on the stack, they could be printed and pushed back onto the reader. This adds new read/print roundtrip requirements on things at the top level of reader conditionals that didn't exist before.

6. Wrap spliced forms at the top level in a `do`. This seems to violate the intention of splicing reader conditional to read as spliced since it is not the same as if those forms were placed separately in the input stream.



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

pulling into 1.7 so we can discuss

Comment by Alex Miller [ 24/Apr/15 11:18 AM ]

Compiler.load() makes calls into LispReader.read() (static call). If the reader reads a top-level splicing conditional read form, that will read the entire form, then return the first spliced element. when LispReader.read() returns, the list carrying the other pending forms is lost.

I see two options:

1) Allow the compiler to call the LispReader with a mutable pendingForms list, basically maintaining that state across the static calls from outside the reader. makes the calling model more complicated and exposes the internal details of the pendingform stuff, but is probably the smaller change.

2) Enhance the LineNumberingPushbackReader in some way to remember the pending forms. This would probably allow us to remove the pending form stuff carried around throughout the LispReader and retain the existing (sensible) api. Much bigger change but probably better direction.

Comment by Nicola Mometto [ 24/Apr/15 11:24 AM ]

What about simply disallowing cond-splicing when top level?
Both proposed options are breaking changes since read currently only requires a java.io.PushbackReader

Comment by Alex Miller [ 24/Apr/15 11:42 AM ]

We want to allow top-level cond-splicing.

Comment by Nicola Mometto [ 24/Apr/15 11:52 AM ]

Would automatically wrapping a top-level cond-splicing in a (do ..) form be out of the question?

I'm personally opposed to supporting this feature as it would change the contract of c.c/read, complicate the implementation of LineNumberingPushbackReader or LispReader and complicate significantly the implementaion of tools.reader's reader types, for no significant benefit.
Is it really that important to be able to write

#~@(:clj [1 2])

rather than

(do #~@(:clj [1 2]))

?

Comment by Rich Hickey [ 18/May/15 10:10 AM ]

Please "Make top-level reader conditional splicing an error and throw an exception" for now.

Comment by Nicola Mometto [ 19/May/15 8:50 AM ]

Might be too late since Rich already gave the OK but the proposed patch doesn't prevent single-element top level conditional splicing forms.
e.g

;; this fails
#~@(:clj [1 2])
;; this works
#~@(:clj [1])

Is this intended?

Comment by Alex Miller [ 19/May/15 11:21 AM ]

New -2 patch catches reader conditional splice of 0 or 1 element.

Comment by Nicola Mometto [ 19/May/15 11:59 AM ]

Attached alternative patch that is less intrusive than clj-1706-2.patch

Comment by Alex Miller [ 19/May/15 2:54 PM ]

clj-1706-3.patch is identical to 0001-CLJ-1706Make-top-level-reader-conditional-splicing.patch but with one whitespace change reverted. Marking latest as screened.

Comment by Alex Miller [ 20/May/15 8:38 AM ]

Rich didn't like the dynvar in -3, so switching back to -2.





[CLJ-1232] Functions with non-qualified return type hints force import of hinted classes when called from other namespace Created: 18/Jul/13  Updated: 20/May/15

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

Type: Defect Priority: Major
Reporter: Tassilo Horn Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: compiler, typehints

Attachments: Text File 0001-auto-qualify-arglists-class-names.patch     Text File 0001-auto-qualify-arglists-class-names-v2.patch     Text File 0001-auto-qualify-arglists-class-names-v3.patch     Text File 0001-throw-on-non-qualified-class-names-that-are-not-auto.patch    
Patch: Code and Test
Approval: Vetted

 Description   

You can add a type hint to function arglists to indicate the return type of a function like so.

user> (import '(java.util List))
java.util.List
user> (defn linkedlist ^List [] (java.util.LinkedList.))
#'user/linkedlist
user> (.size (linkedlist))
0

The problem is that now when I call `linkedlist` exactly as above from another namespace, I'll get an exception because java.util.List is not imported in there.

user> (in-ns 'user2)
#<Namespace user2>
user2> (refer 'user)
nil
user2> (.size (linkedlist))
CompilerException java.lang.IllegalArgumentException: Unable to resolve classname: List, compiling:(NO_SOURCE_PATH:1:1)
user2> (import '(java.util List)) ;; Too bad, need to import List here, too.
java.util.List
user2> (.size (linkedlist))
0

There are two workarounds: You can import the hinted type also in the calling namespace, or you always use fully qualified class names for return type hints. Clearly, the latter is preferable.

But clearly, that's a bug that should be fixed. It's not in analogy to type hints on function parameters which may be simple-named without having any consequences for callers from other namespaces.

Approach: Make the compiler resolve the return tags when necessary (tag is not a string, primitive tag (^long) or array tag (^longs)) and update the Var's :arglist appropriately.

Patch: 0001-auto-qualify-arglists-class-names-v3.patch



 Comments   
Comment by Andy Fingerhut [ 16/Apr/14 3:47 PM ]

To make sure I understand, Nicola, in this ticket you are asking that the Clojure compiler change behavior so that the sample code works correctly with no exceptions, the same way as it would work correctly without exceptions if one of the workarounds were used?

Comment by Tassilo Horn [ 17/Apr/14 12:18 AM ]

Hi Andy. Tassilo here, not Nicola. But yes, the example should work as-is. When I'm allowed to use type hints with simple imported class names for arguments, then doing so for return values should work, too.

Comment by Rich Hickey [ 10/Jun/14 10:41 AM ]

Type hints on function params are only consumed by the function definition, i.e. in the same module as the import/alias. Type hints on returns are just metadata, they don't get 'compiled' and if the metadata is not useful to consumers in other namespaces, it's not a useful hint. So, if it's not a type in the auto-imported set (java.lang), it should be fully qualified.

Comment by Alex Miller [ 10/Jun/14 11:55 AM ]

Based on Rich's comment, this ticket should probably morph into an enhancement request on documentation, probably on http://clojure.org/java_interop#Java Interop-Type Hints.

Comment by Andy Fingerhut [ 10/Jun/14 3:13 PM ]

I would suggest something like the following for a documentation change, after this part of the text on the page Alex links in the previous comment:

For function return values, the type hint can be placed before the arguments vector:

(defn hinted
(^String [])
(^Integer [a])
(^java.util.List [a & args]))

-> #user/hinted

If the return value type hint is for a class that is outside of java.lang, which is the part auto-imported by Clojure, then it must be a fully qualified class name, e.g. java.util.List, not List.

Comment by Nicola Mometto [ 10/Jun/14 4:02 PM ]

I don't understand why we should enforce this complexity to the user.
Why can't we just make the Compiler (or even defn itself) update all the arglists tags with properly resolved ones? (that's what I'm doing in tools.analyzer.jvm)

Comment by Alexander Kiel [ 19/Jul/14 10:02 AM ]

I'm with Nicola here. I also think that defn should resolve the type hint according the imports of the namespace defn is used in.

Comment by Max Penet [ 22/Jul/14 7:06 AM ]

Same here, I was bit by this in the past. The current behavior is clearly counterintuitive.

Comment by Nicola Mometto [ 28/Aug/14 12:58 PM ]

Attached two patches implementing two different solutions:

  • 0001-auto-qualify-arglists-class-names.patch makes the compiler automatically qualify all the tags in the :arglists
  • 0001-throw-on-non-qualified-class-names-that-are-not-auto.patch makes the compiler throw an exception for all public defs whose return tag is a symbol representing a non-qualified class that is not in the auto-import list (approach proposed in IRC by Alex Miller)
Comment by Tassilo Horn [ 29/Aug/14 1:49 AM ]

For what it's worth, I'd prefer the first patch because the second doesn't help in situations where the caller lives in a namespace where the called function's return type hinted class is `ns-unmap`-ed. And there a good reasons for doing that. For example, Process is a java.lang class and Process is a pretty generic name. So in some namespace, I want to define my own Process deftype or defrecord. Without unmapping 'Process first to get rid of the java.lang.Process auto-import, I'd get an exception:

user> (deftype Process [])
IllegalStateException Process already refers to: class java.lang.Process in namespace: user  clojure.lang.Namespace.referenceClass (Namespace.java:140)

Now when I call some function from some library that has a `^Process` return type hint (meaning java.lang.Process there), I get the same exception as in my original report.

I can even get into troubles when only using standard Clojure functions because those have `^String` and `^Class` type hints. IMO, Class is also a pretty generic name I should be able to name my custom deftype/defrecord. And I might also want to have a custom String type/record in my astrophysics system.

Comment by Andy Fingerhut [ 30/Sep/14 4:39 PM ]

Not sure whether the root cause of this behavior is the same as the example in the description or not, but seems a little weird that even for fully qualified Java class names hinting the arg vector, it makes a difference whether it is done with defn or def:

Clojure 1.6.0
user=> (set! *warn-on-reflection* true)
true
user=> (defn f1 ^java.util.LinkedList [coll] (java.util.LinkedList. coll))
#'user/f1
user=> (def f2 (fn ^java.util.LinkedList [coll] (java.util.LinkedList. coll)))
#'user/f2
user=> (.size (f1 [2 3 4]))
3
user=> (.size (f2 [2 3 4]))
Reflection warning, NO_SOURCE_PATH:5:1 - reference to field size can't be resolved.
3
Comment by Alex Miller [ 30/Sep/14 6:21 PM ]

Andy, can you file that as a separate ticket?

Comment by Andy Fingerhut [ 30/Sep/14 9:08 PM ]

Created ticket CLJ-1543 for the issue raised in my comment earlier on 30 Sep 2014.

Comment by Andy Fingerhut [ 01/Oct/14 12:38 PM ]

Tassilo (or anyone), is there a reason to prefer putting the tag on the argument vector in your example? It seems that putting it on the Var name instead avoids this issue:

user=> (clojure-version)
"1.6.0"
user=> (set! *warn-on-reflection* true)
true
user=> (import '(java.util List))
java.util.List
user=> (defn ^List linkedlist [] (java.util.LinkedList.))
#'user/linkedlist
user=> (.size (linkedlist))
0
user=> (ns user2)
nil
user2=> (refer 'user)
nil
user2=> (.size (linkedlist))
0

I suppose that only allows a single type tag, rather than an independent one for each arity.

Comment by Tassilo Horn [ 02/Oct/14 3:16 AM ]

I wasn't aware of the fact that you can put it on the var's name. That's not documented at http://clojure.org/java_interop#Java Interop-Type Hints. But IMHO the documented version with putting the tag on the argument vector is more general since it supports different return type hints for the different arity version. In any case, if both forms are permitted then they should be equivalent in the case the function has only one arity.

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

Please work on the simplest patch that resolves the names

Comment by Alex Miller [ 16/Mar/15 4:34 PM ]

Nicola, in this:

if (tag != null &&
                        !(tag instanceof String) &&
                        primClass((Symbol)tag) == null &&
                        !tagClass((Symbol) tag).getName().startsWith("["))
                        {
                            argvec = (IPersistentVector)((IObj)argvec).withMeta(RT.map(RT.TAG_KEY, Symbol.intern(tagClass((Symbol)tag).getName())));
                        }

doesn't tagClass already handle most of these cases properly already? Can this be simplified? Is there an optimization case in avoiding lookup for a dotted name?

Comment by Nicola Mometto [ 16/Mar/15 5:10 PM ]

Patch 0001-auto-qualify-arglists-class-names-v2.patch avoids doing unnecessary lookups (dotted names, special tags (primitive tags, array tags)) and adds a testcase

Comment by Michael Blume [ 20/May/15 1:13 PM ]

I'm seeing an odd failure with this patch and hystrix defcommands, will post a small reproduction shortly

Comment by Michael Blume [ 20/May/15 1:20 PM ]

https://github.com/MichaelBlume/hystrix-demo

passes lein check with 1.7 beta3, fails with v3 patch applied

Comment by Nicola Mometto [ 20/May/15 1:40 PM ]

During analysis the compiler understands only arglists in the form of (quote ([..]*)) (see https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L557-L558), hystrix emits arglists in the form of (list (quote [..])*).

Not sure what to do about this.

Comment by Michael Blume [ 20/May/15 1:51 PM ]

Possibly just ask Hystrix not to do that?

Comment by Alex Miller [ 20/May/15 2:30 PM ]

test.generative uses non-standard arglists too. I haven't looked at the patch, but if it's sensitive to that, it's probably not good enough.

Comment by Nicola Mometto [ 20/May/15 6:51 PM ]

test.generative uses non-standard :tag, not :arglists

Comment by Alex Miller [ 20/May/15 10:22 PM ]

ah, yes. sorry.





[CLJ-1734] Display more descriptive error message when trying to use reader conditionals in a non-cljc file Created: 19/May/15  Updated: 20/May/15

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

Type: Enhancement Priority: Minor
Reporter: Daniel Compton Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: errormsgs, reader


 Description   

I spent a few puzzled minutes trying to understand the following message from the Clojure compiler:

CompilerException java.lang.RuntimeException: Conditional read not allowed, compiling: <filename>

Eventually I realised it was because I was trying to use reader conditionals in a .clj file that I hadn't renamed to cljc. I think it would be really helpful for people working in mixed clj and cljc codebases to have this error message extended to something like:

"Conditional read not allowed because file does not have extension .cljc"



 Comments   
Comment by Alex Miller [ 19/May/15 11:45 PM ]

The reader doesn't know this - it can be called in multiple ways (from repl, via clojure.core/read, via clojure.core/read-string, load/compile .cljc, load/compile .clj) so that description would actually be wrong in some of those. It seems like you're getting a pretty good error message already - it told you the problem and gave you the file name.

The message could be tweaked to something like "Reader conditionals not allowed in this context" which might give you a better clue.

Comment by Daniel Compton [ 20/May/15 3:12 PM ]

Perhaps I'm not understanding how the reader determines whether reader conditionals are allowed or not, but those would all seem to have different reasons for not being allowed and would be caught by different checks. Each of these checks could give a more specific warning explaining why the read wasn't allowed?

Counteracting my point, it looks like there is only one place where this exception is thrown - https://github.com/clojure/clojure/blob/7b9c61d83304ff9d5f9feddecf23e620c0b33c6e/src/jvm/clojure/lang/LispReader.java#L1406. I'm not sure if this could be extended to give more details in different error cases or if that information isn't available at that point?

Comment by Alex Miller [ 20/May/15 3:36 PM ]

The reader is invoked with an options map which will (or will not) have {:read-cond :allow} or {:read-cond :preserve}. That's the only info the reader has - if either of those is set and a reader conditional is encountered, it throws.

The compiler decides how to initialize these options when it's calling the reader. Users of read and read-string similarly decide which options are allowed when they call it. It would be possible to pass more info into the reader or to catch and rethrow in the compiler where more context is known, but both of those complicate the code for what is already a decent error imho.

Comment by Daniel Compton [ 20/May/15 4:03 PM ]

I agree it is a reasonable error message, I guess we can wait and see how other people find it once 1.7 is released. If it turns out to be an issue for lots of other people then we could revisit this then?





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

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

Type: Enhancement Priority: Critical
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 25
Labels: string

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

 Description   

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

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

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

Patch: clj-1449-more-v1.patch (draft version only – a more serious contender would incorporate Alex Miller's comments from Dec 2 2014)



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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

What about an implementation that works also in cljs?

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

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

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

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

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

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

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

Some things that would be helpful:

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

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

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

Alex, all your comments make sense.

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

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

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

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

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

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

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

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

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

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

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

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

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

Ruby uses "include?" for this.

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

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

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

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





[CLJ-1461] print-dup is broken for some clojure collections Created: 06/Jul/14  Updated: 19/May/15

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

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

Approval: Triaged

 Description   
user=> (print-dup (sorted-set 1) *out*)
#=(clojure.lang.PersistentTreeSet/create [1])nil
user=> (read-string (with-out-str (print-dup (sorted-set 1) *out*)))
ClassCastException Cannot cast clojure.lang.PersistentVector to clojure.lang.ISeq  java.lang.Class.cast (Class.java:3258)
user=> (print-dup (subvec [1] 0) *out*)
#=(clojure.lang.APersistentVector$SubVector/create [1])nil
user=> (read-string (with-out-str (print-dup (subvec [1] 0) *out*)))
IllegalArgumentException No matching method found: create  clojure.lang.Reflector.invokeMatchingMethod (Reflector.java:53)

print-dup assumes all IPersistentCollections not defined via defrecord have a static /create method that take an IPersistentCollection, but this is not true for many clojure collections






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

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

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

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

 Description   

This is more of the same problem as http://dev.clojure.org/jira/browse/CLJ-1315 (incorporated in 1.7-alphas) but in the specific case that the class with the static initialiser is used in a type hint



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

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

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

Patch won't apply to master for me

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

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





[CLJ-1620] Constants are leaked in case of a reentrant eval Created: 18/Dec/14  Updated: 19/May/15

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

Type: Defect Priority: Critical
Reporter: Christophe Grand Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: aot, compiler

Attachments: Text File 0001-CLJ-1620-avoid-constants-leak-in-static-initalizer.patch     Text File 0001-CLJ-1620-avoid-constants-leak-in-static-initalizer-v2.patch     Text File 0001-CLJ-1620-avoid-constants-leak-in-static-initalizer-v3.patch     Text File 0001-CLJ-1620-avoid-constants-leak-in-static-initalizer-v4.patch     Text File clj-1620-v5.patch     Text File eval-bindings.patch    
Patch: Code
Approval: Incomplete

 Description   

Compiling a function that references a non loaded (or uninitialized) class triggers its init static. When the init static loads clojure code, some constants (source code I think) are leaked into the constants pool of the function under compilation.

It prevented CCW from working in some environments (Rational) because the static init of the resulting function was over 64K.

Steps to reproduce:

Load the leak.main ns and run the code in comments: the first function has 15 extra fields despite being identical to the second one.

(ns leak.main)

(defn first-to-load []
  leak.Klass/foo)

(defn second-to-load []
  leak.Klass/foo)

(comment
=> (map (comp count #(.getFields %) class) [first-to-load second-to-load])
(16 1)
)
package leak;
 
import clojure.lang.IFn;
import clojure.lang.RT;
import clojure.lang.Symbol;
 
public class Klass {
  static {
    RT.var("clojure.core", "require").invoke(Symbol.intern("leak.leaky"));
  }
  public static IFn foo = RT.var("leak.leaky", "foo");
}
(ns leak.leaky)

(defn foo
  "Some doc"
  []
  "hello")

(def unrelated 42)

https://gist.github.com/cgrand/5dcb6fe5b269aecc6a5b#file-main-clj-L10

Patch: clj-1620-v5.patch



 Comments   
Comment by Christophe Grand [ 18/Dec/14 3:56 PM ]

Patch from Nicola Mometto

Comment by Nicola Mometto [ 18/Dec/14 4:01 PM ]

Attached the same patch with a more informative better commit message

Comment by Laurent Petit [ 18/Dec/14 4:03 PM ]

I'd like to thank Christophe and Alex for their invaluable help in understanding what was happening, formulating the right hypothesis and then finding a fix.

I would also mention that even if non IBM rational environments where not affected by the bug to the point were CCW would not work, they were still affected. For instance the class for a one-liner function wrapping an interop call weighs 700bytes once the patch is applied, when it weighed 90kbytes with current 1.6 or 1.7.

Comment by Laurent Petit [ 18/Dec/14 5:07 PM ]

In CCW for the initial problematic function, the -v2 patch produces exactly the same bytecode as if the referenced class does not load any namespace in its static initializers.
That is, the patch is valid. I will test it live in the IBM Rational environment ASAP.

Comment by Laurent Petit [ 19/Dec/14 12:10 AM ]

I confirm the patch fixes the issue detected initially in the IBM Rational environment

Comment by Michael Blume [ 06/Jan/15 4:03 PM ]

I have absolutely no idea why, but if I apply this patch, and the patch for CLJ-1544 to master, and then try to build a war from this test project https://github.com/pdenhaan/extend-test I get a scary-looking traceback:

$ lein do clean, war!
Exception in thread "main" java.lang.NoSuchFieldError: __thunk__0__, compiling:(route.clj:1:1)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3606)
	at clojure.lang.Compiler.compile1(Compiler.java:7299)
	at clojure.lang.Compiler.compile1(Compiler.java:7289)
	at clojure.lang.Compiler.compile(Compiler.java:7365)
	at clojure.lang.RT.compile(RT.java:398)
	at clojure.lang.RT.load(RT.java:438)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5362.invoke(core.clj:5668)
	at clojure.core$load_lib.doInvoke(core.clj:5667)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5706)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5789)
	at clojure.lang.RestFn.invoke(RestFn.java:436)
	at extend_test.core.handler$loading__5301__auto____66.invoke(handler.clj:1)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3601)
	at clojure.lang.Compiler.compile1(Compiler.java:7299)
	at clojure.lang.Compiler.compile1(Compiler.java:7289)
	at clojure.lang.Compiler.compile(Compiler.java:7365)
	at clojure.lang.RT.compile(RT.java:398)
	at clojure.lang.RT.load(RT.java:438)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5362.invoke(core.clj:5668)
	at clojure.core$load_lib.doInvoke(core.clj:5667)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5706)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5789)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at extend_test.core.servlet$loading__5301__auto____7.invoke(servlet.clj:1)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3601)
	at clojure.lang.Compiler.compile1(Compiler.java:7299)
	at clojure.lang.Compiler.compile1(Compiler.java:7289)
	at clojure.lang.Compiler.compile1(Compiler.java:7289)
	at clojure.lang.Compiler.compile(Compiler.java:7365)
	at clojure.lang.RT.compile(RT.java:398)
	at clojure.lang.RT.load(RT.java:438)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$compile$fn__5420.invoke(core.clj:5834)
	at clojure.core$compile.invoke(core.clj:5833)
	at user$eval5.invoke(form-init180441230737245034.clj:1)
	at clojure.lang.Compiler.eval(Compiler.java:6776)
	at clojure.lang.Compiler.eval(Compiler.java:6765)
	at clojure.lang.Compiler.eval(Compiler.java:6766)
	at clojure.lang.Compiler.load(Compiler.java:7203)
	at clojure.lang.Compiler.loadFile(Compiler.java:7159)
	at clojure.main$load_script.invoke(main.clj:274)
	at clojure.main$init_opt.invoke(main.clj:279)
	at clojure.main$initialize.invoke(main.clj:307)
	at clojure.main$null_opt.invoke(main.clj:342)
	at clojure.main$main.doInvoke(main.clj:420)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at clojure.lang.Var.invoke(Var.java:383)
	at clojure.lang.AFn.applyToHelper(AFn.java:156)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: java.lang.NoSuchFieldError: __thunk__0__
	at instaparse.core__init.load(Unknown Source)
	at instaparse.core__init.<clinit>(Unknown Source)
	at java.lang.Class.forName0(Native Method)
	at java.lang.Class.forName(Class.java:344)
	at clojure.lang.RT.loadClassForName(RT.java:2141)
	at clojure.lang.RT.load(RT.java:430)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5362.invoke(core.clj:5668)
	at clojure.core$load_lib.doInvoke(core.clj:5667)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5706)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5789)
	at clojure.lang.RestFn.invoke(RestFn.java:436)
	at clout.core$loading__5301__auto____273.invoke(core.clj:1)
	at clout.core__init.load(Unknown Source)
	at clout.core__init.<clinit>(Unknown Source)
	at java.lang.Class.forName0(Native Method)
	at java.lang.Class.forName(Class.java:344)
	at clojure.lang.RT.loadClassForName(RT.java:2141)
	at clojure.lang.RT.load(RT.java:430)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5362.invoke(core.clj:5668)
	at clojure.core$load_lib.doInvoke(core.clj:5667)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5706)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5789)
	at clojure.lang.RestFn.invoke(RestFn.java:482)
	at compojure.core$loading__5301__auto____68.invoke(core.clj:1)
	at compojure.core__init.load(Unknown Source)
	at compojure.core__init.<clinit>(Unknown Source)
	at java.lang.Class.forName0(Native Method)
	at java.lang.Class.forName(Class.java:344)
	at clojure.lang.RT.loadClassForName(RT.java:2141)
	at clojure.lang.RT.load(RT.java:430)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5415.invoke(core.clj:5823)
	at clojure.core$load.doInvoke(core.clj:5822)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5613)
	at clojure.core$load_lib$fn__5362.invoke(core.clj:5668)
	at clojure.core$load_lib.doInvoke(core.clj:5667)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$load_libs.doInvoke(core.clj:5706)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:628)
	at clojure.core$require.doInvoke(core.clj:5789)
	at clojure.lang.RestFn.invoke(RestFn.java:457)
	at compojure.route$loading__5301__auto____1508.invoke(route.clj:1)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.AFn.applyTo(AFn.java:144)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3601)
	... 75 more
Subprocess failed
Comment by Michael Blume [ 06/Jan/15 4:06 PM ]

https://github.com/MichaelBlume/clojure/tree/no-field
https://github.com/MichaelBlume/extend-test/tree/no-field

mvn clean install in the one, lein ring uberwar in the other.

Comment by Nicola Mometto [ 06/Jan/15 6:09 PM ]

Michael, thanks for the report, I've tried investigating this a bit but the big amount of moving parts involved make it really hard to figure out why the combination of the two patches causes this issue.

A helpful minimal case would require no lein and no external dependencies, I'd appreciate some help in debugging this issue if anybody has time.

Comment by Michael Blume [ 06/Jan/15 10:56 PM ]

Ok, looks like the minimal case is

(ns foo (:require [instaparse.core]))

(ns bar (:require [foo]))

and then attempt to AOT-compile both foo and bar.

I don't yet know what's special about instaparse.core.

Comment by Michael Blume [ 06/Jan/15 11:30 PM ]

Well, not a minimal case, of course, but one without lein, at least.

Comment by Michael Blume [ 06/Jan/15 11:51 PM ]

ok, problem is instaparse's defclone macro, I've extracted it to a test repo

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

lein do clean, compile will get you a failure, but the repo has no dependencies so I'm sure there's a way to do that without lein.

Comment by Ghadi Shayban [ 06/Jan/15 11:56 PM ]

Sorry for the barrage of questions, but these classloader bugs are subtle (and close to being solved I hope). Your report is immensely valuable, and yet it will help to be even more specific. There are a cluster of these bugs – and keeping them laser-focused is key.

The minimal case to which you refer is the NoSuchFieldError?
How are is this being invoked this without lein?
What are you calling to AOT? (compile 'bar) ?
What is the classpath? When you invoke originally, is ./target/classes empty?
Does the problem go away with CLJ-979-7 applied?

Comment by Michael Blume [ 07/Jan/15 12:16 AM ]

I have tried and failed to replicate without leiningen. When I just run

java -Dclojure.compile.path=target -cp src:../clojure/target/clojure-1.7.0-aot-SNAPSHOT.jar clojure.lang.Compile thunk-fail.first thunk-fail.second

everything works fine.

Comment by Ghadi Shayban [ 07/Jan/15 12:30 AM ]

The NoSuchFieldError is related to the keyword lookup sites.

Replacing defclone's body with
`(do (:foo {})) is enough to trigger it, with the same ns structure.

Comment by Nicola Mometto [ 07/Jan/15 4:47 AM ]

I have updated the patch for CLJ-1544, now the combination of the new patch + the patch from this ticket should not cause any exception.

That said, a bug in this patch still exists since while the patch for CLJ-1544 had a bug, it was causing a perfectly valid (albeit hardly reproducible) compilation scenario so we should keep debugging this patch with the help of the bugged patch for CLJ-1544.

I guess the first thing to do is figure out what lein compile is doing differently than clojure.Compile

Comment by Nicola Mometto [ 07/Jan/15 4:49 AM ]

Also Ghadi is right, infact replacing the whole body of thunk-fail.core with (:foo {}) is enough.

It would seem like the issue is with AOT (re)compiling top-level keyword lookup sites, my guess is that for some reason this patch is preventing correct generation of the __init static initializer.

Comment by Nicola Mometto [ 07/Jan/15 5:35 AM ]

I still have absolutely no idea what lein compile is doing but I figured out the issue.
The updated patch binds (in eval) the appropriate vars only when already bounded.

Comment by Alex Miller [ 07/Jan/15 9:00 AM ]

Would it be worth using transients on the bindings map now?

Comment by Nicola Mometto [ 07/Jan/15 9:11 AM ]

Makes sense, updated the patch to use a transient map

Comment by Michael Blume [ 07/Jan/15 12:25 PM ]

Is there a test we can add that'll fail in the presence of the v2 patch? preferably independent of the CLJ-1544 patch? I can try to write one myself, but I don't have a lot of familiarity with the Clojure compiler internals.

Comment by Nicola Mometto [ 07/Jan/15 12:32 PM ]

I'll have to think about a way to reproduce that bug, it's not a simple scenario to reproduce.
It involves compiling a namespace from an evaluated context.

Comment by Laurent Petit [ 15/Apr/15 11:14 AM ]

Hello, is there any chance left that this issue will make it to 1.7 ?

Comment by Alex Miller [ 15/Apr/15 11:18 AM ]

Wasn't planning on it - what's the impact for you?

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

The impact is that I need to use a patched version of Clojure for CCW.
While it's currently not that hard to follow clojure's main branch and regularly rebase on it or reapply the patch, it's still a waste of time.

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

I will check with Rich whether it can be screened for 1.7 before we get to RC.

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

same as v4 patch, but just has more diff context

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

the file mentioned in the patch field is not the right one IMHO

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

which one is?

Comment by Laurent Petit [ 01/May/15 8:58 AM ]

I think you previous comment relates to clj-1620-v5.patch, but at the end of the description there's the following line:

Patch: 0001-CLJ-1620-avoid-constants-leak-in-static-initalizer-v4.patch

Comment by Alex Miller [ 01/May/15 9:30 AM ]

Those patches are equivalent with respect to the change they introduce; they just differ in how much diff context they have.

Comment by Alex Miller [ 18/May/15 2:25 PM ]

Rich has ok'ed screening this one for 1.7 but I do not feel that I can mark it screened without understanding it much better than I do. The description, code, and cause information here is not sufficient for me to understand what the problem actually is or why the fix is the right one. The fix seems to address the symptom but I worry that it is just a symptom and that a better understanding of the actual cause would lead to a different or better fix.

The evolution of the patches was driven by bugs in CLJ-1544 (a patch which has been pulled out for being suspect for other reasons). Starting fresh, were those modifications necessary and correct?

Why does this set of vars need to push clean impls into the bindings? Why not some of the other vars (like those pushed in load())? The set chosen here seems to match that from the ReifyParser - why? Why should they only be pushed if they are bound (that is, why is "not bound" not the same as "bound but empty")? Are we affecting performance?

Popping all the way out, is the thing being done by CCW even a thing that should be doable? The description says "Compiling a function that references a non loaded (or uninitialized) class triggers its init static" - should this load even happen? Can we get an example that actually demonstrates what CCW was doing originally?

Comment by Laurent Petit [ 19/May/15 7:12 AM ]

Alex, the question of "should what CCW is doing be doable" can be answered if you answer it on the given example, I think.

The question "should the initialization of the class occur when it could just be loaded" is a good one. Several reports have been made on the Clojure list about this problem, and I guess there is at least one CLJ issue about changing some more classForName into classForNameNonLoading here and there in Clojure.
For instance, it prevents referencing java classes which have code in their static initializers as soon as the code does some supposition about the runtime it is initialized in. This is a problem with Eclipse / SWT, this a problem with Cursive as I remember Colin mentioning a similar issue. And will probably is a problem that can appear each time one tries to AOT compile clojure code interoperating with java classes who happen to have, somewhere within static initializers triggered by the compilation (and this is transitive), assumptions that they are initialized in the proper target runtime environment.

What I don't know is if preventing the initialization to occur in the first place would be sufficient to get rid of the class of problems this bug and the proposed patch tried to solve. I do not claim to totally what is happening either (Christophe and Nicolas were of great help to analyze the issue and create the patch), but as I understand it, it's a kind of "Inception-the-movie-like" bug. Compiling a fn which triggers compiling another fn (here through the loading of clojure namespaces via a java initializer).

If preventing the initialization of class static methods when they are referenced (through interop calls - constructor, field, method, static field, static method-) is the last remaining bit that could cause such "compilation during compilation" scenario, then yes, protecting the compilation process like Nicolas tried to do may not be necessary, and just fixing the undesired loading may be enough.





[CLJ-1669] Move LazyTransformer to an iterator strategy, extend eduction capabilities Created: 04/Mar/15  Updated: 19/May/15  Resolved: 19/May/15

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

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

Attachments: Text File clj-1669-2.patch     Text File clj-1669-3.patch     Text File clj-1669-4.patch     Text File clj-1669-5.patch     Text File clj-1669-6.patch     Text File clj-1669.patch    
Patch: Code and Test
Approval: Ok

 Description   
  • LazyTransformer does a lot of work to be a seq. Instead, switch to creating a transforming iterator.
  • Change sequence to wrap iterator-seq around the transforming iterator.
  • Change the iterator-seq implementation to be chunked. IteratorSeq will no longer be used but is left in case of regressions for now.
  • Change Eduction to provide iteration directly via the transforming iterator.
  • Extend eduction to support multiple xforms.

Performance:

Compared:

  • alpha5 == before any change
  • alpha6 == after clj-1669-6.patch was applied
  • beta3 == latest, includes range enhancement, expanding mapcat enhancement, etc

;; using java 1.8
(use 'criterium.core)
(def s (range 1000))
(def v (vec s))
(def s50 (range 50))
(def v50 (vec s50))

expr alpha5 s alpha5 v alpha6 s alpha6 v beta3 s beta3 v
non-chunking transform            
(into [] (->> s (interpose 5) (partition-all 2))) 432 us 437 us 413 us 411 us 353 us 414 us
(into [] (->> s (eduction (interpose 5) (partition-all 2)))) * 117 us 118 us 117 us 113 us 116 us 113 us
1 chunking transform            
(into [] (map inc s)) 43 us 45 us 35 us 31 us 32 us 36 us
(into [] (map inc) s) 19 us 19 us 18 us 18 us 18 us 16 us
(into [] (sequence (map inc) s)) 100 us 54 us 97 us 65 us 66 us 64 us
(into [] (eduction (map inc) s)) 24 us 19 us 24 us 20 us 20 us 19 us
(doall (map inc (eduction (map inc) s))) 219 us 203 us 98 us 78 us 93 us 89 us
2 chunking transforms        
(into [] (map inc (map inc s))) 53 us 56 us 53 us 54 us 61 us 58 us
(into [] (comp (map inc) (map inc)) s) 13 us 26 us 30 us 26 us 31 us 31 us
(into [] (sequence (comp (map inc) (map inc)) s)) 111 us 64 us 98 us 73 us 83 us 80 us
(into [] (eduction (map inc) (map inc) s)) * 58 us 31 us 58 us 31 us 30 us 31 us
(doall (map inc (eduction (map inc) (map inc) s))) * 240 us 212 us 114 us 93 us 105 us 102 us
expand transform            
(into [] (mapcat range (map inc s50))) 74 us 73 us 67 us 68 us 37 us 39 us
(into [] (sequence (comp (map inc) (mapcat range)) s50)) 111 us 102 us 166 us 159 us 99 us 98 us
(into [] (eduction (map inc) (mapcat range) s50)) * 65 us 64 us 57 us 56 us 27 us 27 us
materialized eduction            
(sort (eduction (map inc) s)) ERR ERR 99 us 77 us 77 us 77 us
(->> s (filter odd?) (map str) (sort-by last)) 1.10 ms 1.25 ms 1.15 ms 1.19 ms 1.14 ms 1.13 ms
(->> s (eduction (filter odd?) (map str)) (sort-by last)) ERR ERR 1.18 ms 1.15 ms 1.13 ms 1.13 ms
  • used comp to combine xforms as eduction only took one in the before case

Patch: clj-1669-6.patch

Screened by:



 Comments   
Comment by Michael Blume [ 05/Mar/15 3:52 PM ]

Nice, I like the direction on this.

CLJ-1515 currently breaks this patch (LongRange cannot be converted to Iterable), but I imagine that'll get better when it absorbs the changes from CLJ-1603

Comment by Alex Miller [ 06/Mar/15 8:11 AM ]

Yeah. colls should be mapped through RT.iter() to catch more cases.

Comment by Alex Miller [ 06/Mar/15 9:52 AM ]

To do:

  • remove Seqable from Eduction
  • support Iterable in RT.toArray()
  • more eduction pipeline tests that require realization at end
Comment by Alex Miller [ 06/Mar/15 1:00 PM ]

Perf numbers show pretty worse results from sequence, will dig in further.

Comment by Alex Miller [ 13/Mar/15 7:41 AM ]

For the s timings, we've actually introduced more steps into the stack:

OLD reduce with s:

LazyTransformer
   seq (range) - every transformation is another layer here

NEW reduce with s:

IteratorSeq 
  TransformingIterator (handles N transformations in 1 step)
    SeqIterator
      seq (range)
Comment by Alex Miller [ 20/Mar/15 10:08 AM ]

Look at perf for:

  • ->> eduction transformation
  • transformation comparison that doesn't support chunking
  • more into vector iteration case
Comment by Alex Miller [ 21/Mar/15 8:45 AM ]

The -5 patch is same -3 except all uses of IteratorSeq have been replaced with a ChunkedCons that is effectively a chunked version of the old IteratorSeq. While no one calls it, I left IteratorSeq in the code base in case of regression.

Generally, the chunked iterator seq reduces the cost in a number of the worst cases and also is a clear benefit in making seqs over a result of eduction or sequence faster to traverse (as they are now chunked).

I think the one potential issue is that seqs over iterators are now chunked when they were not before which could change programs that expect their stateful iterator to be traversed one at a time. This change could be isolated to just to sequence and seq-iterator and mitigated by not changing RT.seqFrom() and seq-iterator to use the new chunking behavior only in sequence and/or with a new chunked-iterator-seq to make it more explicit. The sequence over xf is new so no possible regression there, everything else would just be opt-in.

Comment by Rich Hickey [ 27/Mar/15 9:49 AM ]

push as is but leave unresolved, for perf tweaks

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

clj-1669-6 is identical to clj-1669-5 but removes two commented out debugging lines that were inadvertently included.

Comment by Alex Miller [ 18/May/15 9:47 AM ]

updated for beta3 numbers





[CLJ-1717] Compiler casts System properties to String without prior type check Created: 29/Apr/15  Updated: 18/May/15  Resolved: 18/May/15

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

Type: Defect Priority: Critical
Reporter: Laurent Petit Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: compiler, regression
Environment:

occurs for any JVM / Operating System. Encountered when using Clojure inside the Equinox OSGi framework (reference OSGi implementation used e.g. by Eclipse IDE)


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

 Description   

The Clojure Compiler loops over all System Properties through their EntrySet.
This interface allows for non-String properties to be found. It is leveraged by the Equinox OSGi framework (reference OSGi implementation used e.g. by the Eclipse IDE).

This means that since this new code has been introduced in the Clojure compiler, the official Clojure Jar cannot be used inside Eclipse.

The problem is that Counterclockwise, the Eclipse-based Clojure IDE, at least, is affected, since it is developed with Clojure code.

The attached patch solves the issue by skipping System Properties key/value pairs whose values aren't Strings.



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

Probably a regression related to CLJ-1274.

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

Yes, CLJ-1274 moved the code from the Compile.java class to the Compiler.java class. The code already had the cast problem, but it was probably not an issue at runtime for CCW when only present in Compile.java

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

It looks like maybe this is considered a bug in Eclipse land (which it probably should be)?
https://bugs.eclipse.org/bugs/show_bug.cgi?id=445122

Comment by Laurent Petit [ 29/Apr/15 3:21 PM ]

I agree that it is an abuse on the side of Eclipse-and of a System properties hole that allows to put non-String values and then browse them through methods not using Generics.

From the last comments, it appears that it has only be released very recently, and for only the last stable version of Eclipse. So I expect CCW users with disparate Eclipse versions installed to have the problem if I do not apply the patch to my custom version of Clojure for some more months.

I can live with that if you reject the issue.

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

What is the status for this issue? I understand that being considered an abuse from the OSGi implementation of the System properties contract, you might not want to add code for dealing with this in Compiler.
On the other hand, it's a small 3-lines check with an explanation of why it's done that way, so...

Anyway, feel free to get rid of it and close it so it doesn't get in the way if you think it's not worth the trouble.

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

We haven't had a chance to discuss it further yet. There is reluctance to change it if it's not necessary.





[CLJ-1729] Make Counted and count() return long instead of integer Created: 12/May/15  Updated: 18/May/15

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

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


 Description   

Currently count() returns an int - should bump that up to a long.

On long overflow, count() should throw ArithmeticException. Also see CLJ-1229.






[CLJ-1398] Update URLs in javadoc.clj Created: 02/Apr/14  Updated: 17/May/15

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

Type: Enhancement Priority: Trivial
Reporter: Eli Lindsey Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File 0001-update-apache-commons-javadoc-location.patch     Text File 0002-add-javadoc-lookup-for-guava-and-apache-commons-lang.patch     Text File 0003-add-javadoc-lookup-for-jdk8.patch    
Patch: Code
Approval: Triaged

 Description   

Three minor fixes/enhancements to javadoc.clj:

0001 corrects the URLs for apache commons javadoc (the ones used in javadoc.clj no longer resolve).
0002 adds javadoc lookup for guava and apache commons lang3.
0003 adds javadoc lookup for jdk8.

(Note: contributor agreement is in the mail)



 Comments   
Comment by Andy Fingerhut [ 04/Apr/14 11:22 AM ]

Eli, thanks for the patches. It appears that you are not currently on the list of Clojure contributors here: http://clojure.org/contributing

It is the policy of the Clojure team only to incorporate patches submitted by people who have signed and submitted a Clojure CA. Were you interested in doing that?

Comment by Eli Lindsey [ 04/Apr/14 11:27 AM ]

> It is the policy of the Clojure team only to incorporate patches submitted by people who have signed and submitted a Clojure CA. Were you interested in doing that?

Yup! I mailed off the CA to Rich on Wednesday when this was filed; should be arriving shortly.

Comment by Eli Lindsey [ 09/May/14 8:18 PM ]

Just to note - Clojure CA went through and I'm listed on the contributors page now.





[CLJ-1731] Transient maps can't be assoc!'d to contain more than 8 elements. Created: 17/May/15  Updated: 17/May/15  Resolved: 17/May/15

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

Type: Task Priority: Major
Reporter: Peter Herniman Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: assoc!, transient
Environment:

Linux, Fedora 20
Clojure 1.6.0
OpenJDK 64-Bit Server VM 1.7.0_79-mockbuild_2015_04_15_06_33-b00



 Description   

(let [x (transient {})] (dotimes [i 30] (assoc! x i 0)) (persistent! x))
Will result in

{0 0, 1 0, 2 0, 3 0, 4 0, 5 0, 6 0, 7 0}

instead of the expected 30 element map.

I'm not sure if this is fixed in the most recent version (development) but it doesn't work in 1.6.0.



 Comments   
Comment by Alex Miller [ 17/May/15 6:54 AM ]

This is not correct usage of transient maps. You must use the return value of assoc! for further updates, not bash the same instance. In other words, the update model is the same as with normal maps. There is an outstanding ticket to tweak the docstring slightly to make this clearer.

See a similar example with transient vectors at http://clojure.org/transients.

Comment by Peter Herniman [ 17/May/15 6:23 PM ]

Ah ok, looking at the example on clojure.org clears things up a lot.
I'm not too sure if the docstring does need updating, the tutorial on transients does state that you need to capture the return value explicitly.
Thanks!





[CLJ-1732] Add docstring explanation of (isa? [x1 x2...] [parent1 parent2...]) Created: 17/May/15  Updated: 17/May/15

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

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

Approval: Triaged

 Description   

The "Multimethods and Hierarchies" page mentions that "isa?" has special behavior when aimed at two vectors[1]. But the docstring of "isa?" does not mention it[2].

[1] http://clojure.org/multimethods
[2] http://clojure.github.io/clojure/clojure.core-api.html#clojure.core/isa?






[CLJ-1278] State function's unmunged full name in compiled function's toString() Created: 10/Oct/13  Updated: 17/May/15

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

Type: Enhancement Priority: Major
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 9
Labels: errormsgs, interop

Attachments: Text File CLJ-1278-2.patch     Text File CLJ-1528--function-tostring.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Currently function instances print their toString() with the munged Java name:

user=> (ns proj.util-fns)
nil
proj.util-fns=> (defn a->b [a] (inc a))
#'proj.util-fns/a->b
proj.util-fns=> a->b
#object[proj.util_fns$a__GT_b 0x141ba1f1 "proj.util_fns$a__GT_b@141ba1f1"]

For debugging purposes, it would be useful to have the function toString() describe the Clojure-oriented fn name.

Approach: Store the original name in the function instance and use it in the toString() rather than returning the class name.

proj.util-fns=> a->b
#object[proj.util_fns$a__GT_b 0x47d1a507 "proj.util-fns/a->b(NO_SOURCE_FILE:2)"]

Tradeoffs: Increased function instance size for the function name.

Patch: CLJ-1278-2.patch



 Comments   
Comment by Howard Lewis Ship [ 10/Oct/13 8:39 PM ]

Contains changes and updated tests. I don't have any details on if this affects compiler performance or generated code size in any significant or even measurable way.

Comment by Andy Fingerhut [ 11/Oct/13 4:06 PM ]

Howard, sorry I do not have more useful comments on the changes you make in your patch. Right now I only have a couple of minor comments on its form. The preferred format for patches is that created using the instructions shown on this wiki page: http://dev.clojure.org/display/community/Developing+Patches

Also, there are several parts of your patch that appear to only make changes in the whitespace of lines. It would be best to leave such changes out of a proposed patch.

Comment by Howard Lewis Ship [ 11/Oct/13 5:00 PM ]

Yes, I didn't notice the whitespace changes until after; I must have hit reformat at some point, despite my best efforts. I'll put together a new patch shortly.

Comment by Howard Lewis Ship [ 11/Oct/13 6:26 PM ]

Clean patch

Comment by Howard Lewis Ship [ 25/Nov/14 6:00 PM ]

FYI, it's been a year. The correct file is CLJ-1278-2.patch.

Comment by Howard Lewis Ship [ 25/Nov/14 6:07 PM ]

... hm, something's changed in recent times.

     [java] FAIL in (fn-toString) (fn.clj:83)
     [java] nested functions
     [java] expected: (= (simple-name (.toString (factory-function))) (str "clojure.test-clojure.fn/" "factory-function/fn"))
     [java]   actual: (not (= "clojure.test-clojure.fn/factory-function/fn__7565" "clojure.test-clojure.fn/factory-function/fn"))
     [java]
     [java] FAIL in (fn-toString) (fn.clj:83)
     [java] nested functions
     [java] expected: (= (simple-name (.toString (named-factory-function))) (str "clojure.test-clojure.fn/" "named-factory-function/a-function-name"))
     [java]   actual: (not (= "clojure.test-clojure.fn/named-factory-function/a-function-name__7568" "clojure.test-clojure.fn/named-factory-function/a-function-name"))

I'd be willing to update my patch if there's any indication that it will ever be picked up. It's been over a year since last update.

Comment by Andy Fingerhut [ 26/Nov/14 10:30 AM ]

The change in behavior you are seeing is most likely due to a fix for ticket CLJ-1330.

And in case you were wondering, no, I am not the person who knows what tickets are of interest. I know that this one has gotten a fair number of votes, and by votes is one of the top ranked enhancement suggestions - look under "enhancements" on this report, or search for 1330: http://jafingerhut.github.io/clj-ticket-status/CLJ-top-tickets-by-weighted-vote.html

The features going into Clojure 1.7 are pretty well decided upon, and a fair number of other fixes and enhancements were delayed to 1.8. A longer than 1 year wait is not unusual, especially for enhancements.

Comment by Howard Lewis Ship [ 26/Nov/14 3:06 PM ]

Thanks for the info; don't want to come off as whiny but The Great Silence is off putting to someone who wants to help improve things.

I'll update my patch, and hope to see some motion for 1.8.

Comment by Alex Miller [ 26/Nov/14 3:43 PM ]

There are ~400 open tickets for Clojure. As a printing enhancement, this is generally considered lower priority than defects. Additionally, the proposal changes the compiler, bytecode generation code, and adds fields to generated objects, which has unassessed and potentially wide impacts. The combination of these things means it might be a while before we get around to looking at it.

Things that you could do to help:
1) Simplify the description. Someone coming to this ticket (screeners and ultimately Rich) want to look at the description and get the maximal understanding with the minimal effort. We have some guidelines on this at http://dev.clojure.org/display/community/Creating+Tickets if you haven't seen it. For an enhancement, a short (1-2 sentence) description of the problem and an example I can run in the repl is best. Then a proposal (again, as short as possible). Examples: CLJ-1529, CLJ-1325, CLJ-1378. For an enhancement like this, seeing (succinct) before/after versions where a user will see this is often the quickest way for a screener to understand the benefit.

2) Anticipate and remove blockers. As I mentioned above, you are changing the size of every function object. What is the impact on size and construction time? Providing data and/or a test harness saves a screener from doing this work. It's better to leave details in attachments or comments and refer to it in the description if it's lengthy.

3) Have others screen (per http://dev.clojure.org/display/community/Screening+Tickets ) - while that is the job a screener (often me) will have to re-do, having more eyeballs on it early helps. Ask on #clojure for someone else to take a look, try it, etc. If there are open questions, leaving those in the description helps guide my work.

Comment by Howard Lewis Ship [ 26/Nov/14 4:09 PM ]

Alex, thanks for the advice. I'll follow through. Some of that data is already present, but I can make it more prominent.

I know that I'm overwhelmed by the number of issues (including enhancements and minor improvements) on the Tapestry issue list, so I'm understanding of problem space.

Comment by Steve Miner [ 17/May/15 9:06 AM ]

You could instead implement toString() on something like AFn.java.

public String toString() {
    String name = getClass().getSimpleName();
    return Compiler.demunge(name);
}
Comment by Alex Miller [ 17/May/15 11:06 AM ]

Munge+demunge is a lossy operation. Consider demunge as "best effort", not something to rely on.





[CLJ-703] Improve writeClassFile performance Created: 04/Jan/11  Updated: 17/May/15

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

Type: Enhancement Priority: Critical
Reporter: Jürgen Hötzel Assignee: Unassigned
Resolution: Unresolved Votes: 25
Labels: Compiler, performance

Attachments: Text File 0001-use-File.mkdirs-instead-of-mkdir-every-single-direct.patch     Text File 0002-Ensure-atomic-creation-of-class-files-by-renaming-a-.patch     Text File improve-writeclassfile-perf.patch     Text File remove-flush-and-sync-only.patch     Text File remove-sync-only.patch    
Patch: Code
Approval: Triaged

 Description   

This Discussion about timing issues when writing class files led to the the current implementation of synchronous writes to disk. This leads to bad performance/system-load when compiling Clojure code.

This Discussion questioned the current implentation.

Synchronous writes are not necessary and also do not ensure (crash while calling write) valid classfiles.

These Patches (0001 is just a code cleanup for creating the directory structure) ensures atomic creation of classfiles by using File.renameTo()



 Comments   
Comment by David Powell [ 17/Jan/11 2:16 PM ]

Removing sync makes clojure build much faster. I wonder why it was added in the first place? I guess only Rich knows? I assume that it is not necessary.

If we are removing sync though, I wouldn't bother with the atomic rename stuff. Doing that sort of thing can cause problems on some platforms, eg with search indexers and virus checkers momentarily locking files after they are created.

The patch seems to be assuming that sync is there for some reason, but my initial assumption would be that sync isn't necessary - perhaps it was working around some issue that no longer exists?

Comment by Jürgen Hötzel [ 19/Jan/11 2:05 PM ]

Although its unlikely: there is a possible race condition "loading a paritally written classfile"?:

https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/RT.java#L393

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

The new improve-writeclassfile-perf version of the patch combines the two previous patches into a single patch file and brings them up-to-date with master. I can split the two changes back out into separate patch files if desired, but I figured out current tooling is more geared towards a single patch being applied.

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

FWIW, both fixes look sane. The first one is a nice cleanup. The second one is a little more interesting in that uses a rename operation to put the final file into place. It removes the sync call, which does make things faster. In general, if we're concerned about on-disk consistency, we should really have a combination of the two: write the full contents to a tmp file, sync it, and atomically rename to the destination file name.

Neither the current master, nor the current patch will guarantee on-disk consistency across a machine wide crash. The current master could crash before the sync() occurs, leaving the file in an inconsistent state. With the patch, the OS may not get the file from file cache to disk before an OS level crash occurs, and leave the file in an inconsistent state as well. The benefit of the patch version is that the whole file does atomically come into view at once. It does have a nasty side effect of leaving around a temp file if the compiler crashes just before the rename though.

Perhaps a little more work to catch an exception and clean up is in order? In general, I like the patched version better.

Comment by Ivan Kozik [ 05/Oct/12 7:07 PM ]

File.renameTo returns false on (most?) errors, but the patch doesn't check for failure. Docs say "The return value should always be checked to make sure that the rename operation was successful." Failure might be especially likely on Windows, where files are opened by others without FILE_SHARE_DELETE.

Comment by Dave Della Costa [ 23/Jun/14 11:37 PM ]

We've been wondering why our compilation times on linux were so slow. It became the last straw when we walked away from one project and came back after 15 minutes and it was not done yet.

After some fruitless investigation into our linux configuration and lein java args, we stumbled upon this issue via the associated Clojure group thread. Upon commenting out the flush() and sync() lines (https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L7171-L7172) and compiling Clojure 1.6 ourselves, our projects all started compiling in under a minute.

Point being, can we at least provide some flag to allow for "unsafe compilation" or something? As it is, this is bad enough that we've manually modified all our local versions of Clojure to work around the issue.

Comment by Tim McCormack [ 30/Sep/14 4:10 PM ]

Additional motivation: This becomes really unpleasant on an encrypted filesystem, since write and read latency become higher.

As a partial workaround, I've been using this script to mount a ramdisk over top of target, which speeds up compilation 2-4x: https://gist.github.com/timmc/6c397644668bcf41041f (but removing flush() and sync() entirely would probably speed things up even more, if safe)

Comment by Ragnar Dahlen [ 06/Oct/14 12:30 PM ]

I'd like to explore this issue further as I also don't think the flush and sync calls add any value, but do have a severe impact on performance.

To resurrect the discussion, I've attached a new patch with the following approach:

  • create a temporary file
  • write class bytecode to temporary file, no flush or sync
  • close temporary file
  • atomic rename of temporary file to class file name

It is different to previous patches in that:

  • it applies cleanly to master
  • it checks return value from File.renameTo
  • it omits proposed File.mkdirs change as the current implementation is actually converting from an "internal name", where forward slashes are assumed (splits on "/"), to a platform specific path using File.separator. I'm not convinced that the previous patch is safe on all versions of Windows, and I think it's separate from the main issue here.

I opted for the atomic rename of a temp file to avoid leaving empty class files with a correct expected class file name in case of failure.

It is my understanding that this patch will guarantee that:

  • when writeClassFile returns successfully, a class file with the expected name will exists, and subsequent reads from that file name will return the expected bytecode (possibly from kernel buffers).
  • when writeClassFile fails, a class file with the expected name will not have been created (or updated if it previously existed).

Anything preventing the operating system from flushing its buffers to disk (power failure etc) might leave a corrupt (empty, partially written) class file. It's my opinion that that is acceptable behaviour, and worth the performance improvement (I'm seeing AOT compilation reduced from 1m20s -> 22s on one of our codebases, would be happy to provide more benchmarks if requested).

Would be grateful for feedback/testing of this patch.

Comment by Ragnar Dahlen [ 08/Oct/14 6:00 AM ]

We're testing this patch on various projects/platforms at my company. So far we've seen:

  • Significantly reduced compilation times on Linux (two typical examples: 30s to 15s, 1m30s to 30s)
  • No significant change in compilation times on Mac OSX.
  • File.renameTo consistently failing on a Windows machine.

My understanding is that the performance difference between Linux and OSX is due to differences in how these platforms implement fsync. OSX by default does not actually tell the drive to flush its buffers (requires fcntl F_FULLSYNC for this, not used by JVMs) [1], whereas Linux does [2].

Our very limited test shows (as was previously pointed out) that File.renameTo is problematic on Windows. I've attached a new patch that doesn't use rename, and only has the the sync call removed (flush is a no-op for FileOutputStreams). We're currently testing this patch.

The drawback of this patch is that it may leave correctly named, but empty class files if the write fails. One option would be to try and delete the file in the catch block. Personally, I wouldn't expect a compilation that failed because of OS/IO reasons to leave my classfiles in a consistent state.

[1]: "Note that while fsync() will flush all data from the host to the drive (i.e. the "permanent storage device"), the drive itself may not physically write the data to the platters for quite some time and it may be written in an out-of-order sequence.": https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man2/fsync.2.html

[2]: "[...] includes writing through or flushing a disk cache if present."
http://man7.org/linux/man-pages/man2/fsync.2.html

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

Currently this ticket is not a state where it can be evaluated, and I would like it to get there before we consider tickets for 1.8.

The description for this ticket needs to be something that a screener read to fully understand the problem, proposed solution, performance implications, and tradeoffs. Right now it does not seem up to date with information discussed in comments, which patch should be considered, performance data, and what we might lose by making the change. It would be great if someone could help get this in shape.

Comment by Andy Fingerhut [ 17/May/15 10:16 AM ]

Alex, you will probably get a wider audience of contributors willing to help if you sent an email to clojure-dev every once in a while with a list of tickets you want help updating. Right now I think it is this one and CLJ-1449, but it would be nice if there were a single report link contributors could click on to find out what you are looking for help with.

Normally there is the "Needs Patch" state for the next release, but for CLJ-1449 you are looking for a patch for the next-next release, so "Needs Patch" won't do it.

Perhaps if there were 'standard' labels created for 'Alex requests ticket updates' for such tickets, and filter for them?

Comment by Ragnar Dahlen [ 17/May/15 10:47 AM ]

I'd be happy to help improve the state of this ticket. I'm not the original reporter, is it still OK for me to change description etc. to address your concerns?

Comment by Alex Miller [ 17/May/15 11:02 AM ]

Andy - I will probably do so soon but I thought previous commenters would see this now.

Ragnar - absolutely!





[CLJ-1319] array-map fails lazily if passed an odd number of arguments Created: 08/Jan/14  Updated: 15/May/15

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

Type: Defect Priority: Minor
Reporter: Stuart Sierra Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs, ft

Attachments: Text File 0001-CLJ-1319-Throw-on-odd-arguments-to-PersistentArrayMa.patch     Text File 0002-CLJ-1319-Throw-on-odd-arguments-to-PersistentArrayMa.patch    
Patch: Code and Test
Approval: Triaged

 Description   

If called with an odd number of arguments, array-map does not throw an exception until the map is realized, when it throws the confusing ArrayIndexOutOfBoundsException.

Example, in 1.5.1 and 1.6.0-alpha3:

user=> (def m (hash-map :a 1 :b))
IllegalArgumentException No value supplied for key: :b  clojure.lang.PersistentHashMap.create (PersistentHashMap.java:77)

user=> (def m (array-map :a 1 :b))
#'user/m
user=> (prn m)
ArrayIndexOutOfBoundsException 3
  clojure.lang.PersistentArrayMap$Seq.first
  (PersistentArrayMap.java:313)

Approach: Catch on construction and throw same error as hash-map.

user=> (def m (array-map :a 1 :b))
IllegalArgumentException No value supplied for key: :b  clojure.lang.PersistentArrayMap.createAsIfByAssoc (PersistentArrayMap.java:78)

Patch: 0002-CLJ-1319-Throw-on-odd-arguments-to-PersistentArrayMa.patch
Screened by: Alex Miller



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

PersistentArrayMap.createAsIfByAssoc could check length is even to catch this

Comment by Jason Felice [ 27/Jan/14 1:01 PM ]

A better error message would be nice... this is the best I could think of.

Comment by OHTA Shogo [ 14/May/15 7:06 AM ]

I suffered from this problem recently. Anything to resolve before applying the patch?

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

The error message should match the message for hash-map:

user=> (hash-map 1 2 3)
IllegalArgumentException No value supplied for key: 3  clojure.lang.PersistentHashMap.create (PersistentHashMap.java:77)
user=> (array-map 1 2 3)
IllegalArgumentException Odd number of arguments when creating map  clojure.lang.PersistentArrayMap.createAsIfByAssoc (PersistentArrayMap.java:78)
Comment by OHTA Shogo [ 14/May/15 8:49 PM ]

Let me have a try. I just updated the patch!

0002-CLJ-1319-Throw-on-odd-arguments-to-PersistentArrayMa.patch

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

Looks good.





[CLJ-1239] faster, more flexible dispatch for clojure.walk Created: 29/Jul/13  Updated: 14/May/15

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

Type: Enhancement Priority: Major
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Unresolved Votes: 6
Labels: walk

Attachments: Text File 0001-CLJ-1239-protocol-dispatch-for-clojure.walk.patch     Text File 0002-CLJ-1239-protocol-dispatch-for-clojure.walk.patch    
Patch: Code
Approval: Triaged

 Description   

The conditional dispatch in clojure.walk is slow and not open to extension. Prior to CLJ-1105 it did not support records.

Approach: Reimplement clojure.walk using protocols. The public API does not change. Users can extend the walk protocol to other types (for example, Java collections) if desired. As in CLJ-1105, this version of clojure.walk supports records.

Patch: 0002-CLJ-1239-protocol-dispatch-for-clojure.walk.patch

Performance: My tests indicate this is 1.5x-2x the speed of the original clojure.walk. See https://github.com/stuartsierra/clojure.walk2 for benchmarks.

Risks: This approach carries some risk of breaking user code that relied on type-specific behavior of the old clojure.walk. When running the full Clojure test suite, I discovered (and fixed) some breakages that did not show up in clojure.walk's unit tests. See, for example, commit 730eb75 in clojure.walk2



 Comments   
Comment by Vjeran Marcinko [ 19/Oct/13 12:32 PM ]

It looks, as it is now, that walking the tree and replacing forms doesn't preserve original meta-data contained in data structures.

Comment by Andy Fingerhut [ 23/Nov/13 1:11 AM ]

Patch 0001-CLJ-1239-protocol-dispatch-for-clojure.walk.patch no longer applies cleanly to latest Clojure master since the patch for CLJ-1105 was committed on Nov 22, 2013. From the description, it appears the intent was either that patch or this one, not both, so I am not sure what should happen with this patch, or even this ticket.

Comment by Alex Miller [ 23/Nov/13 1:52 AM ]

This patch and ticket are still candidates for future release.

Comment by Stuart Sierra [ 20/Dec/13 9:14 AM ]

Added new patch that applies on latest master after CLJ-1105.

Comment by Chouser [ 27/Feb/14 10:26 AM ]

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

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

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

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

Here's an implementation that doesn't suffer from that problem, though it does scary class name munging instead: https://github.com/LonoCloud/synthread/blob/a315f861e04fd33ba5398adf6b5e75579d18ce4c/src/lonocloud/synthread/impl.clj#L66

Perhaps we could add to the defrecord abstraction to support well the kind of things that synthread code is doing clumsily, and then walk could take advantage of that.

Comment by Alex Miller [ 27/Feb/14 2:11 PM ]

@Chouser, can you file a new ticket related to this? It's hard to manage work on something from comments on a closed ticket.

Comment by Alex Miller [ 27/Feb/14 3:54 PM ]

@Chouser - Never mind! I was thinking this was the change that went into 1.6. Carry on.

Comment by Nicola Mometto [ 27/Feb/14 5:17 PM ]

Alex, for what it matters clojure-1.6.0 after CLJ-1105 exibits the same behaviour as described by Chouser for this patch





[CLJ-1209] clojure.test does not print ex-info in error reports Created: 11/May/13  Updated: 14/May/15

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

Type: Enhancement Priority: Critical
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: clojure.test

Attachments: Text File 0002-CLJ-1209-show-ex-data-in-clojure-test.patch     File clj-test-print-ex-data.diff     Text File output-with-0002-patch.txt    
Patch: Code
Approval: Triaged

 Description   

clojure.test does not print the data attached to ExceptionInfo in error reports.

(use 'clojure.test)
(deftest ex-test (throw (ex-info "err" {:some :data})))
(ex-test)

ERROR in (ex-test) (core.clj:4591)
Uncaught exception, not in assertion.
expected: nil
  actual: clojure.lang.ExceptionInfo: err
 at clojure.core$ex_info.invoke (core.clj:4591)
    user/fn (NO_SOURCE_FILE:2)
    clojure.test$test_var$fn__7666.invoke (test.clj:704)
    clojure.test$test_var.invoke (test.clj:704)
    ...

Approach: In clojure.stacktrace, which clojure.test uses for printing exceptions, add a check for ex-data and pr it.

After:

ERROR in (ex-test) (core.clj:4591)
Uncaught exception, not in assertion.
expected: nil
  actual: clojure.lang.ExceptionInfo: err
{:some :data}
 at clojure.core$ex_info.invoke (core.clj:4591)
    user/fn (NO_SOURCE_FILE:3)
    clojure.test$test_var$fn__7667.invoke (test.clj:704)
    clojure.test$test_var.invoke (test.clj:704)

Patch: 0002-CLJ-1209-show-ex-data-in-clojure-test.patch



 Comments   
Comment by Alex Miller [ 20/Dec/13 9:53 AM ]

Great idea, thx for the patch!

Comment by Alex Miller [ 20/Dec/13 9:54 AM ]

Would be great to see a before and after example of the output.

Comment by Ivan Kozik [ 12/Jul/14 10:35 PM ]

Attaching sample output

Comment by Stuart Sierra [ 05/Sep/14 3:24 PM ]

As pointed out on IRC, there's a possible risk of trying to print an infinite lazy sequence that happened to be included in ex-data.

To mitigate, consider binding *print-length* and *print-level* to small numbers around the call to pr.

Comment by Stephen C. Gilardi [ 13/May/15 2:39 PM ]

http://dev.clojure.org/jira/browse/CLJ-1716 may cover this well enough that this issue can be closed.

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

I don't think 1716 covers it at all as clojure.test/clojure.stacktrace don't use the new throwable printing. But they could! And that might be a better solution than the patch here.

For example, the existing patch does not consider what to do about nested exceptions, some of which might have ex-data. The new printer handles all that in a consistent way.





[CLJ-1130] When unable to match a static method, report arity caller was looking for, avoid misleading field error Created: 17/Dec/12  Updated: 13/May/15

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

Type: Enhancement Priority: Major
Reporter: Howard Lewis Ship Assignee: Stuart Halloway
Resolution: Unresolved Votes: 1
Labels: errormsgs, ft

Attachments: Text File clj-1130-v1.txt     File clj-1130-v2.diff     File clj-1130-v2-ignore-ws.diff     Text File clj-1130-v2.txt     File clj-1130-v3.diff     File clj-1130-v4.diff     File clj-1130-v5.diff    
Patch: Code and Test
Approval: Vetted

 Description   

Incorrectly invoking a static method with 0 parameters yields a NoSuchFieldException:

user=> (Long/parseLong)
CompilerException java.lang.NoSuchFieldException: parseLong, compiling:(NO_SOURCE_PATH:1:1) 
user=> (Long/parseLong "5" 10 3)
CompilerException java.lang.IllegalArgumentException: No matching method: parseLong, compiling:(NO_SOURCE_PATH:2:1)

Approach: Error reporting enhanced to report desired arg count and to avoid a field exception confusion if 0 arg method.

user=> (Long/parseLong)
CompilerException java.lang.IllegalArgumentException: No matching method parseLong found taking 0 args for class java.lang.Long, compiling:(NO_SOURCE_PATH:1:1)
user=> (Long/parseLong "5" 10 3)
CompilerException java.lang.IllegalArgumentException: No matching method parseLong found taking 3 args for class java.lang.Long, compiling:(NO_SOURCE_PATH:2:1)

Patch: clj-1130-v5.diff

Screened by: Alex Miller



 Comments   
Comment by Michael Drogalis [ 06/Jan/13 6:44 PM ]

It looks like it's first trying to resolve a field by name, since field access with / is legal. For example:

user=> (Integer/parseInt)
CompilerException java.lang.NoSuchFieldException: parseInt, compiling:(NO_SOURCE_PATH:1)

user=> (Integer/MAX_VALUE)
2147483647

Would trying to resolve a method before a field fix this?

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

Similarities to CLJ-1248 (there a warning, here an error).

Comment by Andy Fingerhut [ 09/Sep/13 12:36 AM ]

Patch clj-1130-v1.txt changes the error message in a situation when one attempts to invoke a static method with no args, and there is no such 0-arg static method. The message now says that there is no such method with that name and 0 args, rather than that there is no such static field with that name.

Comment by Alex Miller [ 04/Oct/13 3:56 PM ]

I updated the patch to simplify it a bit but more importantly to remove the check by exception and instead use the Reflector method that can answer this question.

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

Alex, thank you for the improvements to the code. It looks better to me.

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

due to indentation changes, this patch appears to touch much more than it probably does, making it difficult to approve.

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

Any suggestions on what can be done to make progress here? Would it help to attach a patch made with "-w" option to ignore lines that differ only in whitespace? Provide git diff command line options that do this, after the patch is applied to your local workspace? Make a patch that leaves the indentation 'incorrect' after the change (involuntary shudder)?

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

The indentation has intentionally changed because the if/else structure has changed. I don't think making the patch incorrect to reduce changes is a good idea.

Comment by Andy Fingerhut [ 25/Oct/13 11:32 AM ]

Well, the 'incorrect' was in quotes because I was asking about a proposed patch that had the correct logic, but misleading indentation. Agreed it isn't a good idea, hence the shudder. I'm just brainstorming ideas to make the patch less difficult to approve.

Comment by Howard Lewis Ship [ 25/Oct/13 11:43 AM ]

At some point, you may need to bite the bullet and reformat some of the Clojure code .... Compiler.java had a crazy mix of tabs, spaces, and just completely wrong stuff.

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

Re-marking screened. Not sure what else to do.

Comment by Andy Fingerhut [ 04/Nov/13 8:35 AM ]

clj-1130-v2-ignore-ws.diff is identical to clj-1130-v2.diff, except it was produced with a command that ignores differences in a line due only to whitespace, i.e.: 'git format-patch master --stdout -w > clj-1130-v2-ignore-ws.diff'

It is not intended as the patch to be applied. It is only intended to make it easier to see that many of the lines in clj-1130-v2.diff are truly only differences in indentation.

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

Thanks Andy...

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

This patch ignores the fact that method is checked for first above:

if(c != null)
  maybeField = Reflector.getMethods(c, 0, munge(sym.name), true).size() == 0;

Which is why the field code is unconditional. I'm fine with making errors better, but changing logic as well deserves more scrutiny.

Comment by Alex Miller [ 06/Dec/13 9:01 PM ]

This patch is intentionally trying to avoid calling StaticFieldExpr in the field code as that is where the (Long/parseLong) case (erroneously calling an n-arity static method with 0 args) will throw a field-oriented exception instead of a method-oriented exception. By adding the extra check here, this case falls through into the method case and throws later on calling StaticMethodExpr instead.

The early check is a check for methods of the specified arity. The later check is for the existence of a field of matching name. Combined, they lead to a better error message.

However, another alternative is to set maybeField in the first check based on field existence, not on invocation arity. That just improves the maybeField informaiton and the existing code then naturally throws the correct exception (and the patch is much simpler).

The similar case for calling n-arity instance methods with 0-arity has the same problem for the same reason:

user=> (.setTime (java.util.Date.))
IllegalArgumentException No matching field found: setTime for class java.util.Date  clojure.lang.Reflector.getInstanceField (Reflector.java:271)

Thus we can also adjust the other call that sets maybeField (which now is much less maybe).

I will attach a patch that covers these cases and update the ticket for someone to screen.

Comment by Stuart Sierra [ 08/Dec/13 12:24 PM ]

Screened. The patch clj-1130-v3.diff works as advertised.

This patch only improves error messages for cases when the type of the
target object is known to the compiler. In reflective calls, the error
messages are still the same.

Example, after this patch, given these definitions:

(def v 42)
(defn untagged-f [] 42)
(defn ^Long tagged-f [] 42)

The following expressions produce new error messages:

(.foo v 1)
;; IllegalArgumentException No matching method found: foo taking 1 args
;; for class java.lang.Long clojure.lang.Reflector.invokeMatchingMethod
;; (Reflector.java:53)

(.foo (tagged-f))
;; IllegalArgumentException No matching method found: foo taking 0 args
;; for class java.lang.Long clojure.lang.Reflector.invokeMatchingMethod
;; (Reflector.java:53)

These expressions still use the old error messages:

(.foo v)
;; IllegalArgumentException No matching field found: foo for class
;; java.lang.Long clojure.lang.Reflector.getInstanceField
;; (Reflector.java:271)

(.foo (untagged-f))
;; IllegalArgumentException No matching field found: foo for class
;; java.lang.Long clojure.lang.Reflector.getInstanceField
;; (Reflector.java:271)
Comment by Rich Hickey [ 03/Jan/14 8:41 AM ]

Changing the logic to get a different error message is something that needs to be done with great care. This now seems to prefer fields over methods, changing the semantics.

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

v4 patch simply enhances error messaages

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

clj-1130-v4.diff has the same patch repeated twice in the file. clj-1130-v5.diff is identical, except deleting the redundant copy.





[CLJ-1005] Use transient map in zipmap Created: 30/May/12  Updated: 13/May/15

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

Type: Enhancement Priority: Critical
Reporter: Michał Marczyk Assignee: Unassigned
Resolution: Unresolved Votes: 11
Labels: ft, performance

Attachments: Text File 0001-Use-transient-map-in-zipmap.2.patch     Text File 0001-Use-transient-map-in-zipmap.patch     Text File 0002-CLJ-1005-use-transient-map-in-zipmap.patch     Text File CLJ-1005-zipmap-iterators.patch    
Patch: Code
Approval: Vetted

 Description   

#'zipmap constructs a map without transients, where transients could improve performance.

Approach: Use a transient map internally, along with iterators for the keys and values. A persistent map is returned as before. The definition is also moved so that it resides below that of #'transient.

Performance:

(def xs (range 16384))
(def ys (range 16))

expression 1.7.0-beta3 +patch  
(zipmap xs xs) 4.50 ms 2.12 ms large map
(zipmap ys ys) 2.75 us 2.07 us small map

Patch: CLJ-1005-zipmap-iterators.patch

Screened by: Alex Miller



 Comments   
Comment by Aaron Bedra [ 14/Aug/12 9:24 PM ]

Why is the old implementation left and commented out? If we are going to move to a new implementation, the old one should be removed.

Comment by Michał Marczyk [ 15/Aug/12 4:17 AM ]

As mentioned in the ticket description, the previously attached patch follows the pattern of into whose non-transient-enabled definition is left in core.clj with a #_ in front – I wasn't sure if that's something desirable in all cases.

Here's a new patch with the old impl removed.

Comment by Andy Fingerhut [ 15/Aug/12 10:37 AM ]

Thanks for the updated patch, Michal. Sorry to raise such a minor issue, but would you mind using a different name for the updated patch? I know JIRA can handle multiple attached files with the same name, but my prescreening code isn't quite that talented yet, and it can lead to confusion when discussing patches.

Comment by Michał Marczyk [ 15/Aug/12 10:42 AM ]

Thanks for the heads-up, Andy! I've reattached the new patch under a new name.

Comment by Andy Fingerhut [ 16/Aug/12 8:24 PM ]

Presumptuously changing Approval from Incomplete back to None after the Michal's updated patch was added, addressing the reason the ticket was marked incomplete.

Comment by Aaron Bedra [ 11/Apr/13 5:32 PM ]

The patch looks good and applies cleanly. Are there additional tests that we should run to verify that this is providing the improvement we think it is. Also, is there a discussion somewhere that started this ticket? There isn't a lot of context here.

Comment by Michał Marczyk [ 11/Apr/13 6:19 PM ]

Hi Aaron,

Thanks for looking into this!

From what I've been able to observe, this change hugely improves zipmap times for large maps. For small maps, there is a small improvement. Here are two basic Criterium benchmarks (transient-zipmap defined at the REPL as in the patch):

;;; large map
user=> (def xs (range 16384))
#'user/xs
user=> (last xs)
16383
user=> (c/bench (zipmap xs xs))
Evaluation count : 13920 in 60 samples of 232 calls.
             Execution time mean : 4.329635 ms
    Execution time std-deviation : 77.791989 us
   Execution time lower quantile : 4.215050 ms ( 2.5%)
   Execution time upper quantile : 4.494120 ms (97.5%)
nil
user=> (c/bench (transient-zipmap xs xs))
Evaluation count : 21180 in 60 samples of 353 calls.
             Execution time mean : 2.818339 ms
    Execution time std-deviation : 110.751493 us
   Execution time lower quantile : 2.618971 ms ( 2.5%)
   Execution time upper quantile : 3.025812 ms (97.5%)

Found 2 outliers in 60 samples (3.3333 %)
	low-severe	 2 (3.3333 %)
 Variance from outliers : 25.4675 % Variance is moderately inflated by outliers
nil

;;; small map
user=> (def ys (range 16))
#'user/ys
user=> (last ys)
15
user=> (c/bench (zipmap ys ys))
Evaluation count : 16639020 in 60 samples of 277317 calls.
             Execution time mean : 3.803683 us
    Execution time std-deviation : 88.431220 ns
   Execution time lower quantile : 3.638146 us ( 2.5%)
   Execution time upper quantile : 3.935160 us (97.5%)
nil
user=> (c/bench (transient-zipmap ys ys))
Evaluation count : 18536880 in 60 samples of 308948 calls.
             Execution time mean : 3.412992 us
    Execution time std-deviation : 81.338284 ns
   Execution time lower quantile : 3.303888 us ( 2.5%)
   Execution time upper quantile : 3.545549 us (97.5%)
nil

Clearly the semantics are preserved provided transients satisfy their contract.

I think I might not have started a ggroup thread for this, sorry.

Comment by Andy Fingerhut [ 03/Sep/14 8:10 PM ]

Patch 0001-Use-transient-map-in-zipmap.2.patch dated Aug 15 2012 does not apply cleanly to latest master after some commits were made to Clojure on Sep 3 2014.

I have not checked whether this patch is straightforward to update. See the section "Updating stale patches" at http://dev.clojure.org/display/community/Developing+Patches for suggestions on how to update patches.

Comment by Michał Marczyk [ 14/Sep/14 12:48 PM ]

Thanks, Andy. It was straightforward to update – an automatic rebase. Here's the updated patch.

Comment by Ghadi Shayban [ 22/Sep/14 9:58 AM ]

New patch using clojure.lang.RT/iter, criterium shows >30% more perf in the best case. Less alloc probably but I didn't measure. CLJ-1499 (better iterators) is related





[CLJ-1730] Improve `refer` performance Created: 13/May/15  Updated: 13/May/15

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

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

Attachments: Text File refer-perf.patch    
Patch: Code
Approval: Triaged

 Description   

refer underlies require, use, and refer-clojure use cases and is not particularly efficient at its primary job of copying symbol/var mapping from one namespace to another.

Approach: Some improvements that can be made:

  • Go directly to the namespace mappings and avoid creating filtered intermediate maps (ns-publics)
  • Use transients to build map of references to refer
  • Instead of cas'ing each new reference individually, build map of all changes, then cas
  • For (:require :only ...) case - instead of walking all referred vars and looking for matches, walk only the included vars and look up each one

There are undoubtedly more dramatic changes (like immutable namespaces) in how all this works that could further improve performance but I tried to make the scope small-ish for this change.

While individual refer timings are greatly reduced (~50% reduction for (refer clojure.core), ~90% reduction for :only use), refer is only a small component of broader require load times so the improvements in practice are modest.

Performance:

expr in a new repl 1.7.0-beta3 1.7.0-beta3+patch
(in-ns 'foo) (clojure.core/refer 'clojure.core) 2.65 ms 0.994 ms
(in-ns 'bar) (clojure.core/refer 'clojure.core :only '[inc dec]) 1.04 ms 0.113 ms
(use 'criterium.core) 0.877 ms 0.762 ms
(require '[clojure.core.async :refer (>!! <!! chan close!)]) 3408 ms 3302 ms

Patch: refer-perf.patch






[CLJ-1728] source fn fails for fns with conditional code Created: 10/May/15  Updated: 12/May/15  Resolved: 12/May/15

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

Type: Defect Priority: Major
Reporter: Mike Fikes Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: reader, repl
Environment:

1.7.0-beta2


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

 Description   

Note: Similar to issue CLJS-1261.

If you use the source Clojure REPL function on a function defined in a CLJC file, where the function itself contains some conditional code, then the operation will fail with "Conditional read not allowed".

To reproduce:
Do a lein new testme, rename the core.clj file to core.cljc, and then add the following

(defn f 
  "Eff"
  [] 
  1)

(defn g 
  "Gee"
  []
  #?(:clj "clj" :cljs "cljs"))

Additionally, revise the project.clj to specify 1.7.0-beta2.

Require the testme.core namespace, referring :all.

Verify that you can call, get the doc for, and source for f.

But, on the other hand, while you can call and get the doc for g, you can't do (source testme.core/g).

user=> (source testme.core/g)

RuntimeException Conditional read not allowed  clojure.lang.Util.runtimeException (Util.java:221)
user=> (pst)
RuntimeException Conditional read not allowed
	clojure.lang.Util.runtimeException (Util.java:221)
	clojure.lang.LispReader$ConditionalReader.checkConditionalAllowed (LispReader.java:1406)
	clojure.lang.LispReader$ConditionalReader.invoke (LispReader.java:1410)
	clojure.lang.LispReader$DispatchReader.invoke (LispReader.java:682)
	clojure.lang.LispReader.read (LispReader.java:255)
	clojure.lang.LispReader.readDelimitedList (LispReader.java:1189)
	clojure.lang.LispReader$ListReader.invoke (LispReader.java:1038)
	clojure.lang.LispReader.read (LispReader.java:255)
	clojure.lang.LispReader.read (LispReader.java:195)
	clojure.lang.LispReader.read (LispReader.java:190)
	clojure.core/read (core.clj:3638)
	clojure.core/read (core.clj:3636)
nil

Approach: Set {:read-cond :allow} if source file extension is .cljc. Test above works now.

Patch: clj-1728.patch



 Comments   
Comment by Mike Fikes [ 11/May/15 8:05 AM ]

I tested with Alex's cli-1728.patch, and it works for me.

Comment by Mike Fikes [ 12/May/15 7:02 PM ]

Confirmed fixed using master.





[CLJ-1727] range confused by large bounds Created: 07/May/15  Updated: 12/May/15  Resolved: 12/May/15

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

Type: Defect Priority: Major
Reporter: Steve Miner Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: regression

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

 Description   

There are a number of issues related to counting and overflow in the current LongRange implementation.

expression 1.6.0 1.7.0-beta2 +patch comment
(range (- Long/MAX_VALUE 2) Long/MAX_VALUE) (9223372036854775805 9223372036854775806) OOME (9223372036854775805 9223372036854775806) top of long range
(count (range (- Long/MAX_VALUE 2) Long/MAX_VALUE)) 2 2 2 top of long range
(range (+ Long/MIN_VALUE 2) Long/MIN_VALUE -1) (-9223372036854775806 -9223372036854775807) OOME (-9223372036854775806 -9223372036854775807) bottom of long range
(count (range (+ Long/MIN_VALUE 2) Long/MIN_VALUE -1)) 2 2 2 bottom of long range
(range Long/MIN_VALUE Long/MAX_VALUE Long/MAX_VALUE) ArithmeticEx OOME (-9223372036854775806 -1 9223372036854775806) large positive step
(count (range Long/MIN_VALUE Long/MAX_VALUE Long/MAX_VALUE)) ArithmeticEx 0 3 large positive step
(range Long/MAX_VALUE Long/MIN_VALUE Long/MIN_VALUE) ArithmeticEx OOME (9223372036854775807 -1) large negative step
(count (range Long/MAX_VALUE Long/MIN_VALUE Long/MIN_VALUE)) ArithmeticEx 0 2 large negative step
(count (range 0 Long/MAX_VALUE)) overflows to nonsense -2147483648 ArithmeticEx number of values in range > Integer.MAX_VALUE

Cause: There were several bugs, both old and new, in the counting related code for range, particularly around overflow and (introduced in CLJ-1709) coercion error.

Approach: The patched code:

  • Uses only exact values (no double conversion in there)
  • Fixes the algorithm for integer ceiling to be correct
  • Explicitly does overflow-checking calculations when necessary (chunking, counting, and iterator)
  • In the case of overflow, falls back to slower stepping algorithm if necessary (this is only in pathological cases like above)
  • Added many new tests thanks to Andy Fingerhut and Steve Miner.

One particular question is what to do in the case where the count of a range is > Integer.MAX_VALUE. The choices are:
1. Return Integer.MAX_VALUE (Java collection size() solution)
2. Throw ArithmeticOverflowException (since your answer is going to be wrong)
3. Overflow and let bad stuff happen (Clojure 1.6 does this)

The current patch takes approach #2, per Rich.

Performance check:

expr 1.6 beta1 beta2 beta2+patch
(count (range (* 1024 1024))) 63 ms 0 ms 0 ms 0 ms
(reduce + (map inc (range (* 1024 1024)))) 55 ms 35 ms 34 ms 32 ms
(reduce + (map inc (map inc (range (* 1024 1024))))) 74 ms 59 ms 56 ms 54 ms
(count (keep odd? (range (* 1024 1024)))) 77 ms 52 ms 48 ms 49 ms
(transduce (comp (map inc) (map inc)) + (range (* 1024 1024))) n/a 30 ms 26 ms 26 ms
(reduce + 0 (range (* 2048 1024))) 72 ms 29 ms 29 ms 21 ms
(reduce + 0 (rest (range (* 2048 1024)))) 73 ms 29 ms 30 ms 21 ms
(doall (range 0 31)) 1.38 us 0.97 us 0.73 us 0.75 us
(doall (range 0 32)) 1.38 us 0.99 us 0.76 us 0.77 us
(doall (range 0 4096)) 171 us 126 us 125 us 98 us
(into [] (map inc (range 31))) 1.87 us 1.34 us 1.27 us 1.33 us
(into [] (map inc) (range 31)) n/a 0.76 ms 0.76 ms 0.76 ms
(into [] (range 128)) 5.26 us 2.18 us 2.15 us 2.22 us

Patch: clj-1727-4.patch



 Comments   
Comment by Steve Miner [ 07/May/15 10:49 AM ]

It looks like something is overflowing when doing the count calculation. Here's another example:

(count (range (- Long/MAX_VALUE 7)))
-2147483648

Comment by Alex Miller [ 07/May/15 10:54 AM ]

Looks like lack of overflow checking in the chunk logic maybe.

Comment by Alex Miller [ 07/May/15 11:11 AM ]

The example in the description is overflowing in computing nextStart in forceChunk(). That should be fixable, will consider some options.

The example in the comment overflows calculating the count, which is > max int. I'm not sure there actually is a good answer in that particular case. The Java Collection interface expects count() to return Integer.MAX_VALUE in this case (which is bad, but equally bad as every other incorrect answer when the value is not representable in the type).

Comment by Steve Miner [ 07/May/15 11:18 AM ]

LongRange absCount looks suspicious. That double math might be wrong. If the (end - start) is large, the conversion to double loses integer precision. For example, in Clojure:

(- Long/MAX_VALUE (long (double (- Long/MAX_VALUE 1000))))
1023

You might have expected 1000, but the double conversion was not exact. Of course, it works from some values, like exactly Long/MAX_VALUE.

I think it might be safer to restrict the LongRange to the safe bounds, basically inside (bit-shift-right Long/MAX_VALUE 10). Or just use (long Integer/MAX_VALUE) as a reasonable max.

Comment by Andy Fingerhut [ 07/May/15 12:02 PM ]

Some other fun test cases, if the goal is to make LongRange work for entire range of longs:

user=> (take 5 (range (- Long/MAX_VALUE 2) Long/MAX_VALUE))
(9223372036854775805 9223372036854775806 9223372036854775807 -9223372036854775808 -9223372036854775807)
user=> (take 5 (range (- Long/MAX_VALUE 2) Long/MAX_VALUE 7))
(9223372036854775805 -9223372036854775804 -9223372036854775797 -9223372036854775790 -9223372036854775783)
user=> (take 5 (range (- Long/MAX_VALUE 2) Long/MAX_VALUE Long/MAX_VALUE))
(9223372036854775805 -4 9223372036854775803 -6 9223372036854775801)
Comment by Andy Fingerhut [ 07/May/15 1:24 PM ]

Attachment clj-1709-wip-1.patch is intentionally in the wrong format, since it is only intended as a hint of what might be done.

I think using float or double arithmetic for absCount is a very bad idea, given all the subtle roundoff things that could happen there. Exact arithmetic is best.

It has known limitations mentioned in a few comments in the code. There may be unknown limitations, too.

Generative tests that specifically tried to hit the corner cases, e.g. start values often near Long/MIN_VALUE, end values near Long/MAX_VALUE, step values near 0 and the extreme long values, etc. are much more likely to hit any remaining bugs here, but are also very slow to test given the size of the ranges.

Comment by Andy Fingerhut [ 07/May/15 1:36 PM ]

I am liking Steve Miner's suggestion, or something like it, the more I think on it. That is, check a few more conditions on the values of start, end, and step in clojure.core/range, and if they are not met, avoid LongRange and use Range instead. This would put the common cases in LongRange, and only unusual corner cases in the slower Range. At the same time, it could simplify the code for LongRange and help keep it fast.

Comment by Alex Miller [ 07/May/15 4:17 PM ]

The competing constraint here is of course performance. Adding more checks also makes the fast common path slower. By no means ruling it out, but need to consider it.

Comment by Andy Fingerhut [ 07/May/15 5:23 PM ]

Attachment clj-1709-wip-2.patch is a fleshed out version of the approach suggested by Steve Miner – avoid LongRange/create if the long args to range are such that LongRange would misbehave. The example-based tests could be expanded a bit.

Comment by Alex Miller [ 08/May/15 11:03 AM ]

I have a solution for this that does not need additional up-front checks and has (I think) minimal impact on performance. Polishing it...

Comment by Alex Miller [ 08/May/15 11:03 AM ]

Oh, and big thanks Andy for the tests - I will totally steal those, they were very helpful.

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

Here are a couple more that I would recommend stealing (implying that the clojure.core/range return value should be equal to the clojure.lang.Range/create version):

(range -1 Long/MAX_VALUE Long/MAX_VALUE)  ; large step values make (step * CHUNK_SIZE) overflow
(range 1 Long/MIN_VALUE Long/MIN_VALUE)
Comment by Andy Fingerhut [ 08/May/15 7:52 PM ]

Alex, clj-1727.patch looks solid to me. Or at least I couldn't find anything wrong with it in 30-40 minutes.

One question: In count(), why fall back to super.count() if the actual value fits in a long but is greater than Integer.MAX_VALUE ? Because we want to be bug-compatible with count() in that case, sometimes returning overflowed values? (see example below). It seems faster and better to return Integer.MAX_VALUE in that case.

user=> *clojure-version*
{:major 1, :minor 6, :incremental 0, :qualifier nil}
user=> (count (range (+ Integer/MAX_VALUE 2)))
-2147483647
Comment by Alex Miller [ 08/May/15 8:46 PM ]

Yeah I went back and forth on that. I actually I was throwing an exception before this version. There is no good answer.

Comment by Steve Miner [ 09/May/15 2:19 PM ]

I did some quick tests with the patch and it looks good. I have to agree with Andy that it would make sense to return Integer.MAX_VALUE for the overflow cases. The super.count() is likely to be so slow that it might as well be an infinite loop. If I'm reading the code correctly, stepOffset is always (step > 0 ? -1 : l). I would expect that to be a very fast computation (especially with a final step) so it's probably not worth caching in a field.

Comment by Alex Miller [ 09/May/15 3:10 PM ]

New -2 patch avoids the new field (~same perf) and changes behavior on count over max int.

Comment by Steve Miner [ 10/May/15 1:06 PM ]

Testing with patch-2. It looks like there are a couple of edge cases that can give negative results where I would have expected Integer/MAX_VALUE (for any overflow of int count).

user=> Integer/MAX_VALUE
2147483647
user=> (count (range Long/MIN_VALUE -2))
2147483647 ;OK
user=> (count (range Long/MIN_VALUE -1))
-2147483648

user=> (count (range 1 Long/MAX_VALUE))
2147483647 ;OK
user=> (count (range 0 Long/MAX_VALUE))
-2147483648

I think that count() should return Integer.MAX_VALUE when there's an ArithmeticException, instead of trying super.count() there.

Comment by Andy Fingerhut [ 10/May/15 2:53 PM ]

Steve, I think that Integer.MAX_VALUE is often an incorrect value for count(), when rangeCount throws an exception. For example, here are some results with patch-2, tweaked to make rangeCount public:

user=> (def x (range 0 1 2))
#'user/x
user=> (. x rangeCount Long/MIN_VALUE Long/MAX_VALUE Long/MAX_VALUE)
ArithmeticException integer overflow  clojure.lang.Numbers.throwIntOverflow (Numbers.java:1501)
user=> (count (range Long/MIN_VALUE Long/MAX_VALUE Long/MAX_VALUE))
3      ; correct value

Also, it seems that the reason it is returning these incorrect values is because super.count() is ASeq.count(), which starts at int 1, and then when it discovers that the remaining thing is a LongRange, which implemented Counted, it calls LongRange#count() and in turn LongRange#rangeCount() on a sequence one shorter. My debug prints in LongRange#count() confused me mightily until I figured out that this is what it is doing.

That also means that expressions like the one below overflow the stack, because of mutually recursive calls between LongRange#count() and ASeq#count().

user=> (count (range Long/MIN_VALUE 10000))
StackOverflowError   java.lang.Exception.<init> (Exception.java:66)

One way to correct the stack overflow issue would be to effectively copy Aseq#count() code into the place where LongRange#count() calls super.count().

If that loop was then modified to check for (i < 0), to detect overflow, and returning Integer.MAX_VALUE if that ever happened, that would also eliminate overflow for LongRange's (but not all ASeq's).

Comment by Steve Miner [ 10/May/15 3:28 PM ]

Good point about the case of a large step potentially causing the exception, in which case the range may still have a reasonable count.

Comment by Alex Miller [ 11/May/15 2:43 AM ]

New -3 patch changes the fallback to use the iterator. The iterator was also not safe from overflow, so I fixed that too.

For counting ranges > Integer/MAX_VALUE, now:

user=> (count (range Long/MIN_VALUE 0))
2147483647
Comment by Andy Fingerhut [ 12/May/15 10:27 AM ]

Re: clj-1727-4.patch
I, for one, will never be filing a bug that (count (range Long/MIN_VALUE Long/MAX_VALUE)) overflows a long

Comment by Steve Miner [ 12/May/15 3:44 PM ]

And I will resist suggesting a count' (ala inc' and dec'). Thanks for fixing these edge cases. The original report came from real life experience when I was trying to fix my own double math problems with TCHECK-67.





[CLJ-1726] New iterate and cycle impls have delayed computations but don't implement IPending Created: 06/May/15  Updated: 12/May/15  Resolved: 12/May/15

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

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

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

 Description   

When moving from LazySeqs to the new types we lost this but I don't think we should. Tools like Cursive use this for deciding when and how much to realize from a lazy sequence.

Approach:

  • iterate - The head of an iterate will always have the seed value and return 1 realized value. Subsequent elements will start unrealized and then become realized when the iterate function has been invoked to produce the value.
  • cycle - Returns unrealized if _current has been forced (initially null for all nodes after the first node).

(Note that range and repeat effectively always have their first element realized so I have chosen not to implement IPending - there is no delayed computation pending.)

;; setup
(def i (iterate inc 0))
(def c (cycle [1 2 3]))

user=>  (mapv realized? [i (next i) c (next c)])
[true false true false]
user=> (fnext i)
1
user=> (fnext c)
2
user=> (mapv realized? [i (next i) c (next c)])
[true true true true]

Patch: clj-1726-2.patch



 Comments   
Comment by Fogus [ 08/May/15 9:44 AM ]

There are three things that I like about this patch:

1) The implementations of the isRealize method provide meaning to the somewhat opaque encoding inherent in _current and (less opaque) UNREALIZED_SEED.

2) The use of realized? is generally useful outside of IDE contexts.

3) It's small and easy to grasp.





[CLJ-1723] NPE with eduction + cat on a collection containing nil Created: 04/May/15  Updated: 12/May/15  Resolved: 12/May/15

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

Type: Defect Priority: Critical
Reporter: Moritz Heidkamp Assignee: Alex Miller
Resolution: Completed Votes: 0
Labels: transducers

Attachments: Text File clj-1723.patch    
Patch: Code and Test
Approval: Ok

 Description   

Using the cat transducer with eduction leads to an NPE when the collection contains at least one collection with more than one item of which at least one is nil. The shortest reproduction case I could come up with is this:

(eduction cat [[nil nil]])

Cause: An expanding transducer (cat, mapcat) puts the expansion on an internal buffer, which is a ConcurrentLinkedQueue. Java Queue impls do not support adding or removing null b/c null is used as a special value in some of the Queue apis.

Approach: Switch from ConcurrentLinkedQueue to LinkedList. LinkedList supports both Queue and other semantics as well and does support nulls (with caveats that that is a bad thing to do if you're using the Queue apis and expecting those special semantics). However, the TransformerIterator usage does not rely on any of that. LinkedList is also obviously not concurrency friendly, but the buffer is only used by a single thread at a time and the volatile field guarantees visibility, so this is fine.

I re-ran some of the perf tests from CLJ-1669 and found the expanding transducer test there (into [] (eduction (map inc) (mapcat range) s50)) went from 27 us to 24 us, so there is a bit of a perf improvement as well.

Patch: clj-1723.patch



 Comments   
Comment by Alex Miller [ 04/May/15 12:03 PM ]

Gah, Java Queues don't allow null. I have some prior work on other impls for this so I'm working on a fix.

Comment by Fogus [ 08/May/15 9:49 AM ]

This is a very straight-forward solution that works and is easy to justify and grasp.





[CLJ-1716] IExceptionInfo should print with its ex-data Created: 26/Apr/15  Updated: 12/May/15  Resolved: 12/May/15

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

Type: Enhancement Priority: Major
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: reader

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

 Description   

The new (for 1.7) data-reader printing format for exceptions does not include the ex-data when relevant:

user> *clojure-version*
{:major 1, :minor 7, :incremental 0, :qualifier "beta2"}
user> (pr (ex-info "msg" {:my-data 42}))
#error {
 :cause "msg"
 :via
 [{:type clojure.lang.ExceptionInfo
   :message "msg"
   :at [clojure.core$ex_info invoke "core.clj" 4591]}]
 :trace
 [[clojure.core$ex_info invoke "core.clj" 4591]
  [user$eval9314 invoke "form-init6701752258113826186.clj" 1]
  ;; ...
]}

Approach: If ExceptionInfo is caught, also print :data key with ex-data. Include :data key for each ExceptionInfo in via.

After:

#error {
 :cause "msg"
 :data {:my-data 42}
 :via
 [{:type clojure.lang.ExceptionInfo
   :message "msg"
   :data {:my-data 42}
   :at [clojure.core$ex_info invoke "core.clj" 4591]}]
 :trace
 [[clojure.core$ex_info invoke "core.clj" 4591]
  [user$eval1 invoke "NO_SOURCE_FILE" 1]
  ;; elided
  ]}

Example with nested ExceptionInfo:

(try 
  (throw (ex-info "cause" {:a :b})) 
  (catch Exception e 
    (throw (ex-info "wrapped" {:c :d} e))))

;; yields:

#error {
 :cause "cause"
 :data {:a :b}
 :via
 [{:type clojure.lang.ExceptionInfo
   :message "wrapped"
   :data {:c :d}
   :at [clojure.core$ex_info invoke "core.clj" 4591]}
  {:type clojure.lang.ExceptionInfo
   :message "cause"
   :data {:a :b}
   :at [clojure.core$ex_info invoke "core.clj" 4591]}]
 :trace
 [[clojure.core$ex_info invoke "core.clj" 4591]
  [user$eval5 invoke "NO_SOURCE_FILE" 4]
  ;; elided
  ]}

Patch: CLJ-1716.patch



 Comments   
Comment by Gary Fredericks [ 26/Apr/15 9:30 PM ]

Added two patch files, the first with tests for the existing code, the second adding the :data key and extra tests for that.

Comment by Alex Miller [ 28/Apr/15 9:55 AM ]

Screening comments:

  • Combine the two patches into a single patch
  • Include :data for all ExceptionInfo in :via (when appropriate) (and leave it where it is)
  • when you git format-patch, throw -W on there to get more context (I think it would help in this case)
  • everything else looks good
Comment by Gary Fredericks [ 29/Apr/15 9:44 AM ]

Cool – I'm assuming "single patch" implies "single commit" since I couldn't find a way to dump two commits into one patch file.

Comment by Alex Miller [ 29/Apr/15 10:08 AM ]

yeah, single commit is what I meant. you can just commit multiple times and format-patch to get a single patch with multiple commits, but would prefer single.

Comment by Gary Fredericks [ 30/Apr/15 9:14 AM ]

Attached CLJ-1716.patch, which has just one commit, and now attaches data both to everything appropriate in :via and at the top level (so the data for the root cause is duplicated).

Added one more test case while I was at it.





[CLJ-1648] Use equals() instead of == when resolving Symbol Created: 22/Jan/15  Updated: 12/May/15  Resolved: 12/May/15

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

Type: Defect Priority: Critical
Reporter: Steven Yi Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: Compiler

Attachments: File resolve-symbol-equals.diff    
Patch: Code
Approval: Ok

 Description   

In Compiler.java, resolveSymbol() uses == to compare a Symbol's ns and the found namespace's name. This can result in a false comparison result, though the name's may be equal. In the following example:

ond.core=> (require '[clojure.string])
nil
ond.core=> `(clojure.string/join "," [1 2])
false : true ;; reported from System.out.println code I put into Compiler.java for == vs .equals()
nil

The result is that a new Symbol is allocated, when the previous one should be returned.

Prior to Clojure 1.7, Symbol name and ns were interned so == would actually have worked, but that is no longer the case.

Patch: resolve-symbol-equals.diff

Screened by: Alex Miller

[1] - https://groups.google.com/forum/#!topic/clojure-dev/58fYUSIEfxg






[CLJ-1195] emit-hinted-impl expands to non-ns-qualified invocation of 'fn' Created: 09/Apr/13  Updated: 12/May/15  Resolved: 12/May/15

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

Type: Defect Priority: Major
Reporter: Jason Wolfe Assignee: Unassigned
Resolution: Completed Votes: 11
Labels: None
Environment:

Mac os X Clojure 1.5.1 onwards.


Attachments: File extend-emits-qualified-fn.diff    
Patch: Code
Approval: Ok

 Description   
(ns plumbing.tmp
  (:refer-clojure :exclude [fn]))

(defprotocol Foo
  (foo [this]))

(extend-protocol Foo
  Object
  (foo [this]))

yields

CompilerException java.lang.RuntimeException: Unable to resolve symbol: fn in this context, compiling/Users/w01fe/prismatic/prismatic/plumbing/src/plumbing/tmp.clj:7:1)

This makes it difficult to construct a namespace that provides a replacement def for fn.



 Comments   
Comment by Jason Wolfe [ 21/Dec/14 9:36 PM ]

changes 'fn to `fn in two places in core_deftype.clj.

Comment by Jason Wolfe [ 21/Dec/14 9:40 PM ]

Attached patch extend-emits-qualified-fn.diff from 21 Dec 2014 replaces
'fn with `fn in two places in core_deftype.clj. Tests pass with this patch.
Also verified that there are no other places in Clojure where 'fn
is emitted.





[CLJ-1210] error message for (clojure.java.io/reader nil) — consistency for use with io/resource Created: 23/May/13  Updated: 11/May/15

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

Type: Enhancement Priority: Major
Reporter: Trevor Wennblom Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: checkargs, errormsgs, ft, io

Attachments: File extend-io-factory-to-nil.diff    
Patch: Code and Test
Approval: Triaged

 Description   

This seems to be a common idiom:

(clojure.java.io/reader (clojure.java.io/resource "myfile"))

When a file is available these are the behaviors:

=> (clojure.java.io/reader "resources/myfile")
#<BufferedReader java.io.BufferedReader@1f291df0>

=> (clojure.java.io/resource "myfile")
#<URL file:/project/resources/myfile>

=> (clojure.java.io/reader (clojure.java.io/resource "myfile"))
#<BufferedReader java.io.BufferedReader@1db04f7c>

If the file (resource) is unavailable:

=> (clojure.java.io/reader "resources/nofile")
FileNotFoundException resources/nofile (No such file or directory)  java.io.FileInputStream.open (FileInputStream.java:-2)

=> (clojure.java.io/resource "nofile")
nil

=> (clojure.java.io/reader (clojure.java.io/resource "nofile"))
IllegalArgumentException No implementation of method: :make-reader of protocol: #'clojure.java.io/IOFactory found for class: nil  clojure.core/-cache-protocol-fn (core_deftype.clj:541)
;; EXPECTED: better error type/message

The main enhancement request is to have a better error message from `(clojure.java.io/reader nil)`.

Approach: Extend IOFactory to nil, providing error messages consistent with the default error messages provided for Object. After:

user=> (clojure.java.io/reader (clojure.java.io/resource "nofile"))
IllegalArgumentException Cannot open <nil> as a Reader.  clojure.java.io/fn--9213 (io.clj:290)

Patch: extend-io-factory-to-nil.diff



 Comments   
Comment by Alexander Redington [ 14/Feb/14 3:13 PM ]

This patch extends IOFactory to nil, providing error messages consistent with the default error messages provided for Object.

Comment by Benjamin Peter [ 15/Feb/14 1:31 PM ]

Looks like a good solution to me as a user. Thanks for the effort!

Comment by Dennis Schridde [ 12/Jul/14 2:01 AM ]

I would also be interested in a solution, as I am currently running into this with the ClojureScript compiler.





[CLJ-1705] vector-of throws NullPointerException if given unrecognized type Created: 14/Apr/15  Updated: 06/May/15

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

Type: Defect Priority: Minor
Reporter: John Croisant Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: checkargs, errormsgs, newbie
Environment:

MacOS X Version 10.9.5

java version "1.8.0_11"
Java(TM) SE Runtime Environment (build 1.8.0_11-b12)
Java HotSpot(TM) 64-Bit Server VM (build 25.11-b03, mixed mode)


Attachments: Text File clj-1705-2.patch     Text File clj-1705-3.patch     Text File CLJ-1705.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Summary:

If the user passes an unrecognized type keyword to vector-of, it will throw a NullPointerException with no message. This gives no indication to the user of what the problem is, which can frustrate the user and make debugging harder than it needs to be.

Example:

user=> (vector-of :integer 1 2 3) ; user meant (vector-of :int 1 2 3)

NullPointerException   clojure.core/vector-of (gvec.clj:472)

Approach: Throw more informative error message than NPE with more info.

After:

user=> (vector-of :integer 1 2 3)
IllegalArgumentException Unrecognized type :integer  clojure.core/vector-of (gvec.clj:498)

Patch: clj-1705-3.patch



 Comments   
Comment by Johan Mena [ 05/May/15 12:12 AM ]

Here’s my proposed solution: https://github.com/jhn/clojure/commit/0522fffd7893e8c79969f5c6ac91a333484c8e23 — It works as expected and the tests pass.

One more thing I'm not sure about is how to create a patch for this. The ticket mentions "Release 1.4, Release 1.6” as affected versions, but I found this to still be present in 1.7 and in 1.5 too. Do I checkout only these two tags (1.4, 1.6) from the git repo and create a patch for each one separately? Or what's the usual way to go about it? I read through the Issue Tracking section of the wiki but it's still not very clear.

Thanks.

Johan

Comment by Alex Miller [ 05/May/15 7:09 AM ]

in general, just work from master. We don't generally back port to older versions.

You should make a patch following the instructions at http://dev.clojure.org/display/community/Developing+Patches

Comment by Andy Fingerhut [ 05/May/15 10:20 AM ]

Johan, are you listed as "johan (jhn)" on the contributor list here? http://clojure.org/contributing

If so, it would be good to update that listing to something like "Johan Mena (jhn)" for the time when someone needs to check that you have signed a Clojure CA, before your patch is committed.

Comment by Alex Miller [ 05/May/15 10:35 AM ]

I believe that's correct - I have updated the contributing page.

Comment by Johan Mena [ 05/May/15 1:57 PM ]

The patch I attached yesterday did start from master, so I think it's good to go for review.

Thanks Alex & Andy!

Comment by Alex Miller [ 06/May/15 9:56 AM ]

Ok, I looked at the patch. Functionally, seems ok other than that the find+destructuing doesn't seem to have a use over get.

However, I also looked at performance for the happy path using criterium and the following test:

(quick-bench (vector-of :int 1 2))

Before the patch: 27.418350 ns
After the patch: 79.883695 ns

The reason for this is that the prior code created a single map and constructed the array managers in there once, whereas the patch causes the array manager to be created each time through the code.

Re-extracting the map and replacing the find with get, I was back to: 34.320916 ns

I think that's too much of a hit for this change so I looked at a couple variants:

  • def'ing each am separately and using a `case` expression - 36.566750 ns
  • using `or` instead of if-let - 29.680252 ns

I think that last one is pretty close, but we're paying a hit just to invoke the new function, so my final stab was turning that into a macro - 28.411626 ns, which seems tolerable to me. Because I used a macro, I also had to move the type hints in vector-of to avoid reflection.

Since I had everything sitting here, I went ahead and made a new patch. I feel bad about replacing yours, but not sure what else makes sense to do.

Comment by Alex Miller [ 06/May/15 10:02 AM ]

Ok, I split up the patch into test and code commits so you have credit for some of the changes! Since it's got my stuff in here, this will have to wait till it gets added to 1.8 to get screened by someone else.

Comment by Johan Mena [ 06/May/15 11:04 AM ]

Haha, you shouldn't have bothered, but thanks!

And thanks for the detailed explanation! Actually 'get' was my first approach too but the (get map key not-found) form kept throwing the exception in the `not-found` part even in the happy case. I asked around and found out the else part needs to be evaluated in this case too. Someone suggested using a macro but I'm not very familiar with that just yet, so I went with if-let, which seemed readable. Just out of curiosity, when you tried the get approach, did you use (get map key) and check for nil to throw the exception?

I ran my code through the irc channel and got positive feedback, so completely forgot about performance. Will keep it in mind next time.





[CLJ-1385] Docstrings for `conj!` and `assoc!` should suggest using the return value; effect not always in-place Created: 16/Mar/14  Updated: 06/May/15

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

Type: Enhancement Priority: Minor
Reporter: Pyry Jahkola Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: collections, docstring, ft

Attachments: Text File CLJ-1385-reword-docstrings-on-transient-update-funct-2.patch     Text File CLJ-1385-reword-docstrings-on-transient-update-funct.patch    
Patch: Code
Approval: Triaged

 Description   

The docstrings of both `assoc!` and `conj!` say "Returns coll.", possibly suggesting the transient edit happens (always) in-place, `coll` being the first argument. However, this is not the case and the returned collection should always be the one that's used.

Approach: Replace "Returns coll." with "Returns an updated collection." in `conj!`, `assoc!`, `pop!` docstrings.

Patch: CLJ-1385-reword-docstrings-on-transient-update-funct-2.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 16/Mar/14 8:49 AM ]

When modifying transient collections, it is required to use the collection returned from functions like assoc!. The ! here indicates its destructive nature. The transients page (http://clojure.org/transients) describes the calling pattern pretty explicitly: "You must capture and use the return value in the next call."

I do not agree that we should be guiding programmers away from using functions like assoc! – transients are used as a performance optimization and using assoc! or conj! in a loop is often the fastest version of that. However I do think it would be helpful to make the docstring more explicit.

Comment by Gary Fredericks [ 05/Apr/14 10:23 AM ]

Alex I think you must have misread the ticket – the OP is suggesting guiding toward using the return value of assoc!, not avoiding assoc! altogether.

And the docstring is not simply inexplicit, it's actually incorrect specifically in the case that the OP pointed out. conj! and assoc do not return coll at the point where array-maps transition to hash-maps, and the fact that they do otherwise is supposed to be an implementation detail as far as I understand it.

Comment by Alex Miller [ 05/Apr/14 11:55 AM ]

@Gary - you're right, I did misread that.

assoc and conj both explicitly say "return a new collection" whereas assoc! and conj! say "Returns coll." I read that as "returns the modified collection" without regard to whether it's the identical instance, but I can read it your way too.

Would saying "Returns updated collection." transmit the right idea? Using "collection" instead of "coll" removes the concrete tie to the variable and "updated" hints more strongly that you should use the return value.

Comment by Pyry Jahkola [ 05/Apr/14 12:47 PM ]

@Alex, that update makes it sound right to me, FWIW.

Comment by Gary Fredericks [ 05/Apr/14 2:37 PM ]

Yeah, I think that's better. Thanks Alex. I'd be happy to submit a patch for that but I'm assuming patches are too heavy for this kind of change?

Comment by Andy Fingerhut [ 06/Apr/14 3:35 PM ]

Patches are exactly what has been done in the past for this kind of change, if it is in a doc string and not on the clojure.org web page.

Comment by Alex Miller [ 06/Apr/14 4:13 PM ]

Yup, patch desired.

Comment by Gary Fredericks [ 06/Apr/14 5:32 PM ]

Glad I asked.

Patch is attached that also updates the docstring for pop! which had the same issue, though arguably it's less important since afaik pop! does always return the identical collection (but I don't think this is part of the contract).

Comment by Andy Fingerhut [ 06/Aug/14 2:14 PM ]

Patch CLJ-1385-reword-docstrings-on-transient-update-funct.patch dated Apr 6 2014 no longer applies to latest Clojure master cleanly, due to some changes committed earlier today. I suspect it should be straightforward to update the patch to apply cleanly, given that they are doc string changes, but there may have been doc string changes committed to master, too.

Comment by Gary Fredericks [ 06/Aug/14 3:04 PM ]

Attached a new patch.





[CLJ-970] extend/implement parameterized types (generics) Created: 10/Apr/12  Updated: 06/May/15

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

Type: Enhancement Priority: Minor
Reporter: Jim Blomo Assignee: Jim Blomo
Resolution: Unresolved Votes: 1
Labels: interop

Attachments: Text File clj-970-extend-implement-parameterized-types-patch2.txt     File extend-implement-parameterized-types.diff    
Patch: Code and Test
Approval: Triaged

 Description   

When extending parameterized types, class files can track the original signatures of the superclass and super interfaces so that the original types can be obtained at run time. This runtime reflection is used in some Java frameworks, and implementing it in Clojure can enable interop. See http://groups.google.com/group/clojure/browse_thread/thread/5efd692804df3f47/1336e591c2eedfa1 for examples of this request.

This proposal checks the :parameters keyword in type meta information. If a parameter is found, it is added to the class signature.



 Comments   
Comment by Jim Blomo [ 14/Apr/12 11:30 AM ]

2012-04-14 extend-implement-parameterized-types.diff is the correctly formatted `git format-patch master` for this change. It supersedes clojure-parameterized-generics.diff from 2012-04-10.

Comment by Andy Fingerhut [ 19/Aug/12 5:00 AM ]

Patch clj-970-extend-implement-parameterized-types-patch2.txt dated Aug 19 2012 is identical to Jim Blomo's patch extend-implement-parameterized-types.diff dated Apr 14 2012, except it has updated context lines so that it applies cleanly to latest master as of today.





[CLJ-1718] (if test then else?) is inconsistent Created: 30/Apr/15  Updated: 05/May/15  Resolved: 30/Apr/15

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

Type: Defect Priority: Major
Reporter: Jigen Daisuke Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

MacOS X 10.10.2



 Description   

Issue CLJ-1640 was "fixed" by adding a comment to the documentation.
I think that's not enough, because CLJ-1640 actually points to a major inconsistency.
Just have a look at the following code:

;; I know, the definition of y is bad code, and I would
;; never write such rubish, BUT some people do (e.g. the
;; guys at MS, responsible for the sqljdbc4.jar JDBC
;; driver).
(def y (Boolean. false))

(if (= false y)
(println "that's ok"))

(if y
(println "that's inconsistent, because y is " y
" and we proved it with the above if statement"))



 Comments   
Comment by Alex Miller [ 30/Apr/15 3:25 PM ]

CLJ-1640 was closed by the submitter, no change was made for that.

The decision was made long ago by Clojure to canonicalize on Boolean/FALSE as the false value. We do not plan to make any changes in this regard.

In cases where you are dealing with Java libraries that construct new Boolean instances rather than use the canonical ones, you are responsible for normalizing these values. Please file issues with those libraries as appropriate.

Comment by Andy Fingerhut [ 30/Apr/15 3:41 PM ]

The longish example/article on ClojureDocs.org might contain additional useful info. Even Java recommends against ever using (Boolean. false): http://clojuredocs.org/clojure.core/if

Comment by Jigen Daisuke [ 30/Apr/15 5:08 PM ]

But, if Boolean/FALSE is the false value, shouldn't

(if (= (Boolean. false) false) true false)

better return false?

Comment by Andy Fingerhut [ 30/Apr/15 6:12 PM ]

These all return 2:

(if nil 1 2)
(if false 1 2)
(if Boolean/FALSE 1 2)

clojure.core/=, like Java .equals, returns true when comparing Boolean/FALSE and (Boolean. false). Blame Java.

Comment by Jigen Daisuke [ 05/May/15 4:51 PM ]

@Andy Fingerhut

I know all that, but this doesn't make it any more consistent

See, if they won't fix how "if" treats custom made false-values, the next best
thing to do were to manifest this behaviour of "if" into the "=" function (and
yes, that would mean that "=" isn't a mere ".equals" call anymore, because
Boolean false-values would have to be treated in a special way). But then at
least we would have a consistent system - though not exactly in the way I'd
prefer.

Therefore, I can't blame Java. If they had implemented "if" right in the first place,
everything were fine. So this one is on Clojure.

Comment by Andy Fingerhut [ 05/May/15 5:50 PM ]

I'd like to withdraw the last 2 sentences of my previous comment. I think Alex's comment is the shortest accurate answer: It was a choice in Clojure's implementation to do this. As it is right now, there are some compiled 'if' expressions that compare the test expression against both nil (Java null) and Java Boolean.FALSE, and allowing (Boolean. false) to also be treated as false would either require comparing against a third value, or calling a method like Boolean.valueOf() before doing the comparison. Perhaps a micro-optimization, but seems to me like a reasonable one, given the recommendations in Java not to use the Boolean constructors.





[CLJ-1671] Clojure stream socket repl Created: 09/Mar/15  Updated: 05/May/15

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

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

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

 Description   

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

The goal is to provide a simple streaming socket repl as part of Clojure. The socket repl should support both program-to-program communication (by exchanging data) and human-readable printing (which mirrors current behavior). The socket repl server will be started only when supplying a port to clojure.main or by explicitly starting it by calling the provided function.

Each socket connection will be given its own repl context and unique starting user namespace (user, user1, user2, etc) with separate repl stack and proper bindings. On session termination, the namespace will be removed. *in* and *out* will be bound to incoming and outgoing streams. Tools can communicate with the runtime while also providing a user repl by opening two connections to the same server.

There are two known cases where the repl interprets non-readable objects and prints them for human consumption in a non-readable way: objects with no specific print-method and when handling Throwables. Both cases (Object and Throwable) now have a print-method implementation to return a tagged-literal representation. By default the socket repl will print exceptions with the print-method data form. Optionally, the user can set a custom repl exception printer. A function that provides the current human readable exception printing will be provided.

Problems:

  1. Socket server to accept connections and serve a repl to a client
    • Runtime configuration via data
  2. Client repl sessions should be independent
    • Separate user namespace
    • Separate bindings
    • Namespace removed on client shutdown
    • Communicate solely via data for program-to-program communication
  3. Stdio has both out and err streams but socket has only single out
  4. Repls should be nestable
    • Repl within a repl binds streams appropriately
    • Means of control (exit)

Features:

  1. Printing as data
    • Of object without print-method: as #object
    • Of Throwable: as #error
  2. Start socket server from command line
    • Configure: host, port, whether to prompt, error printing
  3. Start socket server programmatically
    • Configure: host, port, whether to prompt, error printing, whether to bind err to out
    • Control: close returned socket server to stop listening
  4. Start stdio repl from command line
    • Configure: whether to prompt, error printing
  5. On socket client accept
    • Create new user namespace
    • Bind error printer according to server config
  6. In socket client repl
    • Bind new error printer function
  7. On socket client disconnect
    • Remove user namespace
    • Close socket

Impl notes: This adds a new -s option to clojure.main that will start a socket server listening on a given host:port. Each client is given a new userN namespace (starting from user1). It binds *in*, *out*, and *err*. Each client connection consumes a daemon thread named "Socket REPL Client N" (matching the user namespace). On client disconnect, the user namespace is removed and thread will die.

There are two system properties that can be used to control whether the prompt and the default error printer:

  • clojure.repl.socket.prompt (default=false) - whether to print prompts
  • clojure.repl.socket.err-printer (default=clojure.main/err->map) - function to format exceptions

The existing stdio repl behaves the same as before, but it's behavior can be influenced by two new similar system properties:

  • clojure.repl.stdio.prompt (default=true)
  • clojure.repl.socket.err-printer (default=clojure.main/err-print)

You can also start a socket repl server programmatically (shown here with all kwarg options - pick the ones you need):

(def ss 
  (clojure.main/socket-repl-server 
    :host "localhost"                   ;; default=<loopback>, accepts InetAddress or String
    :port 5555                          ;; default=0 (ephemeral)
    :use-prompt true                    ;; default=false
    :bind-err true                      ;; default=true, to bind \*err* to \*out*
    :err-printer clojure.main/err->map  ;; default=clojure.main/err->map, print Throwable to \*out*
    ))

The server socket is returned. Closing it will stop listening on the port (existing client connections will still be alive).

If you want to test with the server port, telnet makes a great client:

;; term 1:
$ java -cp target/classes -Dclojure.repl.socket.prompt=true clojure.main -s 127.0.0.1:5555

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

A dynamic var clojure.main/*err-printer* is provided to customize printing of exceptions. It's bound by :err-printer if invoked programmatically or the system property if started from the command line, but it can be dynamically rebound during the session if desired:

user1=> (set! clojure.main/*err-printer* clojure.main/err-print)
#object[clojure.main$err_print 0x317bee01 "clojure.main$err_print@317bee01"]
user1=> (/ 1 0)
ArithmeticException Divide by zero  clojure.lang.Numbers.divide (Numbers.java:158)

Patch: clj-1671-4.patch



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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

New -4 patch changes:

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

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

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

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

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

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

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

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

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

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

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

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

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

Marking incomplete, pending at least the repl exit question.

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

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

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

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

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

OK thanks for the update.

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

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

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

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

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

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





[CLJ-1473] Bad pre/post conditions silently passed Created: 24/Jul/14  Updated: 04/May/15

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

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: errormsgs, ft

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

 Description   

Before:

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

After:

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

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



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

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

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

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

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

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

Test should be improved as it could have caught that.

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

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





[CLJ-700] contains? broken for transient collections Created: 01/Jan/11  Updated: 04/May/15

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

Type: Defect Priority: Critical
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 17
Labels: transient

Attachments: Java Source File 0001-Refactor-of-some-of-the-clojure-.java-code-to-fix-CL.patch     File clj-700-7.diff     File clj-700-8.diff     Text File clj-700-9.patch     File clj-700.diff     Text File clj-700-patch4.txt     Text File clj-700-patch6.txt     Text File clj-700-rt.patch    
Patch: Code and Test
Approval: Vetted

 Description   

Behavior with Clojure 1.6.0:

user=> (contains? (transient {:x "fine"}) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentArrayMap$TransientArrayMap  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient (hash-map :x "fine")) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentHashMap$TransientHashMap  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient [1 2 3]) 0)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentVector$TransientVector  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient #{:x}) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentHashSet$TransientHashSet  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (:x (transient #{:x}))
nil
;; expected: :x

user=> (get (transient #{:x}) :x)
nil
;; expected: :x

Cause: This is caused by expectations in clojure.lang.RT regarding the type of collections for some methods, e.g. contains() and getFrom(). Checking for contains looks to see if the instance passed in is Associative (a subinterface of PersistentCollection), or IPersistentSet.

Approach: Expand the types that RT.getFrom(), RT.contains(), and RT.find() can handle to cover the additional transient interfaces.

Alternative: Other older patches (prob best exemplified by clj-700-8.diff) restructure the collections type hierarchy. That is a much bigger change than the one taken here but is perhaps a better long-term path. That patch refactors several of the Clojure interfaces so that logic abstract from the issue of immutability is pulled out to a general interface (e.g. ISet, IAssociative), but preserves the contract specified (e.g. Associatives only return Associatives when calling assoc()). With more general interfaces in place the contains() and getFrom() methods were then altered to conditionally use the general interfaces which are agnostic of persistence vs. transience.

Patch: clj-700-9.patch



 Comments   
Comment by Herwig Hochleitner [ 01/Jan/11 8:01 PM ]

the same is also true for TransientVectors

{{(contains? (transient [1 2 3]) 0)}}

false

Comment by Herwig Hochleitner [ 01/Jan/11 8:25 PM ]

As expected, TransientSets have the same issue; plus an additional, probably related one.

(:x (transient #{:x}))

nil

(get (transient #{:x}) :x)

nil

Comment by Alexander Redington [ 07/Jan/11 2:07 PM ]

This is caused by expectations in clojure.lang.RT regarding the type of collections for some methods, e.g. contains() and getFrom(). Checking for contains looks to see if the instance passed in is Associative (a subinterface of PersistentCollection), or IPersistentSet.

This patch refactors several of the Clojure interfaces so that logic abstract from the issue of immutability is pulled out to a general interface (e.g. ISet, IAssociative), but preserves the contract specified (e.g. Associatives only return Associatives when calling assoc()).

With more general interfaces in place the contains() and getFrom() methods were then altered to conditionally use the general interfaces which are agnostic of persistence vs. transience. Includes tests in transients.clj to verify the changes fix this problem.

Comment by Stuart Halloway [ 28/Jan/11 10:35 AM ]

Rich: Patch doesn't currently apply, but I would like to get your take on approach here. In particular:

  1. this represents working back from the defect to rethinking abstractions (good!). Does it go far enough?
  2. what are good names for the interfaces introduced here?
Comment by Alexander Redington [ 25/Mar/11 7:44 AM ]

Rebased the patch off the latest pull of master as of 3/25/2011, it should apply cleanly now.

Comment by Stuart Sierra [ 17/Feb/12 2:59 PM ]

Latest patch does not apply as of f5bcf647

Comment by Andy Fingerhut [ 17/Feb/12 5:59 PM ]

clj-700-patch2.txt does patch cleanly to latest Clojure head as of a few mins ago. No changes to patch except in context around changed lines.

Comment by Andy Fingerhut [ 07/Mar/12 3:23 AM ]

Sigh. Git patches applied via 'git am' are fragile beasts indeed. Look at them the wrong way and they fail to apply.

clj-700-patch3.txt applies cleanly to latest master as of Mar 7, 2012, but not if you use this command:

git am -s < clj-700-patch3.txt

I am pretty sure this is because of DOS CR/LF line endings in the file src/jvm/clojure/lang/Associative.java. The patch does apply cleanly if you use this command:

git am --keep-cr -s < clj-700-patch3.txt

Comment by Andy Fingerhut [ 23/Mar/12 6:34 PM ]

This ticket was changed to Incomplete and waiting on Rich when Stuart Halloway asked for feedback on the approach on 28/Jan/2011. Stuart Sierra changed it to not waiting on Rich on 17/Feb/2012 when he noted the patch didn't apply cleanly. Latest patch clj-700-patch3.txt does apply cleanly, but doesn't change the approach used since the time Stuart Halloway's concern was raised. Should it be marked as waiting on Rich again? Something else?

Comment by Stuart Halloway [ 08/Jun/12 12:44 PM ]

Patch 4 incorporates patch 3, and brings it up to date on hashing (i.e. uses hasheq).

Comment by Andy Fingerhut [ 08/Jun/12 12:52 PM ]

Removed clj-700-patch3.txt in favor of Stuart Halloway's improved clj-700-patch4.txt dated June 8, 2012.

Comment by Andy Fingerhut [ 18/Jun/12 3:06 PM ]

clj-700-patch5.txt dated June 18, 2012 is the same as Stuart Halloway's clj-700-patch4.txt, except for context lines that have changed in Clojure master since Stuart's patch was created. clj-700-patch4.txt no longer applies cleanly.

Comment by Andy Fingerhut [ 19/Aug/12 4:47 AM ]

Adding clj-700-patch6.txt, which is identical to Stuart Halloway's clj-700-patch4.txt, except that it applies cleanly to latest master as of Aug 19, 2012. Note that as described above, you must use the --keep-cr option to 'git am' when applying this patch for it to succeed. Removing clj-700-patch5.txt, since it no longer applies cleanly.

Comment by Stuart Sierra [ 24/Aug/12 1:08 PM ]

Patch fails as of commit 1c8eb16a14ce5daefef1df68d2f6b1f143003140

Comment by Andy Fingerhut [ 24/Aug/12 1:53 PM ]

Which patch did you try, and what command did you use? I tried applying clj-700-patch6.txt to the same commit, using the following command, and it applied, albeit with the warning messages shown:

% git am --keep-cr -s < clj-700-patch6.txt
Applying: Refactor of some of the clojure .java code to fix CLJ-700.
/Users/jafinger/clj/latest-clj/clojure/.git/rebase-apply/patch:29: trailing whitespace.
public interface Associative extends IPersistentCollection, IAssociative{
warning: 1 line adds whitespace errors.
Applying: more CLJ-700: refresh to use hasheq

Note the --keep-cr option, which is necessary for this patch to succeed. It is recommended in the "Screening Tickets" section of the JIRA workflow wiki page here: http://dev.clojure.org/display/design/JIRA+workflow

Comment by Andy Fingerhut [ 28/Aug/12 5:48 PM ]

Presumptuously changing Approval from Incomplete back to None, since the latest patch does apply cleanly if the --keep-cr option is used. It was in Screened state recently, but I'm not so presumptuous as to change it to Screened

Comment by Alex Miller [ 19/Aug/13 12:26 PM ]

I think through a series of different hands on this ticket it got knocked way back in the list. Re-marking vetted as it's previously been all the way up through screening. Should also keep an eye on CLJ-787 as it may have some collisions with this one.

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

clj-700-7.diff is identical to clj-700-patch6.txt, except it applies cleanly to latest master. Only some lines of context in a test file have changed.

When I say "applies cleanly", I mean that there is one warning when using the proper "git am" command from the dev wiki page. This is because one line replaced in Associative.java has a CR/LF at the end of the line, because all lines in that file do.

Comment by Herwig Hochleitner [ 17/Feb/14 9:54 AM ]

Since clojure 1.5, contains? throws an IllegalArgumentException on transients.
In 1.6.0-beta1, transients are no longer marked as alpha.

Does this mean, that we won't be able to distinguish between a nil value and no value on a transient?

Comment by Stuart Halloway [ 27/Jun/14 10:20 AM ]

Request for someone to (1) update patch to apply cleanly, and (2) summarize approach so I don't have to read through the comment history.

Comment by Andy Fingerhut [ 27/Jun/14 11:02 AM ]

The latest patch is clj-700-7.diff dated Nov 8, 2013. I believe it is impossible to create a patch that applies any more cleanly using git for source files that have carriage returns in them, which at least one modified source file does. Here is the command I used on latest Clojure master as of today (Jun 27 2014), which is the same as that of March 25 2014:

% git am -s --keep-cr --ignore-whitespace < ~/clj/patches/clj-700-7.diff 
Applying: Refactor of some of the clojure .java code to fix CLJ-700.
/Users/admin/clj/latest-clj/clojure/.git/rebase-apply/patch:29: trailing whitespace.
public interface Associative extends IPersistentCollection, IAssociative{
warning: 1 line adds whitespace errors.
Applying: more CLJ-700: refresh to use hasheq

If you want a patch that doesn't have the 'trailing whitespace' warning in it, I think someone would have to commit a change that removed the carriage returns from file Associative.java. If you want such a patch, let me know and we can remove all of them from every source file and be done with this annoyance.

Comment by Andy Fingerhut [ 27/Jun/14 11:19 AM ]

Updated description to contain a copy of only those comments that seemed 'interesting'. Most comments have simply been "attached an updated patch that applies cleanly", or "changed the state of this ticket for reason X".

Comment by Alex Miller [ 27/Jun/14 1:19 PM ]

Looks like Andy did as requested, moving back to Screenable.

Comment by Andy Fingerhut [ 29/Aug/14 4:27 PM ]

Patch clj-700-7.diff dated Nov 8 2013 no longer applied cleanly to latest master after some commits were made to Clojure on Aug 29, 2014. It did apply cleanly before that day.

I have not checked how easy or difficult it might be to update this patch.

Comment by Andy Fingerhut [ 01/Sep/14 3:59 AM ]

Patch clj-700-8.diff dated Sep 1 2014 is identical to clj-700-7.diff, except that it applies "cleanly" to latest master, by which I mean it applies as cleanly as I think it is possible to apply for a git patch to a file with carriage return/line feed line endings, as one of the modified files still does.

Comment by Alex Miller [ 17/Dec/14 3:12 PM ]

Added new patch with alternate approach that just makes RT know about transients instead of refactoring the class hierarchy.

clj-700-rt.patch

In some ways I think the class hierarchy refactoring is due, but I'm not totally on board with all the changes in those patches and it has impacts on collections outside Clojure itself that are hard to reason about.





[CLJ-1673] Improve clojure.repl/dir-fn to work on namespace aliases in addition to canonical namespaces. Created: 11/Mar/15  Updated: 04/May/15

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

Type: Enhancement Priority: Minor
Reporter: Jason Whitlark Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: ft, repl

Attachments: Text File clj-1673-2.patch     Text File improve_dir.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Extend clojure.repl/dir to work with the aliases in the current namespace

After:

user=> (require '[clojure.string :as s])
nil
user=> (dir s)
blank?
capitalize
...etc

Patch: clj-1673-2.patch
Screened by: Alex Miller



 Comments   
Comment by Jason Whitlark [ 11/Mar/15 4:00 PM ]

Possible unit test, since clojure.string is aliased in the test file:

(is (= (dir-fn 'clojure.string) (dir-fn 'str)))

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

Please add a test...

Comment by Jason Whitlark [ 02/May/15 10:35 PM ]

Updated patch, tweaked dir-fn, added test.

Comment by Jason Whitlark [ 02/May/15 10:38 PM ]

Should be good to go. I removed the old patch.

Comment by Alex Miller [ 04/May/15 4:38 PM ]

Tweaked patch just to remove errant blank line, otherwise same.





[CLJ-1725] Add missing transducer creating functions to clojure.org/transducers Created: 04/May/15  Updated: 04/May/15  Resolved: 04/May/15

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

Type: Enhancement Priority: Minor
Reporter: A. R Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

The following should be added to the list:

  • distinct
  • interpose
  • map-indexed


 Comments   
Comment by Alex Miller [ 04/May/15 3:15 PM ]

Thanks, I'll take care of that.

Comment by A. R [ 04/May/15 4:05 PM ]

Nevermind this comment. Had an old version.

Comment by Alex Miller [ 04/May/15 4:29 PM ]

Added.





[CLJ-735] Improve error message when a protocol method is not found Created: 04/Feb/11  Updated: 04/May/15

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: errormsgs

Attachments: File protocolerr.diff    
Patch: Code
Approval: Triaged

 Description   

If you call a protocol function but pass the wrong arity (forget an argument for example), you currently a message that says "No single method ... of interface ... found for function ... of protocol ...". The code in question is getting matching methods from the Reflector and creates this message if the number of matches != 1.

There are really two cases there:

  • matches == 0 - this happens frequently due to typos
  • matches > 1 - this presumably happens infrequently

I propose that the == 0 case instead should have slightly different text at the beginning and a hint as to the intended arity within it:

"No method: ... of interface ... with arity ... found for function ... of protocol ...".

The >1 case should have similar changes: "Multiple methods: ... of interface ... with arity ... found for function ... of protocol ...".

Patch is attached. I used case which presumably should have better performance than a nested if/else. I was not sure whether the reported arity should match the actual Java method arity or Clojure protocol function arity (including the target). I did the former.

I did not add a test as I wasn't sure whether checking error messages in tests was appropriate or not. Happy to add that if requested.



 Comments   
Comment by Chas Emerick [ 14/Jul/11 6:39 AM ]

I was not sure whether the reported arity should match the actual Java method arity or Clojure protocol function arity (including the target). I did the former.

I think it should be the latter. The message is emitted when the protocol methods are being invoked through the corresponding function, so it should be consistent with the errors emitted by regular functions.

+1 for some tests, too. There certainly are tests for reflection warnings and such.

FWIW, I'm happy to take this on if Alex is otherwise occupied.

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

Picking this up a zillion years later ...

1) I think I would now change the control flow slightly to make the normal path (methods.size() == 1) first and the error cases after that without that switch/case stuff:

if(methods.size() == 1) { 
  this.onMethod = (java.lang.reflect.Method) methods.get(0);
} else if(methods.size() == 0) {
  ...
} else {
  ...
}

2) The Compiler code should use tabs instead of spaces.

3) I would like tests added for the error messages in these two cases. Those can go in clojure.test-clojure.protocols.





[CLJ-1527] Harmonize accepted / documented symbol and keyword syntax over various readers Created: 18/Sep/14  Updated: 04/May/15

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

Type: Defect Priority: Major
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 9
Labels: reader

Approval: Triaged

 Description   

Documentation Issues

http://clojure.org/reader#The%20Reader--Reader%20forms is ambigous on whether foo/bar/baz is allowed. Also, it doesn't mention the tick ' as a valid constituent character.
The EDN spec also currently omits ', ticket here: https://github.com/edn-format/edn/issues/67

Implementation Issues

clojure.core/read, as well as clojure.edn/read accept symbols like foo/bar/baz, even though they should be rejected.

References

https://groups.google.com/d/topic/clojure-dev/b09WvRR90Zc/discussion
Related ticket: CLJ-1286



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

The Clojure reader documentation also does not mention the following symbols as valid constituent characters. They are all mentioned as valid symbol constituent characters in the EDN readme here: https://github.com/edn-format/edn#symbols

dollar sign - used in Clojure/JVM to separate Java subclass names from class names, e.g. java.util.Map$Entry
percent sign - not sure why this is part of edn spec. In Clojure it seems only to be used inside #() for args like % %1 %&
ampersand - like in &form and &env in macro definitions
equals - clojure.core/= and many others
less-than - clojure.core/< clojure.core/<=
greater-than - clojure.core/> clojure.core/>=

I don't know whether Clojure and edn specs should be the same in this regard, but it seemed worth mentioning for this ticket.





[CLJ-1225] quot overflow issues around Long/MIN_VALUE for BigInt Created: 25/Jun/13  Updated: 04/May/15

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

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

Attachments: Text File clj-1225-2.txt     Text File clj-1225-fix-division-overflow-patch-v1.txt    
Patch: Code and Test
Approval: Triaged

 Description   

In Clojure 1.5.1, see the following undesirable behavior regarding incorrect quot results for BigInts:

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

Similar issue to CLJ-1222. The root cause is that Java division of longs gives a numerically incorrect answer of Long.MIN_VALUE for (Long.MIN_VALUE / -1), because the numerically correct answer does not fit in a long. I believe this is the only pair of arguments for long division that gives a numerically incorrect answer, because division with a denominator having an absolute value of 2 or more gives a result closer to 0 than the numerator, and everything works fine for a denominator of 1 or -1, except this one case.

Related issues: CLJ-1222 for multiply, CLJ-1253 for / on longs, CLJ-1254 for quot on longs

Patch: clj-1225-2.txt
Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 25/Jun/13 11:03 AM ]

Patch clj-1225-fix-division-overflow-patch-v1.txt dated Jun 25 2013 may be one good way to address this issue. It modifies quot and / to return the numerically correct (BigInt) answer when given args Long/MIN_VALUE and -1.

It also removes the quotient intrinsic that does a JVM LDIV operation on longs for quot, since that operation is one of those that gives the incorrect result. I have not done any performance testing with this patch yet, but I have verified that it does not introduce any new reflection warnings when compiling Clojure itself.

Comment by Andy Fingerhut [ 25/Jun/13 11:13 AM ]

Another possible approach would be to create unchecked-quotient and quot', which together with quot would correspond to the existing unchecked-multiply, *' and *. That is a more significant change. One potential concern it addresses that patch clj-1225-fix-division-overflow-patch-v1.txt does not is that patch leaves a Clojure developer with no way to do a primitive Java long division except by writing Java code.

Comment by Rich Hickey [ 05/Sep/13 8:39 AM ]

this is two separate issues, one with longs and one with bigints. long problem should throw

Comment by Andy Fingerhut [ 06/Sep/13 12:37 AM ]

Updating description for BigInt issue only. Will create separate ticket for incorrect behavior of / and quot on long type args Long/MIN_VALUE and -1.

Comment by Andy Fingerhut [ 06/Sep/13 12:41 AM ]

Patch clj-1225-2.txt fixes this issue with quot on BigInts, with tests for quot and / on these values. / on BigInt worked fine before, but added the tests in case someone decides to change the implementation and forgets this corner case.





[CLJ-1724] Reuse call to seq() in LazySeq/hashcode for else case Created: 04/May/15  Updated: 04/May/15

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

Type: Enhancement Priority: Minor
Reporter: Jozef Wagner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections, ft, performance

Attachments: File clj-1724.diff    
Patch: Code
Approval: Triaged

 Description   

In LazySeq/hashCode, seq() is called twice for non-empty seqs. First call to seq() can be reused in else case.






[CLJ-1373] LazySeq should utilize cached hash from its underlying seq. Created: 09/Mar/14  Updated: 04/May/15

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

Type: Enhancement Priority: Major
Reporter: Jozef Wagner Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: collections, performance
Environment:

1.6.0 master SNAPSHOT


Attachments: File clj-1373-2.diff     File clj-1373.diff    
Patch: Code
Approval: Triaged

 Description   

Even if underlying seq contains a cached hash, LazySeq computes it every time.

user=> (def a (range 100000))
#'user/a
user=> (time (hash a))
"Elapsed time: 46.904273 msecs"
375952610
user=> (time (hash a)) ;; RECOMPUTE
"Elapsed time: 10.879098 msecs"
375952610
user=> (def b (seq a))
#'user/b
user=> (time (hash b))
"Elapsed time: 10.572522 msecs"
375952610
user=> (time (hash b)) ;; CACHED HASH
"Elapsed time: 0.024927 msecs"
375952610
user=> (def c (lazy-seq b))
#'user/c
user=> (time (hash c))
"Elapsed time: 12.207651 msecs"
375952610
user=> (time (hash c))
"Elapsed time: 10.995798 msecs"
375952610


 Comments   
Comment by Jozef Wagner [ 09/Mar/14 9:20 AM ]

Added patch which checks if underlying seq implements IHashEq and if yes, uses that hash instead of recomputing.

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

In this patch, can you update the else case (the original code) to use s rather than this, so seq() is not re-called?

Comment by Jozef Wagner [ 04/May/15 12:30 PM ]

Added patch clj-1373-2.diff that reuses s for else case.





[CLJ-1562] some->,some->>,cond->,cond->> and as-> doesn't work with (recur) Created: 11/Oct/14  Updated: 04/May/15

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

Type: Defect Priority: Critical
Reporter: Nahuel Greco Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File fix-CLJ-1418_and_1562.patch    
Patch: Code
Approval: Triaged

 Description   

some-> and his friends doesn't work with recur, because they never place the last expression in tail position. For example:

(loop [l [1 2 3]] 
  (some-> l 
          next 
          recur))

raises UnsupportedOperationException: Can only recur from tail position

This is similar to the bug reported for as-> at http://dev.clojure.org/jira/browse/CLJ-1418 (see the comment at http://dev.clojure.org/jira/browse/CLJ-1418?focusedCommentId=35702&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-35702)

It can be fixed by changing the some-> definition to:

(defmacro some->
  "When expr is not nil, threads it into the first form (via ->),
  and when that result is not nil, through the next etc"
  {:added "1.5"}
  [expr & forms]
  (let [g (gensym)
        pstep (fn [step] `(if (nil? ~g) nil (-> ~g ~step)))]
    `(let [~g ~expr
           ~@(interleave (repeat g) (map pstep (butlast forms)))]
       ~(if forms
          (pstep (last forms))
          g))))

Similar fixes can be done for some->>, cond->, cond->> and as->.

Note -> supports recur without problems, fixing this will homogenize *-> macros behaviour.



 Comments   
Comment by Alex Miller [ 29/Apr/15 11:15 AM ]

Would be great if there was a patch here to consider that covered the set of affected macros.

Comment by Nahuel Greco [ 03/May/15 12:39 PM ]

Attached a patch from https://github.com/nahuel/clojure/compare/fix-CLJ-1418/1562 . This patch also fixes CLJ-1418

Comment by Alex Miller [ 04/May/15 11:17 AM ]

This patch looks like a great start - I will need to look at it further. One thing I just noticed is that none of these macros has any tests. I would love to take this opportunity to rectify that by adding tests for all of them.





[CLJ-1259] Speed up pprint Created: 09/Sep/13  Updated: 04/May/15

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

Type: Enhancement Priority: Major
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: ft, performance, print

Attachments: Text File clj-1259-1.txt    
Patch: Code
Approval: Triaged

 Description   

There are many occurrences of reflection in the pprint implementation.

By eliminating all of them, I ran one benchmark of pprint'ing a Clojure map that resulted in a 300 Kbyte output. After eliminating reflection, the elapsed time to pprint was reduced by 18% (about 14.0 sec down to about 11.5 sec) on a recent model MacBook Pro.

Patch: clj-1259-1.txt
Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 09/Sep/13 11:36 PM ]

Patch clj-1259-1.txt eliminates all occurrences of reflection in pprint, and all files loaded from pprint.clj. It also sets warn-on-reflection to true for those files, in hopes of making it more obvious if a new use of reflection is added there.





[CLJ-1456] The compiler ignores too few or too many arguments to throw Created: 30/Jun/14  Updated: 04/May/15

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

Type: Enhancement Priority: Major
Reporter: Alf Kristian Støyle Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, ft

Attachments: Text File clj-1456-4.patch     Text File v3_0001-CLJ-1456-counting-forms-to-catch-malformed-throw-for.patch    
Patch: Code and Test
Approval: Triaged

 Description   

The compiler does not fail on "malformed" throw forms:

user=> (defn foo [] (throw))
#'user/foo

user=> (foo)
NullPointerException   user/foo (NO_SOURCE_FILE:1)

user=> (defn bar [] (throw Exception baz))
#'user/bar

user=> (bar)
ClassCastException java.lang.Class cannot be cast to java.lang.Throwable  user/bar (NO_SOURCE_FILE:1)

; This one works, but ignored-symbol, should probably not be ignored
user=> (defn quux [] (throw (Exception. "Works!") ignored-symbol))
#'user/quux

user=> (quux)
Exception Works!  user/quux (NO_SOURCE_FILE:1)

The compiler can easily avoid these by counting forms.

Patch: clj-1456-4.patch

Screened by: Alex Miller



 Comments   
Comment by Alf Kristian Støyle [ 30/Jun/14 11:56 AM ]

Not sure how to create a test for the attached patch. Will happily do so if anyone has a suggestion.

Comment by Alex Miller [ 30/Jun/14 12:23 PM ]

Re testing, I think the examples you give are good - you should add tests to test/clojure/test_clojure/compilation.clj that eval the form and expect compilation errors. I'm sure you can find similar examples.

Comment by Alf Kristian Støyle [ 30/Jun/14 2:01 PM ]

Newest patch also contains a few tests.

Comment by Andy Fingerhut [ 29/Aug/14 4:54 PM ]

All patches dated Jun 30 2014 and earlier no longer applied cleanly to latest master after some commits were made to Clojure on Aug 29, 2014. They did apply cleanly before that day.

I have not checked how easy or difficult it might be to update this patch. See section "Updating Stale Patches" on this wiki page for some tips on updating patches: http://dev.clojure.org/display/community/Developing+Patches

Alf, it can help avoid confusion if different patches have different file names. JIRA lets you create multiple attachments with the same name, but I wouldn't recommend it.

Comment by Alf Kristian Støyle [ 30/Aug/14 2:18 AM ]

It was easy to fix the patch. Uploaded the new patch v3_0001-CLJ-1456-counting-forms-to-catch-malformed-throw-for.patch, which applies cleanly to the current master.

Comment by Andy Fingerhut [ 08/Jan/15 6:07 PM ]

Alf, while JIRA can handle multiple attachments for the same ticket with the same name, it can get confusing for people trying to determine which one with the same name is meant. Could you remove or rename one of your identically-named attachments? Instructions for deleting patches are in the "Removing patches" section on this wiki page: http://dev.clojure.org/display/community/Developing+Patches

Comment by Alf Kristian Støyle [ 10/Jan/15 9:12 AM ]

Removed both obsolete attachments. So shouldn't be confusing any more

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

-4 patch is same, just refreshed to apply to master





[CLJ-1485] clojure.test.junit/with-junit-output doesn't handle multiple expressions Created: 29/Jul/14  Updated: 04/May/15

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

Type: Defect Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: clojure.test, ft

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

 Description   

From the docstring description, and the use of ~@body, the intent of with-junit-output was to support a body containing multiple forms (for side-effects). However, calling it with multiple expressions will yield an error about the bindings in the let form.

(defmacro with-junit-output
  "Execute body with modified test-is reporting functions that write
  JUnit-compatible XML output."
  {:added "1.1"}
  [& body]
  `(binding [t/report junit-report
             *var-context* (list)
             *depth* 1]
     (t/with-test-out
       (println "<?xml version=\"1.0\" encoding=\"UTF-8\"?>")
       (println "<testsuites>"))
     (let [result# ~@body]
       (t/with-test-out (println "</testsuites>"))
       result#)))

Cause: The ~@body in the macro is spliced into code expecting a single expression.

Approach: Wrap a (do ) around the ~@body.

Patch: clj-1485.patch

Screened by: Alex Miller



 Comments   
Comment by Howard Lewis Ship [ 29/Jul/14 4:59 PM ]

Patch for issue





[CLJ-1644] into-array fails for sequences starting with nil Created: 15/Jan/15  Updated: 04/May/15

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

Type: Defect Priority: Minor
Reporter: Michael Blume Assignee: Michael Blume
Resolution: Unresolved Votes: 0
Labels: arrays, ft

Attachments: Text File CLJ-1644-array-first-nil-v1.patch     Text File CLJ-1644-array-first-nil-v2.patch    
Patch: Code and Test
Approval: Triaged

 Description   

The into-array doc string implies that into-array will fall back to an Object array if aseq is supplied, but if the first element of aseq is nil, an NPE occurs.

user=> (doc into-array)
-------------------------
clojure.core/into-array
([aseq] [type aseq])
  Returns an array with components set to the values in aseq. The array's
  component type is type if provided, or the type of the first value in
  aseq if present, or Object. All values in aseq must be compatible with
  the component type. Class objects for the primitive types can be obtained
  using, e.g., Integer/TYPE.
user=> (into-array [nil 1 2])
NullPointerException   clojure.lang.RT.seqToTypedArray (RT.java:1691)

Approach: Check for nil and use Object as the array type.

Patch: CLJ-1644-array-first-nil-v2.patch

Screened by: Alex Miller



 Comments   
Comment by Michael Blume [ 15/Jan/15 1:45 PM ]

uploading patch v1 (adding nil as a cons-able element in CLJ-1643 will also bring this out, but I don't want to make the one patch depend on the other)

Comment by Phill Wolf [ 12/Mar/15 7:06 PM ]

Searching through the sequence for a non-null item is not consistent with into-array's docstring. The docstring says, "The array's component type is type if provided, or the type of the first value in aseq if present, or Object." In keeping with the docstring, wouldn't a null first item suggest an array of Object?

Working harder than that (by searching the sequence) only delays the inevitable: a whole sequence of nulls producing an Object array, instead of an array of the type the programmer expected, and triggering a run-time crash.

In summary: this patch goes farther than necessary, but even so, it does not cure the risk of unexpected results from nulls. A simpler remedy – returning an array of Object if the first item is null – would be consistent with the docstring and avoid raising unfounded expectations. Adding a statement that null is of type Object to the docstring could help programmers avoid falling into the trap.

Comment by Alex Miller [ 12/Mar/15 8:29 PM ]

No search through the sequence will pass screening, please just add a nil check.

Comment by Michael Blume [ 13/Mar/15 4:39 PM ]

done





[CLJ-1399] missing field munging when recreating deftypes serialized into byte code Created: 02/Apr/14  Updated: 04/May/15

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

Type: Defect Priority: Critical
Reporter: Kevin Downey Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: compiler, deftype, ft

Attachments: File clj-1399.diff     File clj-1399-with-test.diff    
Patch: Code and Test
Approval: Triaged

 Description   

deftypes with fields whose names get munged fail when constructed in data reader functions.

user=> (deftype Foo [hello-world])
user.Foo
user=> (alter-var-root #'default-data-readers assoc 'foo (fn [x] (->Foo x)))
{inst #'clojure.instant/read-instant-date, uuid #'clojure.uuid/default-uuid-reader, foo #object[user$eval12$fn__13 0x23c89df9 "user$eval12$fn__13@23c89df9"]}
user=> #foo "1"
CompilerException java.lang.IllegalArgumentException: No matching field found: hello-world for class user.Foo, compiling:(NO_SOURCE_PATH:0:0)

Cause: To embed deftypes in the bytecode the compiler emits the value of each field, then emits a call to the deftypes underlying class's constructor. To get a list of fields the compiler calls .getBasis. The getBasis fields are the "clojure" level field names of the deftype, which the actual "jvm" level field names have been munged (replacing - with _, etc), so the compiler tries to generate code to set values on non-existent fields.

Approach: Munge the field name before emitting it in bytecode.
Patch: clj-1399-with-test.diff
Screened by: Alex Miller



 Comments   
Comment by Kevin Downey [ 02/Apr/14 4:26 PM ]

reproducing case

$ rlwrap java -server -Xmx1G -Xms1G -jar /Users/hiredman/src/clojure/target/clojure-1.6.0-master-SNAPSHOT.jar
Clojure 1.6.0-master-SNAPSHOT
user=> (deftype Foo [hello-world])
user.Foo
user=> (alter-var-root #'default-data-readers assoc 'foo (fn [x] (Foo. x)))
{foo #<user$eval6$fn__7 user$eval6$fn__7@2f953efd>, inst #'clojure.instant/read-instant-date, uuid #'clojure.uuid/default-uuid-reader}
user=> #foo "1"
CompilerException java.lang.IllegalArgumentException: No matching field found: hello-world for class user.Foo, compiling:(NO_SOURCE_PATH:0:0)
user=>
Comment by Kevin Downey [ 02/Apr/14 4:39 PM ]

this patch fixes the issue on the latest master for me

Comment by Chas Emerick [ 02/Apr/14 4:57 PM ]

FWIW, this was precipitated by real experience (I think I created the refheap paste). The workaround is easy (don't use dashes in field names of deftypes you want to return from data reader functions), but I wouldn't expect anyone to guess that that wasn't already oversensitized to munging edge cases.

Comment by Alex Miller [ 29/Apr/15 11:36 AM ]

Could the patch have a test?

Comment by Kevin Downey [ 29/Apr/15 1:20 PM ]

clj-1399-with-test.diff adds a test





[CLJ-1528] clojure.test/inc-report-counter is not thread safe Created: 19/Sep/14  Updated: 04/May/15

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

Type: Defect Priority: Critical
Reporter: Alexander Redington Assignee: Alexander Redington
Resolution: Unresolved Votes: 1
Labels: ft, test
Environment:

OS X, Clojure 1.7, Macbook pro


Attachments: File fix-CLJ-1528.diff    
Patch: Code
Approval: Triaged

 Description   

clojure.test/inc-report-counter function combines dereferencing the report-counters ref and operating on the previous state of the ref, leading to race conditions during concurrent use.

(dosync (commute *report-counters* assoc name
                 (inc (or (*report-counters* name) 0))))

Approach: Rewrite update function to be entirely in terms of the old state:

(dosync (commute *report-counters* update-in [name] (fnil inc 0)))

Patch: fix-CLJ-1528.diff
Screened by: Alex Miller



 Comments   
Comment by Alexander Redington [ 19/Sep/14 10:58 AM ]

Fixes 1528





[CLJ-1533] Oddity in type tag usage for primInvoke Created: 24/Sep/14  Updated: 04/May/15

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

Type: Defect Priority: Critical
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: ft, typehints

Attachments: Text File 0001-CLJ-1533-inject-original-var-form-meta-in-constructe.patch     Text File clj-1533-2.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Some odd behavior demonstrated in Clojure 1.6.0 REPL below. Why does the (Math/abs (f2 -3)) call issue a reflection warning? It seems like perhaps it should not, given the other examples.

user=> (clojure-version)
"1.6.0"
user=> (set! *warn-on-reflection* true)
true
user=> (defn ^{:tag 'long} f1 [x] (inc x))
#'user/f1
user=> (Math/abs (f1 -3))
2
user=> (defn ^{:tag 'long} f2 [^long x] (inc x))
#'user/f2
user=> (Math/abs (f2 -3))
Reflection warning, NO_SOURCE_PATH:6:1 - call to static method abs on java.lang.Math can't be resolved (argument types: java.lang.Object).
2
user=> (defn ^{:tag 'long} f3 ^long [^long x] (inc x))
#'user/f3
user=> (Math/abs (f3 -3))
2

Cause: invokePrim path does not take into account var or form meta

Approach: apply var and form meta to invokePrim expression

Patch: clj-1533-2.patch

Screened by: Alex Miller



 Comments   
Comment by Nicola Mometto [ 25/Sep/14 9:47 AM ]

The issue is similar to http://dev.clojure.org/jira/browse/CLJ-1491

Comment by Nicola Mometto [ 25/Sep/14 9:58 AM ]

The root cause was also almost the same, the proposed patch is a superset of the one proposed for CLJ-1491

Comment by Alex Miller [ 25/Sep/14 10:09 AM ]

Can we include 1491 cases in this ticket and mark 1491 a duplicate?

Comment by Alex Miller [ 25/Sep/14 10:09 AM ]

Also needs tests in the patch.

Comment by Nicola Mometto [ 25/Sep/14 10:23 AM ]

Updated the patch with testcases for both issues, I agree that CLJ-1491 should be closed as duplicate

Comment by Alex Miller [ 04/May/15 8:52 AM ]

New patch is identical, just refreshed to apply to master





[CLJ-1242] get/= on sorted collections when types don't match result in a ClassCastException Created: 31/Jul/13  Updated: 03/May/15

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

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

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

 Description   

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

but

user=> (= (sorted-set 1) :a)
false

also

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



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

PersistentVector also has the same problem.

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

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

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

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

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

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





[CLJ-1418] make as-> macro compatible with destructuring Created: 09/May/14  Updated: 03/May/15  Resolved: 03/May/15

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

Type: Enhancement Priority: Critical
Reporter: Nahuel Greco Assignee: Unassigned
Resolution: Duplicate Votes: 10
Labels: None
Environment:

all environments


Patch: Code
Approval: Triaged

 Description   

The as-> macro doesn't work with destructuring. This is invalid code:

(-> [1 2] 
    (as-> [a & b] 
          [a (inc b)] 
          [(inc a) b]))

because it is expanded to:

(let [[a & b] [1 2]
        [a & b] [a (inc b)]
        [a & b] [(inc a) b]]
       [a & b])  ;; this last expression will not compile

but with a little redefinition is possible to make as-> work with
destructuring:

(defmacro as->
  "Binds name to expr, evaluates the first form in the lexical context
  of that binding, then binds name to that result, repeating for each
  successive form, returning the result of the last form."
  {:added "1.5"}
  [expr name & forms]
  `(let [~name ~expr
         ~@(interleave (repeat name) (butlast forms))]
     ~(last forms)))

now the previous example will expand to:

(let [[a & b] [1 2]
      [a & b] [a (inc b)]]
     [(inc a) b])

The following example shows why an as-> destructuring compatible
macro can be useful. This code parses a defmulti like parameter list
by reusing a destructuring form:

(defmacro defmulti2 [mm-name & opts]
 (-> [{} opts]
      (as-> [m [e & r :as o]] 
            (if (string? e) 
              [(assoc m :docstring e) r] 
              [m                      o])
            (if (map? e)
              [(assoc m :attr-map e :dispatch-fn (first r)) (next r)]
              [(assoc m             :dispatch-fn e)         r])
            ...

Compare with the original defmulti:

(defmacro defmulti [mm-name & options]
  (let [docstring   (if (string? (first options))
                      (first options)
                      nil)
        options     (if (string? (first options))
                      (next options)
                      options)
        m           (if (map? (first options))
                      (first options)
                      {})
        options     (if (map? (first options))
                      (next options)
                      options)
        dispatch-fn (first options)
        options     (next options)
        m           (if docstring
                      (assoc m :doc docstring)
                      m)
        ...


 Comments   
Comment by Nahuel Greco [ 09/May/14 2:12 AM ]

note, this issue is badly formated, for a more legible form:

https://gist.github.com/nahuel/a34a9fe967c035a3d069

Comment by Nahuel Greco [ 13/Sep/14 6:15 AM ]

Related: you cannot use recur as the last expression of as->, because the macroexpansion will not place it at tail position. The fix proposed above also fixes that, so you can use something like:

(loop []
  (as-> [] x
        ;;  manipulate x
        (when (empty? x) (recur)))))
Comment by Michael Blume [ 17/Oct/14 1:14 PM ]

I don't actually understand what the &s are doing in the example code? In the first step of the first example it looks like you're binding b to the list (2), and then trying to increment that, which fails

user=> (let [[a & b] [1 2]
  #_=>       [a & b] [a (inc b)]]
  #_=>      [(inc a) b])

ClassCastException clojure.lang.PersistentVector$ChunkedSeq cannot be cast to java.lang.Number  clojure.lang.Numbers.inc (Numbers.java:110)
user=> (let [[a b] [1 2]
  #_=>       [a b] [a (inc b)]]
  #_=>      [(inc a) b])
[2 3]
Comment by Nahuel Greco [ 17/Oct/14 2:16 PM ]

Michael Blume: Sorry, example is wrong, replace [a & b] with [a & [b]]:

(-> [1 2] 
    (as-> [a & [b]] 
          [a (inc b)] 
          [(inc a) b]))

;=> expands to: 

(let [[a & [b]] [1 2] 
      [a & [b]] [a (inc b)] 
      [a & [b]] [(inc a) b]] 
    [a & [b]]) ;; this last expression will not compile

;=> expansion using redefined as-> follows:

(let [[a & [b]] [1 2] 
      [a & [b]] [a (inc b)]] 
    [(inc a) b])  ;; now ok
Comment by Nahuel Greco [ 03/May/15 12:41 PM ]

I attached a patch in CLJ-1562 fixing also this issue.

Comment by Alex Miller [ 03/May/15 7:50 PM ]

Dupe to CLJ-1562





[CLJ-1298] Add more type predicate fns to core Created: 21/Nov/13  Updated: 03/May/15

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

Type: Enhancement Priority: Major
Reporter: Alex Fowler Assignee: Unassigned
Resolution: Unresolved Votes: 15
Labels: None

Approval: Triaged

 Description   

Add more built-in type predicates:

1) Definitely missing: (atom? x), (ref? x), (deref? x), (named? x), (map-entry? x), (lazy-seq? x).
2) Very good to have: (throwable? x), (exception? x), (pattern? x).

The first group is especially important for writing cleaner code with core Clojure.



 Comments   
Comment by Alex Miller [ 21/Nov/13 8:42 AM ]

In general many of the existing predicates map to interfaces. I'm guessing these would map to checks on the following types:

atom? = Atom (final class)
ref? = IRef (interface)
deref? = IDeref (interface)
named? = Named (interface, despite no I prefix)
map-entry? = IMapEntry (interface)
lazy-seq? = LazySeq (final class)

throwable? = Throwable
exception? = Exception, but this seems less useful as it feels like the right answer when you likely actually want throwable?
pattern? = java.util.regex.Pattern

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

Yes, they do, and sometimes the code has many checks like (instance? clojure.lang.Atom x). Ok, you can write a little function (atom? x) but it has either to be written in all relevant namespaces or required/referred there from some extra namespace. All this is just a burden. For example, we have predicates like (var? x) or (future? x) which too map to Java classes, but having them abbreviated often makes possible to write a cleaner code.

I feel the first group to be especially significant for it being about core Clojure concepts like atom and ref. Having to fall to manual Java classes check to work with them feels inorganic. Of course we can, but why then do we have (var? x), (fn? x) and other? Imagine, for example:

(cond
(var? x) (...)
(fn? x) (...)
(instance? clojure.lang.Atom x) (...)
(or (instance? clojure.lang.Named x) (instance? clojure.lang.LazySeq x)) (...))

vs

(cond
(var? x) (...)
(fn? x) (...)
(atom? x) (...)
(or (named? x) (lazy-seq? x)) (...))

The second group is too, essential since these concepts are fundamental for the platform (but you're right with the (exception? x) one).

Comment by Alex Fowler [ 22/Nov/13 6:35 AM ]

Also, obviously I missed the (boolean? x) predicate in the original post. Did not even guess it is absent too until I occasionally got into it today. Currently the most clean way we have is to do (or (true? x) (false? x)). Needles to say, it looks weird next to the present (integer? x) or (float? x).

Comment by Brandon Bloom [ 22/Jul/14 1:02 AM ]

Predicates for core types are also very useful for portability to CLJS.

Comment by Brandon Bloom [ 22/Jul/14 1:05 AM ]

I'd be happy to provide a patch for this, but I'd prefer universal interface support where possible. Therefore, this ticket, in my mind, is behind http://dev.clojure.org/jira/browse/CLJ-803 etc.

Comment by Alex Miller [ 22/Jul/14 6:12 AM ]

I don't think it's worth making a ticket for this until Rich has looked at it and determined which parts are wanted.

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

Someone asked about a boolean? predicate, so throwing this one on the list...

Comment by Reid McKenzie [ 02/Dec/14 4:51 PM ]

uuid? maybe. UUIDs have a bit of a strange position in that we have special printer handling for them built into core implying that they are intentionally part of Clojure, but there is no ->UUID constructor and no functions in core that operate on them so I could see this one being specifically declined.

Comment by Brandon Bloom [ 03/May/15 2:50 PM ]

This has been troubling me again with my first cljc project. So, I've added a whole bunch of tickets (with patches!) for individual predicates in both CLJ and CLJS.

Comment by Alex Miller [ 03/May/15 5:35 PM ]

As I said above, I don't want to mess with specific patches or tickets on this until Rich gets a look at this and we decide which stuff should and should not be included. So I'm going to ignore your other tickets for now...





[CLJ-1720] Add clojure.core/pattern? predicate Created: 03/May/15  Updated: 03/May/15

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

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: patch

Attachments: Text File CLJ-1720_v01.patch     Text File CLJ-1720_v02.patch    
Patch: Code and Test

 Description   

Just like http://dev.clojure.org/jira/browse/CLJ-1719 , this helps with clj/cljs compatibility.



 Comments   
Comment by Brandon Bloom [ 03/May/15 2:37 PM ]

See also http://dev.clojure.org/jira/browse/CLJS-1242

Comment by Brandon Bloom [ 03/May/15 2:48 PM ]

Whoops, uploaded wrong patch. Tests actually pass in this v02 patch.





[CLJ-1721] Enable test case for char? Created: 03/May/15  Updated: 03/May/15

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

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: patch

Attachments: Text File CLJ-1721_v01.patch    
Patch: Code and Test

 Description   

clojure.core/char? already exists, but there was no test for it (despite a comment suggesting one).






[CLJ-1719] Add clojure.core/boolean? predicate Created: 03/May/15  Updated: 03/May/15

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

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: patch

Attachments: Text File CLJ-1719_v01.patch    
Patch: Code and Test

 Description   

Having this predicate aids with clj/cljs compatibility.



 Comments   
Comment by Brandon Bloom [ 03/May/15 2:32 PM ]

See also: http://dev.clojure.org/jira/browse/CLJS-1241





[CLJ-99] GC Issue 95: max-key and min-key evaluate k multiple times for arguments Created: 17/Jun/09  Updated: 01/May/15

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

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut