<< Back to previous view

[CLJ-1517] Unrolled small vectors Created: 01/Sep/14  Updated: 31/Jul/15

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

Type: Enhancement Priority: Critical
Reporter: Zach Tellman Assignee: Unassigned
Resolution: Unresolved Votes: 20
Labels: collections, performance

Attachments: File unrolled-collections-2.diff     File unrolled-collections.diff     Text File unrolled-vector-2.patch     Text File unrolled-vector.patch    
Patch: Code
Approval: Incomplete

 Description   

As discussed on the mailing list [1], this patch has two unrolled variants of vectors and maps, with special inner classes for each cardinality. Currently both grow to six elements before spilling over into the general versions of the data structures, which is based on rough testing but can be easily changed. At Rich's request, I haven't included any integration into the rest of the code, and there are top-level static create() methods for each.

The sole reason for this patch is performance, both in terms of creating data structures and performing operations on them. This can be seen as a more verbose version of the trick currently played with PersistentArrayMap spilling over into PersistentHashMap. Based on the benchmarks, which can be run by cloning cambrian-collections [2] and running 'lein test :benchmark', this should supplant PersistentArrayMap. Performance is at least on par with PAM, and often much faster. Especially noteworthy is the creation time, which is 5x faster for maps of all sizes (lein test :only cambrian-collections.map-test/benchmark-construction), and on par for 3-vectors, but 20x faster for 5-vectors. There are similar benefits for hash and equality calculations, as well as calls to reduce().

This is a big patch (over 5k lines), and will be kind of a pain to review. My assumption of correctness is based on the use of collection-check, and the fact that the underlying approach is very simple. I'm happy to provide a high-level description of the approach taken, though, if that will help the review process.

I'm hoping to get this into 1.7, so please let me know if there's anything I can do to help accomplish that.

[1] https://groups.google.com/forum/#!topic/clojure-dev/pDhYoELjrcs
[2] https://github.com/ztellman/cambrian-collections

Patch: unrolled-vector-2.patch

Screener Notes: The approach is clear and understandable. Given the volume of generated code, I believe that best way to improve confidence in this code is to get people using it asap, and add collection-test [3] to the Clojure test suite. I would also like to get the generator [4] included in the Clojure repo. We don't need to necessarily automate running it, but would be nice to have it nearby if we want to tweak something later.

[3] https://github.com/ztellman/collection-check/blob/master/src/collection_check.clj
[4] https://github.com/ztellman/cambrian-collections/blob/master/generate/cambrian_collections/vector.clj



 Comments   
Comment by Zach Tellman [ 01/Sep/14 10:13 PM ]

Oh, I forgot to mention that I didn't make a PersistentUnrolledSet, since the existing wrappers can use the unrolled map implementation. However, it would be moderately faster and more memory efficient to have one, so let me know if it seems worthwhile.

Comment by Nicola Mometto [ 02/Sep/14 5:23 AM ]

Zach, the patch you added isn't in the correct format, they need to be created using `git format-patch`

Comment by Nicola Mometto [ 02/Sep/14 5:31 AM ]

Also, I'm not sure if this is on-scope with the ticket but those patches break with *print-dup*, as it expects a static create(x) method for each inner class.

I'd suggest adding a create(Map x) static method for the inner PersistentUnrolledMap classes and a create(ISeq x) one for the inner PersistentUnrolledVector classes

Comment by Alex Miller [ 02/Sep/14 8:14 AM ]

Re making patches, see: http://dev.clojure.org/display/community/Developing+Patches

Comment by Jozef Wagner [ 02/Sep/14 9:16 AM ]

I wonder what is the overhead of having meta and 2 hash fields in the class. Have you considered a version where the hash is computed on the fly and where you have two sets of collections, one with meta field and one without, using former when the actual metadata is attached to the collection?

Comment by Zach Tellman [ 02/Sep/14 12:13 PM ]

I've attached a patch using the proper method. Somehow I missed the detailed explanation for how to do this, sorry. I know the guidelines say not to delete previous patches, but since the first one isn't useful I've deleted it to minimize confusion.

I did the print-dup friendly create methods, and then realized that once these are properly integrated, 'pr' will just emit these as vectors. I'm fairly sure the create methods aren't necessary, so I've commented them out, but I'm happy to add them back in if they're useful for some reason I can't see.

I haven't given a lot of thought to memory efficiency, but I think caching the hashes are worthwhile. I can see an argument for creating a "with-meta" version of each collection, but since that would double the size of an already enormous patch, I think that should probably wait.

Comment by Zach Tellman [ 03/Sep/14 4:31 PM ]

I found a bug! Like PersistentArrayMap, I have a special code path for comparing keywords, but my generators for collection-check were previously using only integer keys. There was an off-by-one error in the transient map implementation [1], which was not present for non-keyword lookups.

I've taken a close look for other gaps in my test coverage, and can't find any. I don't think this substantively changes the risk of this patch (an updated version of which has been uploaded as 'unrolled-collections-2.diff'), but obviously where there's one bug, there may be others.

[1] https://github.com/ztellman/cambrian-collections/commit/eb7dfe6d12e6774512dbab22a148202052442c6d#diff-4bf78dbf5b453f84ed59795a3bffe5fcR559

Comment by Zach Tellman [ 03/Oct/14 2:34 PM ]

As an additional data point, I swapped out the data structures in the Cheshire JSON library. On the "no keyword-fn decode" benchmark, the current implementation takes 6us, with the unrolled data structures takes 4us, and with no data structures (just lexing the JSON via Jackson) takes 2us. Other benchmarks had similar results. So at least in this scenario, it halves the overhead.

Benchmarks can be run by cloning https://github.com/dakrone/cheshire, unrolled collections can be tested by using the 'unrolled-collections' branch. The pure lexing benchmark can be reproduced by messing around with the cheshire.parse namespace a bit.

Comment by Zach Tellman [ 06/Oct/14 1:31 PM ]

Is there no way to get this into 1.7? It's an awfully big win to push off for another year.

Comment by Alex Miller [ 07/Oct/14 2:08 PM ]

Hey Zach, it's definitely considered important but we have decided to drop almost everything not fully done for 1.7. Timeframe for following release is unknown, but certainly expected to be significantly less than a year.

Comment by John Szakmeister [ 30/Oct/14 2:53 PM ]

You are all free to determine the time table, but I thought I'd point out that Zach is not entirely off-base. Clojure 1.4.0 was released April 5th, 2012. Clojure 1.5.0 was released March 1st, 2013 with 1.6.0 showing up March 25th, 2014. So it appears that the current cadence is around a year.

Comment by Alex Miller [ 30/Oct/14 3:40 PM ]

John, there is no point to comments like this. Let's please keep issue comments focused on the issue.

Comment by Zach Tellman [ 13/Nov/14 12:23 PM ]

I did a small write-up on this patch which should help in the eventual code review: http://blog.factual.com/using-clojure-to-generate-java-to-reimplement-clojure

Comment by Zach Tellman [ 07/Dec/14 10:34 PM ]

Per my conversation with Alex at the Conj, here's a patch that only contains the unrolled vectors, and uses the more efficient constructor for PersistentVector when spilling over.

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

Zach, I created a new placeholder for the map work at http://dev.clojure.org/jira/browse/CLJ-1610.

Comment by Jean Niklas L'orange [ 09/Dec/14 1:52 PM ]

It should probably be noted that core.rrb-vector will break for small vectors by this patch, as it peeks into the underlying structure. This will also break other libraries which peeks into the vector implementation internals, although I'm not aware of any other – certainly not any other contrib library.

Also, two comments on unrolled-vector.patch:

private transient boolean edit = true;
in the Transient class should probably be
private volatile boolean edit = true;
as transient means something entirely different in Java.

conj in the Transient implementation could invalidate itself without any problems (edit = false;) if it is converted into a TransientVector (i.e. spills over) – unless it has a notable overhead. The invalidation can prevent some subtle bugs related to erroneous transient usage.

Comment by Alex Miller [ 09/Dec/14 1:58 PM ]

Jean - understanding the scope of the impact will certainly be part of the integration process for this patch. I appreciate the heads-up. While we try to minimize breakage for things like this, it may be unavoidable for libraries that rely on implementation internals.

Comment by Michał Marczyk [ 09/Dec/14 2:03 PM ]

I'll add support for unrolled vectors to core.rrb-vector the moment they land on master. (Probably with some conditional compilation so as not to break compatibility with earlier versions of Clojure – we'll see when the time comes.)

Comment by Michał Marczyk [ 09/Dec/14 2:06 PM ]

I should say that it'd be possible to add generic support for any "vector lookalikes" by pouring them into regular vectors in linear time. At first glance it seems to me that that'd be out of line with the basic promise of the library, but I'll give it some more thought before the changes actually land.

Comment by Zach Tellman [ 09/Dec/14 5:43 PM ]

Somewhat predictably, the day after I cut the previous patch, someone found an issue [1]. In short, my use of the ArrayChunk wrapper applied the offset twice.

This was not caught by collection-check, which has been updated to catch this particular failure. It was, however, uncovered by Michael Blume's attempts to merge the change into Clojure, which tripped a bunch of alarms in Clojure's test suite. My own attempt to do the same to "prove" that it worked was before I added in the chunked seq functionality, hence this issue persisting until now.

As always, there may be more issues lurking. I hope we can get as many eyeballs on the code between now and 1.8 as possible.

[1] https://github.com/ztellman/cambrian-collections/commit/2e70bbd14640b312db77590d8224e6ed0f535b43
[2] https://github.com/MichaelBlume/clojure/tree/test-vector

Comment by Zach Tellman [ 10/Jul/15 1:54 PM ]

As a companion to the performance analysis in the unrolled map issue, I've run the benchmarks and posted the results at https://gist.github.com/ztellman/10e8959501fb666dc35e. Some notable results:

Comment by Alex Miller [ 13/Jul/15 9:02 AM ]

Stu: I do not think this patch should be marked "screened" until the actual integration and build work (if the generator is integrated) has been completed.

Comment by Alex Miller [ 14/Jul/15 4:33 PM ]

FYI, we have "reset" all big features for 1.8 for the moment (except the socket repl work). We may still include it - that determination will be made later.

Comment by Zach Tellman [ 14/Jul/15 4:43 PM ]

Okay, any idea when the determination will be made? I was excited that we seemed to be finally moving forward on this.

Comment by Alex Miller [ 14/Jul/15 4:51 PM ]

No, but it is now on my work list.

Comment by Rich Hickey [ 15/Jul/15 8:17 AM ]

I wonder if all of the overriding of APersistentVector yields important benefits - e.g. iterator, hashcode etc.

Comment by Zach Tellman [ 15/Jul/15 11:51 AM ]

In the case of hashcode, definitely: https://gist.github.com/ztellman/10e8959501fb666dc35e#file-gistfile1-txt-L1013-L1076. This was actually one of the original things I wanted to speed up.

In the case of the iterator, probably not. I'd be fine removing that.

Comment by Zach Tellman [ 16/Jul/15 5:17 PM ]

So am I to infer from https://github.com/clojure/clojure/commit/36d665793b43f62cfd22354aced4c6892088abd6 that this issue is defunct? If so, I think there's a lot of improvements being left on the table for no particular reason.

Comment by Rich Hickey [ 16/Jul/15 6:34 PM ]

Yes, that commit covers this functionality. It takes a different approach from the patch in building up from a small core, and maximizing improvements to the bases rather than having a lot of redundant definitions per class. That also allowed for immediate integration without as much concern for correctness, as there is little new code. It also emphasizes the use case for tuples, e.g. small vectors used as values that won't be changed, thus de-emphasizing the 'mutable' functions. I disagree that many necessary improvements are being left out. The patch 'optimized' many things that don't matter. Further, there are not big improvements to the pervasive inlining. In addition, the commit includes the integration work at a fraction of the size of the patch. In all, it would have taken much more back and forth to get the patch to conform with this approach than to just get it all done, but I appreciate the inspiration and instigation - thanks!

Comment by Rich Hickey [ 16/Jul/15 6:46 PM ]

That said, this commit need not be the last word - it can serve as a baseline for further optimization. But I'd rather that be driven by need. Clojure could become 10x as large optimizing things that don't matter.

Comment by Zach Tellman [ 19/Jul/15 1:36 PM ]

What is our reference for "relevant" performance? I (or anyone else) can provide microbenchmarks for calculating hashes or whatever else, but that doesn't prove that it's an important improvement. I previously provided benchmarks for JSON decoding in Cheshire, but that's just one of many possible real-world benchmarks. It might be useful to have an agreed-upon list of benchmarks that we can use when debating what is and isn't useful.

Comment by Mike Anderson [ 19/Jul/15 11:14 PM ]

I was interested in this implementation so created a branch that integrates Zach's unrolled vectors on top of clojure 1.8.0-alpha2. I also simplified some of the code (I don't think the metadata handling or unrolled seqs are worthwhile, for example)

Github branch: https://github.com/mikera/clojure/tree/clj-1517

Then I ran a set of micro-benchmarks created by Peter Taoussanis

Results: https://gist.github.com/mikera/72a739c84dd52fa3b6d6

My findings from this testing:

  • Performance is comparable (within +/- 20%) on the majority of tests
  • Zach's approach is noticeably faster (by 70-150%) for 4 operations (reduce, mapv, into, equality)

My view is that these additional optimisations are worthwhile. In particular, I think that reduce and into are very important operations. I also care about mapv quite a lot for core.matrix (It's fundamental to many numerical operations on arrays implemented using Clojure vectors).

Happy to create a patch along these lines if it would be acceptable.

Comment by Zach Tellman [ 19/Jul/15 11:45 PM ]

The `reduce` improvements are likely due to the unrolled reduce and kvreduce impls, but the others are probably because of the unrolled transient implementation. The extra code required to add these would be pretty modest.

Comment by Mike Anderson [ 20/Jul/15 9:20 PM ]

I actually condensed the code down to a single implementation for `Transient` and `TupleSeq`. I don't think these really need to be fully unrolled for each Tuple type. That helps by making the code even smaller (and probably also helps performance, given JVM inline caching etc.)

Comment by Peter Taoussanis [ 21/Jul/15 11:46 AM ]

Hey folks,

Silly question: is there actually a particular set of reference benchmarks that everyone's currently using to test the work on tuples? It took me a while to notice how bad the variance was with my own set of micro benchmarks.

Bumping all the run counts up till the noise starts ~dying down, I'm actually seeing numbers now that don't seem to agree with others here .

Google Docs link: https://docs.google.com/spreadsheets/d/1QHY3lehVF-aKrlOwDQfyDO5SLkGeb_uaj85NZ7tnuL0/edit?usp=sharing
gist with the benchmarks: https://gist.github.com/ptaoussanis/0a294809bc9075b6b02d

Thanks, cheers!

Comment by Zach Tellman [ 21/Jul/15 6:52 PM ]

Hey Peter, I can't reproduce your results, and some of them are so far off what I'd expect that I have to think there was some data gathering error. For instance, the assoc operation being slower is kind of inexplicable, considering the unrolled version doesn't do any copying, etc. Also, all of your numbers are significantly slower than the ones on my 4 year old laptop, which is also a bit strange.

Just to make sure that we're comparing apples to apples, I've adapted your benchmarks into something that pretty-prints the mean runtime and variance for 1.7, 1.8-alpha2, and Mike's 1517 fork. It can be found at https://github.com/ztellman/tuple-benchmark, and the results of a run at https://gist.github.com/ztellman/3701d965228fb9eda084.

Comment by Mike Anderson [ 22/Jul/15 2:24 AM ]

Hey Zach just looked at your benchmarks and they are definitely more consistent with what I am seeing. The overall nanosecond timings look about right from my experience with similar code (e.g. working with small vectors in vectorz-clj).

Comment by Peter Taoussanis [ 22/Jul/15 2:41 AM ]

Hi Zach, thanks for that!

Have updated the results -
Gist: https://gist.github.com/ptaoussanis/0a294809bc9075b6b02d
Google docs: https://goo.gl/khgT83

Note that I've added an extra sheet/tab to the Google doc for your own numbers at https://gist.github.com/ztellman/3701d965228fb9eda084.

Am still struggling to produce results that show any consistent+significant post-JIT benefit to either of the tuple implementations against the micro benchmarks and one larger small-vec-heavy system I had handy.

It's looking to me like it's maybe possible that the JIT's actually optimising away most of the non-tuple inefficiencies in practice?

Of course it's very possible that my results are off, or my expectations wrong. The numbers have been difficult to pin down.

It definitely helped to have a standardised reference micro benchmark to work against (https://github.com/ztellman/tuple-benchmark). Could I perhaps suggest a similar reference macro benchmark (maybe something from core.matrix, Mike?)

Might also be a good idea to define a worthwhile target performance delta for ops like these that run in the nanosecond range (or for the larger reference macro benchmark)?

Just some thoughts from someone passing through in case they're at all useful; know many of you have been deeply involved in this for some time so please feel free to ignore any input that's not helpful

Appreciate all the efforts, cheers!

Comment by Rich Hickey [ 22/Jul/15 9:24 AM ]

I think everyone should back off on their enthusiasm for this approach. After much analysis, I am seeing definite negative impacts to tuples, especially the multiple class approach proposed by Zach. What happens in real code is that the many tuple classes cause call sites that see different sized vectors to become megamorphic, and nothing gets adequately optimized. In particular, call sites that will see tuple-sized and large vectors (i.e. a lot of library code) will optimize differently depending upon which they see often first. So, if you run your first tight loop on vector code that sees tuples, that code could later do much worse (50-100%) on large vectors than before the tuples patch was in place. Being much slower on large collections is a poor tradeoff for being slightly faster on small ones.

Certainly different tuple classes for arity 0-6 is a dead idea. You get as good or better optimization (at some cost in size) from a single class e.g. one with four fields, covering sizes 0-4. I have a working version of this in a local branch. It is better in that sites that include pvectors are only bi-morphic, but I am still somewhat skittish given what I've seen.

The other takeaway is that the micro benchmarks are nearly worthless for detecting these issues.

Comment by Zach Tellman [ 22/Jul/15 11:07 AM ]

I'm pretty sure that all of my real-world applications of the tuples (via clj-tuple) have been fixed cardinality, and wouldn't have surfaced any such issue. Thanks for putting the idea through its paces.

Comment by Mike Anderson [ 22/Jul/15 10:37 PM ]

Rich these are good insights - do you have a benchmark that you are using as representative of real world code?

I agree that it is great if we can avoid call sites becoming megamorphic, though I also believe the ship has sailed on that one already when you consider the multiple types of IPersistentVector that already exist (MapEntry, PersistentVector, SubVector plus any library-defined IPersistentVector instances such as clojure.core.rrb-vector). As a consequence, the JVM is usually not going to be able to prove that a specific IPersistentVector interface invocation is monomorphic, which is when the really big optimisations happen.

In most of the real world code that I've been working with, the same size/type of vector gets used repeatedly (Examples: iterating over map entries, working with a sequence of size-N vectors), so in such cases we should be able to rely on the polymorphic inline cache doing the right thing.

The idea of a single Tuple class for sizes 0-4 is interesting, though I can't help thinking that a lot of the performance gain from this may stem from the fact that a lot of code does stuff like (reduce conj [] .....) or the transient equivalent which is a particularly bad use case for Tuples, at least from the call site caching perspective. There may be a better way to optimise such cases rather than simply trying to make Tuples faster.... e.g. calling asTransient() on a Tuple0 could perhaps switch straight into the PersistentVector implementation.





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

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

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

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

 Description   

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

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

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

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

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



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

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

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

Patch won't apply to master for me

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

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

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

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

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

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

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

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

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

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

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

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

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

Hi Adam,

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





[CLJ-1403] ns-resolve might throw ClassNotFoundException but should return nil Created: 14/Apr/14  Updated: 31/Jul/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: 1
Labels: None

Attachments: Text File 0001-CLJ-1403-ns-resolve-returns-nil-if-class-is-not-foun.patch    
Patch: Code and Test
Approval: Triaged

 Description   

The doc of ns-resolve states that in case the symbol cannot be resolved, it should return nil.

user=> (doc ns-resolve)
-------------------------
clojure.core/ns-resolve
([ns sym] [ns env sym])
  Returns the var or Class to which a symbol will be resolved in the
  namespace (unless found in the environment), else nil.  Note that
  if the symbol is fully qualified, the var/Class to which it resolves
  need not be present in the namespace.
nil

However if the symbol contains dots and is not a resolvable Class, a ClassNotFoundException is thrown

user=> (ns-resolve *ns* 'foo.bar)
ClassNotFoundException foo.bar  java.net.URLClassLoader$1.run (URLClassLoader.java:372)
user=> (pst *e)
ClassNotFoundException foo.bar
	java.net.URLClassLoader$1.run (URLClassLoader.java:372)
	java.net.URLClassLoader$1.run (URLClassLoader.java:361)
	java.security.AccessController.doPrivileged (AccessController.java:-2)
	java.net.URLClassLoader.findClass (URLClassLoader.java:360)
	clojure.lang.DynamicClassLoader.findClass (DynamicClassLoader.java:61)
	java.lang.ClassLoader.loadClass (ClassLoader.java:424)
	java.lang.ClassLoader.loadClass (ClassLoader.java:357)
	java.lang.Class.forName0 (Class.java:-2)
	java.lang.Class.forName (Class.java:340)
	clojure.lang.RT.classForName (RT.java:2065)
	clojure.lang.Compiler.maybeResolveIn (Compiler.java:6963)
	clojure.core/ns-resolve (core.clj:4026)
nil

The attached patch makes ns-resolve return nil in that case instead of throwing an exception



 Comments   
Comment by Alex Miller [ 14/Apr/14 2:07 PM ]

Can you include the (pst *e) ?

Comment by Nicola Mometto [ 14/Apr/14 2:10 PM ]

Added result of (pst *e) in the description

Comment by Andy Fingerhut [ 02/Oct/14 11:36 AM ]

Nicola, the patch 0001-CLJ-1403-ns-resolve-returns-nil-if-class-is-not-foun.patch dated 31 Aug 2014 applies cleanly to latest Clojure master as of Oct 1 2014, but fails to compile with JDK8. I haven't checked whether it compiles cleanly with other JDK versions yet.

Comment by Nicola Mometto [ 02/Oct/14 11:48 AM ]

Updated the patch so that it compiles fine on JDK8





[CLJ-1401] CompilerException / IllegalStateException when overriding vars Created: 10/Apr/14  Updated: 31/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, errormsgs

Approval: Triaged

 Description   
=> (ns foo)
nil
=> (def a 1)
#'foo/a
=> (ns bar (:require [foo :refer :all]))
nil
=> (def a 2)
CompilerException java.lang.IllegalStateException: a already refers to: #'foo/a in namespace: bar, compiling:(NO_SOURCE_PATH:4:1)
	clojure.lang.Compiler.analyzeSeq (Compiler.java:6745)
	clojure.lang.Compiler.analyze (Compiler.java:6529)
	clojure.lang.Compiler.analyze (Compiler.java:6490)
	clojure.lang.Compiler.eval (Compiler.java:6801)
	clojure.lang.Compiler.eval (Compiler.java:6760)
	clojure.core/eval (core.clj:3079)
	clojure.main/repl/read-eval-print--7095/fn--7098 (main.clj:240)
	clojure.main/repl/read-eval-print--7095 (main.clj:240)
	clojure.main/repl/fn--7104 (main.clj:258)
	clojure.main/repl (main.clj:258)
	clojure.main/repl-opt (main.clj:324)
	clojure.main/main (main.clj:422)
Caused by:
IllegalStateException a already refers to: #'foo/a in namespace: bar
	clojure.lang.Namespace.warnOrFailOnReplace (Namespace.java:88)
	clojure.lang.Namespace.intern (Namespace.java:72)
	clojure.lang.Compiler$DefExpr$Parser.parse (Compiler.java:534)
	clojure.lang.Compiler.analyzeSeq (Compiler.java:6738)
	clojure.lang.Compiler.analyze (Compiler.java:6529)

I would expect (at worst) a similar warning to the initial namespace loading, rather than an exception here.



 Comments   
Comment by Alex Miller [ 11/Apr/14 8:26 AM ]

Could you put together a better reproducible test case for this that does not depend on core.matrix? Also, please include the (pst *e) when it occurs.

Comment by Andy Fingerhut [ 11/Apr/14 10:19 AM ]

I have tried the smallest possible Leiningen project I could think of that would cause the warnings about redefinitions, to see if I could get the exception to occur. 'lein new try1' to create the skeleton project, then edit src/try1/core.clj to contain only the following function definitions:

(defn merge
  "This definition of merge replaces clojure.core/merge"
  [x y]
  (- x y))

(defn *
  [x y]
  (* x y))

Then start a REPL with 'lein repl', and I see this behavior:

user=> (require '[try1.core :as c])
WARNING: merge already refers to: #'clojure.core/merge in namespace: try1.core, being replaced by: #'try1.core/merge
WARNING: * already refers to: #'clojure.core/* in namespace: try1.core, being replaced by: #'try1.core/*
nil
user=> (require '[try1.core :as c] )
nil
user=> (require '[try1.core :as c] :reload)
WARNING: merge already refers to: #'clojure.core/merge in namespace: try1.core, being replaced by: #'try1.core/merge
WARNING: * already refers to: #'clojure.core/* in namespace: try1.core, being replaced by: #'try1.core/*
nil

Ths all looks like behavior as I would expect, and I did not see the exception that Mike reports.

It seems that either Ctrl+Alt+L in Counterclockwise does something different than (require ... :reload), or there is something different about Mike's namespace in addition to redefining names in clojure.core that is causing the problem.

Comment by Alex Miller [ 11/Apr/14 11:17 AM ]

Marking this as NR for now - would be happy to see it reopened with an easily reproducible test case.

Comment by Mike Anderson [ 12/Apr/14 12:41 AM ]

To reproduce:

(ns op)
(defn * [a b] (clojure.core/* a b)) ;; gives warning
(ns use-op (:require [op :refer :all])) ;; gives warning
(ns use-op (:require [op :refer :all])) ;; gives error!

I believe Counterclockwise is simply loading the namespace again with CTRL-Alt+L, which is causing the ns form to be re-executed.

The docstring implies that ns can be used multiple times ("Sets ns to the namespace named by name (unevaluated), creating it if needed") so I would certainly expect multiple invocations of ns to be a no-op

Comment by Alex Miller [ 03/Nov/14 10:24 AM ]

Duped in CLJ-1578.

Comment by Mike Anderson [ 09/Jun/15 3:54 AM ]

This is still affecting me, and causing breakage with the latest versions of core.matrix. I don't know if this is a regression or not, but it certainly happens in 1.7.0-RC1

Any chance we can get a fix for 1.7? It is really annoying to have code fail because of this and force of refactoring of user code (my use case is adding a new var to clojure.core.matrix namespace, compiler error in user code that previously defined a var with the same name).

Comment by Mike Anderson [ 09/Jun/15 3:59 AM ]

Closing because I think this is better handled in the related issue

Comment by Mike Anderson [ 09/Jun/15 5:05 AM ]

Reopening because CLJ-1578 apparently does not resolve this specific issue, it only covers vars in clojure.core.

I'd still like to see this fixed for all namespaces, not just clojure.core.

Comment by Mike Anderson [ 09/Jun/15 5:08 AM ]

Reproduction:

=> (ns foo)
nil
=> (def a 1)
#'foo/a
=> (ns bar (:require [foo :refer :all]))
nil
=> (def a 2)
CompilerException java.lang.IllegalStateException: a already refers to: #'foo/a in namespace: bar, compiling:(NO_SOURCE_PATH:1:1)
=> clojure-version
{:major 1, :minor 7, :incremental 0, :qualifier "RC1"}

Stack trace:

CompilerException java.lang.RuntimeException: Unable to resolve symbol: pst in this context, compiling:(NO_SOURCE_PATH:1:1)
clojure.lang.Compiler.analyze (Compiler.java:6543)
clojure.lang.Compiler.analyze (Compiler.java:6485)
clojure.lang.Compiler$InvokeExpr.parse (Compiler.java:3737)
clojure.lang.Compiler.analyzeSeq (Compiler.java:6735)
clojure.lang.Compiler.analyze (Compiler.java:6524)
clojure.lang.Compiler.analyze (Compiler.java:6485)
clojure.lang.Compiler$BodyExpr$Parser.parse (Compiler.java:5861)
clojure.lang.Compiler$FnMethod.parse (Compiler.java:5296)
clojure.lang.Compiler$FnExpr.parse (Compiler.java:3925)
clojure.lang.Compiler.analyzeSeq (Compiler.java:6731)
clojure.lang.Compiler.analyze (Compiler.java:6524)
clojure.lang.Compiler.eval (Compiler.java:6789)
Caused by:
RuntimeException Unable to resolve symbol: pst in this context
clojure.lang.Util.runtimeException (Util.java:221)
clojure.lang.Compiler.resolveIn (Compiler.java:7029)
clojure.lang.Compiler.resolve (Compiler.java:6973)
clojure.lang.Compiler.analyzeSymbol (Compiler.java:6934)
clojure.lang.Compiler.analyze (Compiler.java:6506)
clojure.lang.Compiler.analyze (Compiler.java:6485)

Comment by Nicola Mometto [ 09/Jun/15 5:16 AM ]

As I already commented in CLJ-1578, I don't think this is a bug and I think this ticket should be declined.

Overriding non clojure.core vars has always (since 1.2 at least) caused an exception to be thrown.

Comment by Nicola Mometto [ 09/Jun/15 5:23 AM ]

Mike, maybe it would make sense to bring this issue up in the clojure-dev ml to get some opinions?

Comment by Mike Anderson [ 09/Jun/15 5:42 AM ]

Re-classify it as a feature request, if you prefer.

I still regard it as a defect because I expect :refer :all to work sanely.

Either way, this issue keeps breaking user code in my area (data science / exploratory statistics / data management). The ability to use / refer all is very useful for setting up a convenient namespace for exploratory work, so I don't accept that forcing users to explicitly require every single var used (as Nicola suggests in CLJ-1578) is a practical workaround.

I've also had it cause problems when working at the REPL and reloading namespaces.

If the Clojure core team really wants to keep this annoying behaviour, can we at least have some way to turn it off at the library level? Perhaps some namespace metadata that I can add to the clojure.core.matrix namespace to stop this from triggering?

Comment by Nicola Mometto [ 09/Jun/15 6:23 AM ]

Mike, this is just my personal opinion, I'm not a part of the core team and I don't speak for them, this is why I suggested you wrote on the clojure-dev ml.

Also to clarify, this issue you're reporting cannot manifest itself while reloading namespaces as the exception is thrown as soon as the redefinition happens.

Comment by Alex Miller [ 09/Jun/15 8:47 AM ]

You could use :exclude for this

(ns bar (:require [foo :refer :all :exclude (a)]))
Comment by Mike Anderson [ 09/Jun/15 9:30 AM ]

Hi Alex, that works as a fix when the problem occurs, but doesn't solve the problem of future user code breakage, unless the user accurately anticipates what symbols might get added to "bar" in the future. Which again seems like an unreasonable burden on the user.

What I'm arguing for, I guess, is a default presumption that if the user defines a var in their own namespace, they are happy to replace a similarly named var in any namespaces that they have previously use'd / refer-all'd.

If the user is genuinely concerned about overriding things by accident, I'd be happy with a warn-on-replace which does something analogous to warn-on-reflection. I proposed something similar in CLJ-1257 a while back, even wrote a patch that solves the whole problem in this way. Can we get that patch or something similar in 1.7?

Comment by Nicola Mometto [ 09/Jun/15 10:22 AM ]

CLJ-1746 is relevant and I like that proposal much better than CLJ-1257

Comment by Mike Anderson [ 09/Jun/15 10:43 AM ]

Hi Nicola, CLJ-1746 looks like a reasonable suggestion but still doesn't address the problem here, for the same reason that :exclude doesn't (see my comment to Alex)

The fundamental issue is that the current behaviour causes user code to break when libraries are upgraded to add new vars and requires changes to user code to fix / work around it. I consider that broken behaviour, or at least very bad design.

This is especially since we are encouraged to choose good names: I quote from the library coding standards(http://dev.clojure.org/display/community/Library+Coding+Standards)

"Use good names, and don't be afraid to collide with names in other namespaces. That's what the flexible namespace support is there for."

It isn't very flexible when user code keep breaking.....

Comment by Nicola Mometto [ 09/Jun/15 10:53 AM ]

From the same page, just a two lines below:
"Be explicit and minimalist about dependencies on other packages. (Prefer :require :refer in 1.4+ or :use :only in 1.0-1.3)."

If users carelessly import a whole package, I'd say that's their fault. Since clojure >1.3 the popular consensus has been that :use/:refer :all are not a good idea and people have been moving away from that, preferring :require :refer or :require :as instead.

The few that still use :use/:refer :all do it mostly for backward compatibility or in tests (I myself do it in tools.reader for the former reason).

I really don't think this is such a bad design choice as you seem to think, and I think that most people in the community would agree that the current behaviour and drive towards using :require :refer/require :as in lieu of :use/:refer :all is way more beneficial than harmful

Comment by Mike Anderson [ 09/Jun/15 11:42 AM ]

Hi Nicola, I have no issue with people using explicit :refer / :require, and I agree it is normal practice. It's good to be explicit and I generally do that myself (except in test / demo code).

However we aren't talking about that case : this issue is most relevant in those situations where users (for their own legitimate reasons) have decided to import a whole namespace. This is often convenient (e.g. REPL usage), sometimes it is valuable for specific purposes (e.g. testing).

I personally care about this mostly from the perspective of a library author: I want users to be able to use the library in whatever way is most convenient (which may include importing all vars), and I don't want user code to break randomly when I make a new release. Note that this also means I don't have control over user code so any solution that involves explicit requires, excludes, or some other manual workarounds is not a practical solution.

You can say "that's their fault" but this is currently supposed to be supported in Clojure so I think it ought to work sanely.

Comment by Alex Miller [ 09/Jun/15 12:00 PM ]

I think the library / evolution argument is a good one and I like that it reduces breakage due to evolution of 3rd party libraries. (I feel very strongly about this in clojure.core itself which is auto-referred, less strongly otherwise.)

On the flip side, if we remove this error, we should be explicit about what we are giving up to fully consider it. Presumably we are at least giving up a helpful error that occurs in the case of an accidental override. Are there other impacts?

I do not expect that we're going to change anything in 1.7.

Comment by Stuart Halloway [ 31/Jul/15 3:10 PM ]

Recategorizing as a feature request, as the current behavior was the original intent.

Could a namespace-reflecting macro provide for this use case without requiring a change to core?





[CLJ-1074] Read/print round-trip for +/-Infinity and NaN Created: 21/Sep/12  Updated: 31/Jul/15

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

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

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

 Description   

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

(read-string (binding [*print-dup* true] (pr-str f))

The two options I see are to provide print-method implementations for these and their Float cousins, or to make Infinity, -Infinity, +Infinity, and NaN readable values. Since it sounds like edn may want to provide a spec for these values (see https://groups.google.com/d/topic/clojure-dev/LeJpOhHxESs/discussion and https://github.com/edn-format/edn/issues/2), I think making these values directly readable as already printed is preferable. Something like Double/POSITIVE_INFINITY seems too low-level from edn's perspective, as it would refer to a Java class and constant.

I'm attaching a patch implementing reader support for Infinity, -Infinity, +Infinity, and NaN.



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

Please bring this up on clojure-dev. We'll be able to vet this ticket after that.

Comment by Colin Jones [ 03/Dec/12 1:18 PM ]

Should I respond to my original clojure-dev post about this (linked in the issue description above), or start a new one?

Comment by Andy Fingerhut [ 24/May/13 1:11 PM ]

Patch clj-1074-read-infinity-and-nan-patch-v2.txt dated May 24 2013 is identical to 0001-Read-Infinity-and-NaN.patch dated Sep 21 2012, except it applies cleanly to latest master. The older patch conflicts with a recent commit made for CLJ-873.

Comment by Nicola Mometto [ 25/May/13 11:55 AM ]

clj-1074-read-infinity-and-nan-patch-v2-plus-edn-reader.patch is the same as clj-1074-read-infinity-and-nan-patch-v2.txt except it patches EdnReader too, but it must be applied after #CLJ-873 0001-Fix-CLJ-873-for-EdnReader-too.patch get merged

Comment by Andrew Tarzwell [ 12/Feb/15 12:01 PM ]

We're running into this bug now, applying the patch clj-1074-read-infinity-and-nan-patch-v2-plus-edn-reader.patch seems to resolve it on 1.7 master, but it would be an improvement to not depend on a patched version.

Is there a fix in the works? Or a more up to date conversation on why this hasn't been addressed?

Thanks,
Andrew

Comment by Andy Fingerhut [ 12/Feb/15 12:23 PM ]

Andrew, the tools.reader library provides this enhancement today, if you would prefer using unpatched software, and if it meets your needs. There are a few other small differences between it and the reader built into Clojure, which you can see near the end of its README: https://github.com/clojure/tools.reader

As far as why it hasn't been addressed yet, I think a short accurate answer is that the Clojure developers have been working on other issues, and this one has not been high enough priority to be addressed yet (disclaimer: This is in no way an official answer, just the best guess of an observer who watches this stuff too much).

I see you have voted on the ticket. Good. More votes can in some cases influence the Clojure developers to respond to a ticket earlier rather than later. You may try to persuade others to vote on it, too, if you wish.

If there is some production use of Clojure hindered by the lack of a fix, bringing this up in the Clojure or Clojure Dev Google groups couldn't hurt (signed CA on file required for Clojure Dev group membership – see http://clojure.org/contributing )

Comment by Andrew Tarzwell [ 12/Feb/15 1:46 PM ]

Andy,

Thank you for the quick response, I was unaware of tools.reader having this fixed. That should do us for now.





[CLJ-1446] (def v) with no init supplied destroys #'v metadata Created: 13/Jun/14  Updated: 31/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Nahuel Greco Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Approval: Triaged

 Description   

(def a) destroys #'a metadata, check this:

(def ^:mykey a 1)

(meta #'a)              ;; ok, :mykey is present

(let [v (def a)]
   [(meta v)            ;; NO :mykey present, metadata destroyed
    (identical? v #'a)  ;; true, we are talking of the same var
   ])

(meta #'a)              ;; NO :mykey present

If this is not a bug but a "feature", then we have at least two problems:

1- The def special form documentation doesn't state this behaviour at all, it needs to be clarified. With the current documentation it seems as doing a def with no init supplied will not make any side-effect at all, and this is not true for the var metadata.

2- defmulti uses this form to lookup the var and check if it already binds to a MultiFn, if that is the case then defmulti does nothing... but it really does something, defmulti will destroy the original var metadata in the (supposedly non-destructive) check. This is the involved defmulti fragment:

(let [v# (def ~mm-name)]
  (when-not (and (.hasRoot v#) (instance? clojure.lang.MultiFn (deref v#)))
   ...


 Comments   
Comment by Alex Miller [ 13/Jun/14 4:14 PM ]

I think this is mostly a dupe of CLJ-1148 but I'll leave it as it states the specific problem more precisely.

Comment by Nahuel Greco [ 13/Jun/14 7:35 PM ]

Alex Miller: It seems CLJ-1148 is an special case where this problem shows, but the patches in CLJ-1148 only fixes the issues for defonce, not generally for def, not for defmulti and not clarifies this behaviour in the def special form documentation.

Comment by Stuart Halloway [ 31/Jul/15 2:49 PM ]

I am pretty sure we have been here before, and decided that def is working as desired. (If anybody can find the thread/ticket please add a link.) I think this should be a doc enhancement.

If the behavior of defmethod is a separate bug, please make a separate ticket for that, showing an example problem.

Comment by Andy Fingerhut [ 31/Jul/15 3:00 PM ]

CLJ-1213 might be related, but it doesn't mention metadata, only (def foo) without init value given.





[CLJ-1733] print-dup form unreadable for sorted sets and maps Created: 19/May/15  Updated: 31/Jul/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   

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

Printing

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

Can't read back

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

Possible Fixes

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


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

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

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

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

(defrecord Rec [f])

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

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





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

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

Type: Enhancement Priority: Major
Reporter: Bozhidar Batsov Assignee: Nola Stowe
Resolution: Unresolved Votes: 29
Labels: ft, string

Attachments: Text File add_functions_to_strings-2.patch     Text File add_functions_to_strings-3.patch     Text File add_functions_to_strings-4.patch     Text File add_functions_to_strings-5.patch     Text File add_functions_to_strings-6.patch     Text File add_functions_to_strings.patch     Text File clj-1449-more-v1.patch    
Patch: Code
Approval: Vetted

 Description   

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

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

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

Patch:

  • add_functions_to_strings-4.patch - uses -1 to indicate not-found in index-of
  • add_functions_to_strings-6.patch - uses nil to indicate not-found in index-of

Performance: Tested the following with criterium for execution mean time (all times in ns).

Java Clojure Java Clojure (-4 patch) Clojure (-6 patch)
(.indexOf "banana" "n") (clojure.string/index-of "banana" "n") 8.70 9.03 9.27
(.indexOf "banana" "n" 1) (clojure.string/index-of "banana" "n" 1) 7.29 7.61 7.66
(.indexOf "banana" (int \n)) (clojure.string/index-of "banana" \n) 5.34 6.20 6.20
(.indexOf "banana" (int \n) 1) (clojure.string/index-of "banana" \n 1) 5.38 6.19 6.24
(.startsWith "apple" "a") (clojure.string/starts-with? "apple" "a") 4.84 5.71 5.65
(.endsWith "apple" "e") (clojure.string/ends-with? "apple" "e") 4.82 5.68 5.70
(.contains "baked apple pie" "apple") (clojure.string/includes? "baked apple pie" "apple") 10.78 11.99 12.17

Screened by:

Note: In both Java and JavaScript, indexOf will return -1 to indicate not-found, so this is (at least in these two cases) the status quo. The benefit of nil return in Clojure is that it can be used as a found/not-found condition. The benefit of a -1 return is that it matches Java/JavaScript and that the return type can be hinted as a primitive long, allowing you to feed it into some other arithmetic or string operation without boxing.



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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

What about an implementation that works also in cljs?

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

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

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

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

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

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

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

Some things that would be helpful:

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

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

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

Alex, all your comments make sense.

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

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

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

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

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

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

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

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

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

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

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

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

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

Ruby uses "include?" for this.

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

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

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

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

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

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

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

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

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

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

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

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

Comment by Nola Stowe [ 08/Jul/15 12:46 PM ]

Updated patch to rename methods as specified and add tests.

Comment by Bozhidar Batsov [ 08/Jul/15 3:26 PM ]

A few remarks:

1. added should say 1.8, not 1.9.
2. The docstrings could be improved a bit - normally they should end with a . and refer to all the params of the function in question.

Comment by Nola Stowe [ 08/Jul/15 3:29 PM ]

Thanks Bozhidar Batsov .. I will, and I also got feedback from the slack clojure-dev group (Alex, Ghadi, AndrewHr) so I will be submitting an improved patch soon

Comment by Andy Fingerhut [ 09/Jul/15 11:57 AM ]

Also, I am not sure, but Alex Miller made this comment earlier: "Per the instructions at the top of the clojure.string ns, functions should take the broader CharSequence interface instead of String. Similar to existing clojure.string functions, you will need to provide a CharSequence implementation while also calling into the String functions when you actually have a String."

Looking at the Java classes and interfaces, one of the classes that implements CharSequence is CharBuffer, and it has no indexOf or lastIndexOf methods. If one were to call the proposed index-of or last-index-of with a CharBuffer, I believe you would get an exception for an unknown method.

I don't know if Alex's sentence is essential to any patch to be considered, but wanted to point out that it seems like it effectively means writing your own implementation of each of these methods for some of the classes implementing the CharSequence interface.

Comment by Nola Stowe [ 09/Jul/15 4:15 PM ]

Andy, yes you are right. Alex helped me with my current changes (not posted yet) and where needed I cast the value to a string so that it would have the indexOf methods.

Comment by Nola Stowe [ 10/Jul/15 7:43 AM ]

Added type hints, using StringBuffer instead of string in tests.

Comment by Nola Stowe [ 10/Jul/15 11:53 PM ]

Use critium and did some bench marking, I am not familiar enough to say "hey its good or no, too slow". I tried an optimization but it didn't make much difference.

notes here:

https://www.evernote.com/l/ABLYZbDr8ElIs6fkODAd3poBfIm_Bfr5kLo

Appreciate any feedback

Comment by Alex Miller [ 13/Jul/15 10:37 AM ]

Nola, some comments:

1) In index-of and last-index-of, I think we should use (instance? Character value) instead of char? - this will avoid a function invocation.

2) In index-of and last-index-of, in the branches where you know you have a Character, we should just invoke the proper function on the character instance directly rather than calling int. In this case that is (.charValue ^Character value). However, this function returns char so you still want the ^int hint.

3) In index-of and last-index-of, we should do a couple of things to maximally utilize primitive support for from-index. We should type-hint the incoming value as a ^long (only long and double are specialized cases in Clojure, not int). We should then use the function unchecked-int to optimally convert the long primitive to an int primitive.

4) We should type-hint the return value to a primitive long to avoid boxing the return.

Putting 1+2+3+4 together:

(defn index-of
  "Return index of value (string or char) in s, optionally searching forward from from-index."
  {:added "1.8"}
  (^long [^CharSequence s value]
   (if (instance? Character value)
     (.indexOf (.toString s) ^int (.charValue ^Character value))
     (.indexOf (.toString s) ^String value)))
  (^long [^CharSequence s value ^long from-index]
  (if (instance? Character value)
    (.indexOf (.toString s) ^int (.charValue ^Character value) (unchecked-int from-index))
    (.indexOf (.toString s) ^String value (unchecked-int from-index)))))

Similar changes in last-index-of.

5) In starts-with? and ends-with?, I would move the type hint for substr to the parameter declaration. This is more of a style preference on my part, no functional difference I believe.

6) On includes? I would do the same as #5, except .contains actually takes a CharSequence for the substr, so I would change the type hint from ^String to the broader ^CharSequence.

7) Can you edit the description and add a table with the "execution time mean" number for direct Java vs new Clojure fn for the patch after these changes? Please add tests for the from-index variants of index-of and last-index-of as well.

Comment by Nola Stowe [ 14/Jul/15 5:46 PM ]

optimizations as suggested by Alex Miller

Comment by Andy Fingerhut [ 14/Jul/15 5:50 PM ]

Nola, not a big deal, but Rich has requested that patch file names end with ".patch" or ".diff", I think because whatever he uses to view them recognizes that suffix and puts it in a different viewing/editing mode.

Comment by Nola Stowe [ 14/Jul/15 6:03 PM ]

Andy, so no need to keep around patches when used in comparisons?

Comment by Andy Fingerhut [ 14/Jul/15 8:31 PM ]

It is ok to have multiple versions of patches attached, but names like add_functions_to_strings-2.patch etc. would be preferred over names like add_functions_to_strings.patch-2, so the suffix is ".patch".

Comment by Nola Stowe [ 14/Jul/15 8:55 PM ]

original patch submitted by Nola

Comment by Nola Stowe [ 14/Jul/15 8:56 PM ]

Optimizations suggested by Alex

Comment by Alex Miller [ 14/Jul/15 9:44 PM ]

Nola, I re-did the timings on my machine - these were done with Java 1.8 and no Leiningen involved.

Comment by Alex Miller [ 14/Jul/15 9:57 PM ]

-4 patch is identical to -3 but removes one errant ^long type hint

Otherwise, looks good to me!

Comment by Alex Miller [ 17/Jul/15 10:24 PM ]

Rich, since you didn't put any comments on here, I'm not sure if there's anything else you wanted, so marking screened.

Comment by Stuart Halloway [ 19/Jul/15 7:36 AM ]

I dislike the approach here for several reasons:

  • floats Java-isms (-1 for missing instead of nil?) up into the Clojure API
  • makes narrow string fns out of things that could/should be seq fns

Stepping back, this is all library work. Why should it be in core? If this started in a contrib, it could evolve much more freely.

Comment by Alex Miller [ 19/Jul/15 8:49 AM ]

re Java-isms: totally fair comment worth changing

re narrow string fns: if these were seq fns, they would presumably take seqables of chars and return seqables of chars. When I have strings, I want to work with strings (as with clojure.string/join and clojure.string/reverse vs their seq equivalents).

re library: these particular functions are the most commonly used string functions where current users drop to Java interop. Note that all 5 of these functions have over time been added to the cheatsheet (http://clojure.org/cheatsheet) because they are commonly used. I believe there is significant value in including them in core and standardizing their name and behavior across to CLJS.

Comment by Bozhidar Batsov [ 19/Jul/15 10:29 AM ]

I'm obviously biased, but I totally agree with everything Alex said. Having one core string library and a separate contrib string library is just impractical. With clojure.string already part of the language it's much more sensible to extend it where/when needed instead of encouraging the proliferation of tons of alternative libraries (there are already a couple of those out there, mostly because essential functions are missing). Initial designs are rarely perfect, there's nothing wrong in revisiting and improving them.

I believe this is one of the top voted tickets, which clearly indicates that the Clojure community is quite supportive of this additions.

Comment by Michael Blume [ 19/Jul/15 12:26 PM ]

None of these functions return Strings so they could very easily be seq functions which use the String methods as a fast path when called with Strings, the trouble is then they wouldn't necessarily belong in clojure.string.

Comment by Bozhidar Batsov [ 19/Jul/15 1:49 PM ]

Such an argument can be made for existing functions like clojure.core/subs and clojure.string/reverse - we could have just went with generic functions that operate on seqs and converted the results to strings, but we didn't. It might be just me, but I'm really thinking this is being overanalysed. Sometimes it's really preferable to deal with some things as strings (not to mention more efficient).

Comment by Rich Hickey [ 20/Jul/15 9:39 AM ]

I completely agree with Alex and Bozhidar re: strings vs seqs. Other langs that have only lists and use them for strings are really bad at strings. So - string-specific fns are ok. What about the Java-isms?

Comment by Bozhidar Batsov [ 20/Jul/15 10:08 AM ]

We should address them for sure. Let's wait for a couple of days for Nola to update the patch. If she's too busy I'll do the update myself.

Comment by Nola Stowe [ 20/Jul/15 10:50 AM ]

I will be happy to work on it Saturday. Is nil an acceptable response when index-of "apple" "p" is not found? The other functions return booleans and I think those are ok.

Comment by Bozhidar Batsov [ 20/Jul/15 10:55 AM ]

Yep, we should go with nil.

Comment by Nola Stowe [ 25/Jul/15 4:33 PM ]

Changed index-of and last-index-of to return nil instead of -1 (java-ism), thus had to remove the type hint for the return value.

Seems to have slowed down the function compared to having the function return -1. My benchmarks: https://www.evernote.com/l/ABL2wead73BJZYAkpH5yxsfX9JM8cpUiNhw

Comment by Nola Stowe [ 25/Jul/15 9:05 PM ]

Updated patch to include return value in doc string for index-of last-index-of, otherwise same as last update.

Comment by Alex Miller [ 26/Jul/15 8:40 AM ]

Removing the hint causes the return type to be Object, which forces boxing in the "found" cases.

Comment by Nola Stowe [ 26/Jul/15 8:56 AM ]

Is it worth it to remove the java-ism of returning -1 when not found? Personally, I don't really like a function returning one of two things... found index or nil ... I prefer it being an int at all times.

Comment by Alex Miller [ 26/Jul/15 9:00 AM ]

There is one whitespace issue in the -5 patch:

[~/code/clojure (master)]$ git apply ~/Downloads/add_functions_to_strings-5.patch
/Users/alex/Downloads/add_functions_to_strings-5.patch:34: trailing whitespace.
  (let [result
warning: 1 line adds whitespace errors.

I updated the timings in the description to include both -4 and -5 - the boxing there does have a significant impact.

Comment by Nola Stowe [ 26/Jul/15 9:28 AM ]

Fixed whitespace issue.

Comment by Alex Miller [ 26/Jul/15 9:32 AM ]

Added -6 patch and timings that address most of the performance impact.

Comment by Stuart Halloway [ 27/Jul/15 9:35 AM ]

Alex: Note that all of these functions return numbers and booleans, not strings nor seqs of anything. This is partially why they were left out of the original string implementation.

It isn't clear to me why index-of should be purely a string fn, I regularly need index-of with other things, and both my book and JoC use index-of as an example of a nice-to-have.

Comment by Nicola Mometto [ 27/Jul/15 9:44 AM ]

Stuart, I don't think the fact that those functions don't return string is really relevant, the important fact is that they operate on strings (just like c.set/superset? or subset? don't return sets) and are frequently used on them.

Comment by Bozhidar Batsov [ 27/Jul/15 9:51 AM ]

I'm with Nicola on this one - we have to be practical. There's a reason why this is the top-voted JIRA ticket - Clojure programmers want those functions. That's says plenty.

Anyways, having a generic `index-of` is definitely not a bad idea.

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

How does this compare to making them all seq fns and adding them to clojure.core?

Comment by Andy Fingerhut [ 31/Jul/15 11:39 AM ]

Given Rich's comment earlier "I completely agree with Alex and Bozhidar re: strings vs seqs. Other langs that have only lists and use them for strings are really bad at strings. So - string-specific fns are ok.", do we need to write a separate patch that makes these all into seq fns and do performance comparisons between those vs. the string-specific ones?

Comment by Stuart Halloway [ 31/Jul/15 1:26 PM ]

Andy,

Definitely not! I want to have a design conversation.

We have protocols, and could use them to provide core fns that are specialized to strings, so it isn't clear to me why we would have to be really bad at it.





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

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

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

clojure 1.6.0, clojure 1.7.0-beta1


Approval: Triaged

 Description   

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

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

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



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

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

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

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





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

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

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

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

 Description   

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

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

(reset-test)

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

Expected behavior

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

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

(swap-test)

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

Alternative expected behavior

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

Analysis

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

Approach

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

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


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

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

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

Results

Existing implementation:

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

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

Suggested implementation:

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

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

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

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





[CLJ-700] contains? broken for transient collections Created: 01/Jan/11  Updated: 31/Jul/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: Rich Hickey
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: Incomplete

 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.

Screening Notes

  • the extra conditions in RT add branches to some key functions. get already has a getFrom optimization, but there is no similar containsFrom or findFrom. Is it worth measuring the possible impact of these?
  • I believe the interface refactoring approach (not taken here) is worth separate consideration as an enhancement. If this is done, I think leveraging valAt would be simpler, e.g. allowing HashMap and ArrayMap to share code
  • it is not evident (to me anyway) why some API fns consume ILookup and others do not, among e.g. contains?, get, and find. Possible doc enhancement?
  • there is test code already in place (data_structures.clj) that could easily be expanded to cover transients. It would be nice to do this, or better yet get some test.check tests in place

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.

Comment by Rich Hickey [ 31/Jul/15 11:35 AM ]

I'd like to look at the type hierarchy myself





[CLJ-1224] Records do not cache hash like normal maps Created: 24/Jun/13  Updated: 31/Jul/15

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

Type: Enhancement Priority: Critical
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 17
Labels: defrecord, performance

Attachments: Text File 0001-cache-hasheq-and-hashCode-for-records.patch     Text File 0001-cache-hasheq-and-hashCode-for-records-v2.patch     Text File 0001-CLJ-1224-cache-hasheq-and-hashCode-for-records.patch     Text File 0001-CLJ-1224-cache-hasheq-and-hashCode-for-records-v2.patch    
Patch: Code and Test
Approval: Screened

 Description   

Records do not cache their hash codes like normal Clojure maps, which affects their performance. This problem has been fixed in CLJS, but still affects JVM CLJ.

Approach: Cache hash values in record definitions, similar to maps.

Timings:

coll 1.8.0-master 1.8.0-master+patch
small record 99 ns 9 ns
big record 455 ns 9 ns
(defrecord R [a b])  ;; small
(def r  (R. (range 1e3) (range 1e3)))
(bench (hash r))
(defrecord R [a b c d e f g h i j])  ;; big
(def r (map->R (zipmap [:a :b :c :d :e :f :g :h :i :j] (repeat (range 1e3)))))
(bench (hash r))

Patch: 0001-CLJ-1224-cache-hasheq-and-hashCode-for-records-v2.patch

Screened by: Alex Miller

Also see: http://dev.clojure.org/jira/browse/CLJS-281



 Comments   
Comment by Nicola Mometto [ 14/Feb/14 5:46 PM ]

I want to point out that my patch breaks ABI compatibility.
A possible approach to avoid this would be to have 3 constructors instead of 2, I can write the patch to support this if desired.

Comment by Stuart Halloway [ 27/Jun/14 11:09 AM ]

The patch 0001-CLJ-1224-cache-hasheq-and-hashCode-for-records.patch is broken in at least two ways:

  • The fields __hash and __hasheq are adopted by new records created by .assoc and .without, which will cause those records to have incorrect (and likely colliding) hash values
  • The addition of the new fields breaks the promise of defrecord, which includes an N+2 constructor taking meta and extmap. With the patch, defrecords get an N+4 constructor letting callers pick hash codes.

I found these problems via the following reasoning:

  • Code has been touched near __extmap
  • Grep for all uses of __extmap and see what breaks
Comment by Nicola Mometto [ 27/Jun/14 2:53 PM ]

Patch 0001-cache-hasheq-and-hashCode-for-records.patch fixes both those issues, reintroducing the N+2 arity constructor

Comment by Alex Miller [ 27/Jun/14 4:08 PM ]

Questions addressed, back to Vetted.

Comment by Andy Fingerhut [ 29/Aug/14 4:32 PM ]

All patches dated Jun 7 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.

Comment by Alex Miller [ 29/Aug/14 4:40 PM ]

Would be great to get this one updated as it's otherwise ready to screen.

Comment by Nicola Mometto [ 29/Aug/14 4:58 PM ]

Updated patch to apply to lastest master

Comment by Alex Miller [ 16/Jun/15 4:06 PM ]

1) hash and hasheq are stored as Objects, which seems kind of gross. It would be much better to store them as primitive longs and check whether they'd been calculated by comparing to 0. We still end up with a long -> int conversion but at least we'd avoid boxing.

2) assoc wrongly copies over the __hash and __hasheq to the new instance:

(defrecord R [a])
(def r (->R "abc"))
(hash r)                   ;; -1544829221
(hash (assoc r :a "def"))  ;; -1544829221

3) Needs some tests if they don't already exist:

  • with-meta on a record should not affect hashcode
  • modifying a record with assoc or dissoc should affect hashcode
  • maybe something else?

4) Needs some perf tests with a handful of example records (at least: 1 field, many fields, extmap, etc).

Nicola, I'm happy to let you continue to do dev on this patch with me doing the screening if you have time. Or if you don't have time, let me know and I (or someone else) can work on the dev parts. Would like to get this prepped and clean for 1.8.

Comment by Nicola Mometto [ 16/Jun/15 5:56 PM ]

Updated patch addresses the issues raised, will add some benchmarks later

Comment by Nicola Mometto [ 16/Jun/15 5:59 PM ]

Alex, regarding point 1, I stored __hash and __hasheq as ints rather than longs and compared to -1 rather than 0, to be consistent with how it's done elsewhere in the clojure impl

Comment by Alex Miller [ 17/Jun/15 11:39 AM ]

Looking at the bytecode for hashcode and hasheq, I had two questions:

1) the -1 there is a long and requires an upcast of the private field from int to long. I'm sure that's not a big deal, but wish there was a way to avoid it. I didn't try it but maybe (int -1) would help the compiler out?

2) I wonder whether something like this would perform better:

`(hasheq [this#] 
   (if (== -1 ~'__hasheq)
     (set! ~'__hasheq (int (bit-xor ~type-hash (clojure.lang.APersistentMap/mapHasheq this#)))))
   ~'__hasheq)

The common case will be a failed compare and then the field can be loaded and returned directly without any casting.

Comment by Nicola Mometto [ 17/Jun/15 11:54 AM ]

1- there's no Numbers.equiv(int, int) so even casting -1 to an int wouldn't solve this. a cast is always necessary. if we were to make hasheq a long, we'd need l2i in the return path, making hasheq an int we need an i2l in the comparison.
2- that doesn't remove any casting, it just replaces a load from the local variable stack with a field load:

;; current version
0: ldc2_w        #203                // long -1l
3: aload_0
4: getfield      #236                // Field __hasheq:I
7: i2l
8: lcmp
9: ifne          38
12: ldc2_w        #267                // long 1340417398l
15: aload_0
16: checkcast     #16                 // class clojure/lang/IPersistentMap
19: invokestatic  #274                // Method clojure/lang/APersistentMap.mapHasheq:(Lclojure/lang/IPersistentMap;)I
22: i2l
23: lxor
24: invokestatic  #278                // Method clojure/lang/RT.intCast:(J)I
27: istore_1
28: aload_0
29: iload_1
30: putfield      #236                // Field __hasheq:I
33: iload_1
34: goto          42
37: pop
38: aload_0
39: getfield      #236                // Field __hasheq:I
42: ireturn
;; your version
0: ldc2_w        #203                // long -1l
3: aload_0
4: getfield      #236                // Field __hasheq:I
7: i2l
8: lcmp
9: ifne          35
12: aload_0
13: ldc2_w        #267                // long 1340417398l
16: aload_0
17: checkcast     #16                 // class clojure/lang/IPersistentMap
20: invokestatic  #274                // Method clojure/lang/APersistentMap.mapHasheq:(Lclojure/lang/IPersistentMap;)I
23: i2l
24: lxor
25: invokestatic  #278                // Method clojure/lang/RT.intCast:(J)I
28: putfield      #236                // Field __hasheq:I
31: goto          37
34: pop
35: aconst_null
36: pop
37: aload_0
38: getfield      #236                // Field __hasheq:I
41: ireturn
Comment by Alex Miller [ 17/Jun/15 12:01 PM ]

Fair enough! Looks pretty good to me, still needs the perf numbers.

Comment by Nicola Mometto [ 17/Jun/15 1:00 PM ]
coll 1.7.0-RC2 1.7.0-RC2+patch
big record ~940ns ~10ns
small record ~150ns ~11ns
;; big record, 1.7.0-RC2
user=> (use 'criterium.core)
nil
user=> (defrecord R [a b c d e f g h i j])
user.R
user=> (def r (map->R (zipmap [:a :b :c :d :e :f :g :h :i :j] (repeat (range 1e3)))))
#'user/r
user=> (bench (hash r))
WARNING: Final GC required 1.291182176566658 % of runtime
Evaluation count : 63385020 in 60 samples of 1056417 calls.
             Execution time mean : 943.320293 ns
    Execution time std-deviation : 44.001842 ns
   Execution time lower quantile : 891.919381 ns ( 2.5%)
   Execution time upper quantile : 1.033894 µs (97.5%)
                   Overhead used : 1.980453 ns

;; big record, 1.7.0-RC2 + patch
user=> (defrecord R [a b c d e f g h i j])
user.R
user=> (def r (map->R (zipmap [:a :b :c :d :e :f :g :h :i :j] (repeat (range 1e3)))))
#'user/r
user=> (bench (hash r))
WARNING: Final GC required 1.0097162582088168 % of runtime
Evaluation count : 4820968380 in 60 samples of 80349473 calls.
             Execution time mean : 10.657581 ns
    Execution time std-deviation : 0.668011 ns
   Execution time lower quantile : 9.975656 ns ( 2.5%)
   Execution time upper quantile : 12.190471 ns (97.5%)
                   Overhead used : 2.235715 ns

;; small record 1.7.0-RC2
user=> (defrecord R [a b])
user.R
user=> (def r  (R. (range 1e3) (range 1e3)))
#'user/r
user=> (bench (hash r))
WARNING: Final GC required 1.456092401467115 % of runtime
Evaluation count : 423779160 in 60 samples of 7062986 calls.
             Execution time mean : 147.154359 ns
    Execution time std-deviation : 8.148340 ns
   Execution time lower quantile : 138.052054 ns ( 2.5%)
   Execution time upper quantile : 165.573489 ns (97.5%)
                   Overhead used : 1.629944 ns

;; small record 1.7.0-RC2+patch
user=> (defrecord R [a b])
user.R
user=> (def r  (R. (range 1e3) (range 1e3)))
#'user/r
user=>  (bench (hash r))
WARNING: Final GC required 1.720638384341818 % of runtime
Evaluation count : 4483195020 in 60 samples of 74719917 calls.
             Execution time mean : 11.696574 ns
    Execution time std-deviation : 0.506482 ns
   Execution time lower quantile : 10.982760 ns ( 2.5%)
   Execution time upper quantile : 12.836103 ns (97.5%)
                   Overhead used : 2.123801 ns
Comment by Alex Miller [ 17/Jun/15 3:36 PM ]

Screened for 1.8.

Comment by Alex Miller [ 22/Jun/15 8:38 AM ]

Note that using -1 for the uncomputed hash value can cause issues with transient lazily computed hash codes on serialization (CLJ-1766). In this case, the defrecord cached code is not transient so I don't think it's a problem, but something to be aware of. Using 0 would avoid this potential issue.

Comment by Rich Hickey [ 17/Jul/15 12:00 PM ]

So, why not use 0? You won't need initialization then either

Comment by Nicola Mometto [ 17/Jul/15 7:50 PM ]

Updated patch so that it applies on current master and changed the default hash value from -1 to 0.

Rich, we still need initialization since all the record ctors delegate to the ctor arity with explicit __hash and __hasheq, following the approach of the alt ctors for __extmap and __meta

Comment by Alex Miller [ 17/Jul/15 10:30 PM ]

Moving back to vetted for screening

Comment by Alex Miller [ 28/Jul/15 6:17 PM ]

Hey Nicola, two comments on the hasheq/hashcode impl:

1) I don't think there's any reason to use == in the check instead of =, and = seems better (I think the resulting bytecode is same either way though).

2) The generated bytecode in these cases will call getfield twice in the cached case (once for the check, and once for the return):

public int hasheq();
    Code:
       0: lconst_0      
       1: aload_0       
       2: getfield      #232                // Field __hasheq:I     ;; <-- HERE
       5: i2l           
       6: lcmp          
       7: ifne          36
      10: ldc2_w        #263                // long -989260517l
      13: aload_0       
      14: checkcast     #16                 // class clojure/lang/IPersistentMap
      17: invokestatic  #270                // Method clojure/lang/APersistentMap.mapHasheq:(Lclojure/lang/IPersistentMap;)I
      20: i2l           
      21: lxor          
      22: invokestatic  #274                // Method clojure/lang/RT.intCast:(J)I
      25: istore_1      
      26: aload_0       
      27: iload_1       
      28: putfield      #232                // Field __hasheq:I
      31: iload_1       
      32: goto          40
      35: pop           
      36: aload_0       
      37: getfield      #232                // Field __hasheq:I   ;; <-- HERE
      40: ireturn

Letting a local will avoid that:

`(hasheq [this#] (let [hv# ~'__hasheq]     ;; ADDED
                                  (if (= 0 hv#)           ;; USED
                                    (let [h# (int (bit-xor ~type-hash (clojure.lang.APersistentMap/mapHasheq this#)))]
                                      (set! ~'__hasheq h#)
                                      h#)
                                    hv#)))                ;; USED

Output bytecode:

public int hasheq();
    Code:
       0: aload_0       
       1: getfield      #227                // Field __hasheq:I
       4: istore_1      
       5: lconst_0      
       6: iload_1       
       7: i2l           
       8: lcmp          
       9: ifne          38
      12: ldc2_w        #258                // long -989260517l
      15: aload_0       
      16: checkcast     #16                 // class clojure/lang/IPersistentMap
      19: invokestatic  #265                // Method clojure/lang/APersistentMap.mapHasheq:(Lclojure/lang/IPersistentMap;)I
      22: i2l           
      23: lxor          
      24: invokestatic  #269                // Method clojure/lang/RT.intCast:(J)I
      27: istore_2      
      28: aload_0       
      29: iload_2       
      30: putfield      #227                // Field __hasheq:I
      33: iload_2       
      34: goto          39
      37: pop           
      38: iload_1       
      39: ireturn

For me, this was about 2% faster in bench too.

Comment by Alex Miller [ 28/Jul/15 6:18 PM ]

Equivalent change in hashCode too.

Comment by Nicola Mometto [ 29/Jul/15 4:06 AM ]

Updated patch takes into account Alex's last notes





[CLJ-1586] Compiler doesn't preserve metadata for LazySeq literals Created: 12/Nov/14  Updated: 31/Jul/15

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

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

Attachments: Text File 0001-CLJ-1586-Compiler-doesn-t-preserve-metadata-for-lazy.patch     Text File 0001-Compiler-doesn-t-preserve-metadata-for-lazyseq-liter.patch    
Patch: Code and Test
Approval: Vetted

 Description   

The analyzer in Compiler.java forces evaluation of lazyseq literals, but loses the compile time original metadata of that form, meaning that a type hint will be lost.

Example demonstrating this issue:

user=> (set! *warn-on-reflection* true)
true
user=> (list '.substring (with-meta (concat '(identity) '("foo")) {:tag 'String}) 0)
(.substring (identity "foo") 0)
user=> (eval (list '.substring (with-meta (concat '(identity) '("foo")) {:tag 'String}) 0))
Reflection warning, NO_SOURCE_PATH:6:1 - call to method substring on java.lang.Object can't be resolved (no such method).
"foo"

Forcing the concat call to an ASeq rather than a LazySeq fixes this issue:

user=> (eval (list '.hashCode (with-meta (seq (concat '(identity) '("foo"))) {:tag 'String})))
101574

This ticket blocks http://dev.clojure.org/jira/browse/CLJ-1444 since clojure.core/sequence might return a lazyseq.

This bug affected both tools.analyzer and tools.reader and forced me to commit a fix in tools.reader to work around this issue, see: http://dev.clojure.org/jira/browse/TANAL-99

The proposed patch trivially preserves the form metadata after realizing the lazyseq

Approach: Keep a copy of the original form, and apply its metadata to the realized lazyseq
Patch: 0001-CLJ-1586-Compiler-doesn-t-preserve-metadata-for-lazy.patch
Screened by:



 Comments   
Comment by Stuart Halloway [ 31/Jul/15 9:19 AM ]

Please add a test case.

Comment by Nicola Mometto [ 31/Jul/15 9:32 AM ]

Updated patch with testcase





[CLJ-1766] Serializing+deserializing lists breaks their hash Created: 21/Jun/15  Updated: 31/Jul/15

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

Type: Defect Priority: Major
Reporter: Chris Vermilion Assignee: Andrew Rosa
Resolution: Unresolved Votes: 0
Labels: interop
Environment:

Tested on OS X 10.10.3, Clojure 1.6.0, Java 1.8.0_45-b14, but don't think this is a factor.


Attachments: File serialization_test_mod.diff    
Approval: Vetted

 Description   

clojure.lang.PersistentList implements Serializable but a serialization roundtrip results in a hash of 0. This appears to be caused by ASeq marking its _hash property as transient; other collection base classes don't do this. I don't know if there is a rational for the distinction, but the result is that serializing, then deserializing, a list results in its _hash property being 0 instead of either its previous, calculated value, or -1, which would trigger recalculation.

This means that any code that relies on a list's hash value can break in subtle ways.

Examples:

(import '[java.io ByteArrayInputStream ByteArrayOutputStream ObjectInputStream ObjectOutputStream])

(defn obj->bytes [obj]
  (let [bytestream (ByteArrayOutputStream.)]
    (doto (ObjectOutputStream. bytestream) (.writeObject obj))
    (.toByteArray bytestream)))

(defn bytes->obj [bs]
  (.readObject (ObjectInputStream. (ByteArrayInputStream. bs)))

(defn round-trip [x] (bytes->obj (obj->bytes x)))
user=> (hash '(1))
-1381383523
user=> (hash (round-trip '(1)))
0
user=> #{'(1) (round-trip '(1))}
#{(1) (1)}
user=> (def m {'(1) 1})
#'user/m
user=> (= m (round-trip m))
true
user=> (= (round-trip m) m)
false

Approach: Use 0 as the "uncomputed" hash value, not -1. The auto initialized value on serialization construction will then automatically trigger a re-computation.

Alternate approaches:

  • Implement a readObject method that sets the _hash property to -1. This is somewhat complicated in the face of subclasses, etc.

Note: Also need to consider other classes that use -1 instead of 0 as the uncomputed hash value: APersistentMap, APersistentSet, APersistentVector, PersistentQueue - however I believe in those cases they are not transient and thus may avoid the issue. Perhaps they should be transient though.



 Comments   
Comment by Chris Vermilion [ 21/Jun/15 1:10 PM ]

Yikes, sorry about the formatting above; JIRA newbie and looks like I can't edit. Also, I should have noted that the above functions require an import: (import '[java.io ByteArrayInputStream ByteArrayOutputStream ObjectInputStream ObjectOutputStream]).

Comment by Alex Miller [ 22/Jun/15 7:55 AM ]

Yes, this is a bug. My preference would be to use 0 to indicate "not computed" and thus sidestep this issue. The downside of changing from -1 to 0 is that serialization/deserialization is then broken between versions of Clojure (although maybe if it was already broken, that's not an issue). -1 is used like this for lazily computed hashes in many classes in Clojure so the issue should really be fixed across the board.

Comment by Alex Miller [ 22/Jun/15 8:22 AM ]

There are some serialization round-trip tests in clojure.test-clojure.serialization - seems like those should be updated to compare the .hashCode and hash before/after, which should catch this. I attached a diff (not a proper patch) with that change just as a demonstration, which indeed produces a bunch of failures. That should be incorporated into any fix, possibly along with other failures.

Comment by Alex Miller [ 31/Jul/15 9:04 AM ]

Also, don't worry about crediting me on that test change, please just incorporate it into whatever gets made here.

Comment by Andrew Rosa [ 31/Jul/15 9:31 AM ]

Alex Miller I hope to work on this issue this weekend. There are some conflict with the alpha release schedule?

Comment by Alex Miller [ 31/Jul/15 9:47 AM ]

No conflict - when it's ready we'll look at it.





[CLJ-1232] Functions with non-qualified return type hints force import of hinted classes when called from other namespace Created: 18/Jul/13  Updated: 31/Jul/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-auto-qualify-arglists-class-names-v4.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-v4.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.

Comment by Stuart Halloway [ 10/Jul/15 12:15 PM ]

Opened https://github.com/Netflix/Hystrix/issues/831 to see what is up with hystrix.

Comment by Stuart Halloway [ 31/Jul/15 9:16 AM ]

Can somebody with context update this patch to apply to latest master?

Comment by Nicola Mometto [ 31/Jul/15 9:37 AM ]

patch updated





[CLJ-1609] Fix an edge case in the Reflector's search for a public method declaration Created: 05/Dec/14  Updated: 31/Jul/15

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

Type: Defect Priority: Major
Reporter: Jeremy Heiler Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: interop

Attachments: Text File reflector_method_bug.patch    
Patch: Code
Approval: Vetted

 Description   

The Reflector was not taking into account that a non-public class can implement an interface, and have a non-public parent class contain an implementation of a method on that interface.

The solution I took is to pass in the target object's class instead of the declaring class of the method object.

I've outlined a small example here: https://github.com/jeremyheiler/clj-reflector-bug

The repo contains a Java example that works, and the same example in Clojure that doesn't work. It also includes a patched version of Clojure 1.6.0, and shows that the patch solves the issue. Also, in `src/foo/m.clj`, there is a real example of this bug occurring by using the Java Debug Interface API in the tools.jar library.

I would have added tests to the patch, but I don't think the test runner compiles test Java code, which is required.

Approach: pass the target objects class instead of the declaring class of the method object
Patch: reflector_method_bug.patch
Screened by:



 Comments   
Comment by Alex Miller [ 05/Dec/14 8:48 AM ]

Probably a dupe of CLJ-259. I'll probably dupe that one to this though.

Comment by Jeremy Heiler [ 05/Dec/14 1:33 PM ]

Thanks. Sorry for not finding that myself. CLJ-259 refers to CLJ-126, which I think is covered with this patch.

Comment by Jeremy Heiler [ 06/Dec/14 12:37 PM ]

I was looking into whether or not the target could be null, and it can be when invoking a static method. However, I don't think that code path would make it to the target.getClass() because non-public static methods aren't returned by getMethods().

Comment by Stuart Halloway [ 31/Jul/15 9:24 AM ]

Jeremy,

The compile-tests phase of the build provides a point for compiling Java classes that that tests can then consume. Does that provide the hook you would need to add a test?





[CLJ-1527] Clarify and align valid symbol and keyword rules for Clojure (and edn) Created: 18/Sep/14  Updated: 31/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 11
Labels: reader

Approval: Triaged

 Description   

Known areas of under-specificity (http://clojure.org/reader#The%20Reader--Reader%20forms):

  • symbols (and keywords) description do not mention constituent characters that are currently in use by Clojure functions such as <, >, =, $ (for Java inner classes), & (&form and &env in macros), % (stated to be valid in edn spec)
  • keywords currently accept leading numeric characters which is at odds with the spec - see CLJ-1286

References:



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

The Clojure reader documentation also does not mention the following symbols as valid constituent characters. They are all mentioned as valid symbol constituent characters in the EDN readme here: https://github.com/edn-format/edn#symbols

dollar sign - used in Clojure/JVM to separate Java subclass names from class names, e.g. java.util.Map$Entry
percent sign - not sure why this is part of edn spec. In Clojure it seems only to be used inside #() for args like % %1 %&
ampersand - like in &form and &env in macro definitions
equals - clojure.core/= and many others
less-than - clojure.core/< clojure.core/<=
greater-than - clojure.core/> clojure.core/>=

I don't know whether Clojure and edn specs should be the same in this regard, but it seemed worth mentioning for this ticket.

Comment by Andy Fingerhut [ 01/Jun/15 12:22 AM ]

Alex, Rich made this comment on CLJ-17 in 2011: "Runtime validation off the table for perf reasons. cemerick's suggestion that arbitrary symbol support will render them valid is sound, but arbitrary symbol support is a different ticket/idea." I am not aware of any tickets that propose the enhancement of allowing arbitrary symbols to be supported by Clojure, e.g. via a syntax like

#|white space and arbitrary #$@)$~))@ chars here|

Do you think it is reasonable to create an enhancement ticket for supporting arbitrary characters in symbols and keywords?

Comment by Alex Miller [ 01/Jun/15 6:36 AM ]

Sure. I looked into this a bit as a digression off of feature expressions and #| has been reserved for this potential use. However, there are many tricky issues with it and I do not expect this to happen soon - more likely to be something we're pushed to do when necessary for some other reason.

Comment by Herwig Hochleitner [ 01/Jun/15 8:46 AM ]

Wrong ticket, but to anybody thinking about #|arbitrary symbols (or strings)|, please do consider making the delimiters configurable, as in mime multipart.

Comment by Andy Fingerhut [ 01/Jun/15 8:54 AM ]

I've created a design page for now. I'm sure it does not list many of the tricky issues you have found. I'd be happy to take a shot at documenting them if you have any notes you are willing to share.

http://dev.clojure.org/pages/viewpage.action?pageId=11862058

Comment by Andy Fingerhut [ 01/Jun/15 9:01 AM ]

Herwig, can you edit the design page linked in my previous comment, to add a reference or example to precisely how mime multipart allows delimiters to be configurable, and why you believe fixed delimeters would be a bad idea?

Comment by Herwig Hochleitner [ 01/Jun/15 9:46 AM ]

I've commented on the design page.

Comment by Alex Miller [ 13/Jul/15 12:44 PM ]

Removed a couple of issues that have been clarified on the reader page and are no longer issues.

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

Related to CLJ-1530

Comment by Adam Frey [ 15/Jul/15 11:55 AM ]

Related to this: The Clojure reader will not accept symbols and keywords that contain consecutive colons (See LispReader.java), although that is permitted by the current EDN spec. Here is a GitHub issue regarding consecutive colons. I would like to qualify why consecutive colons are disallowed, and sync up the Clojure Reader and the EDN spec on this.

Comment by Herwig Hochleitner [ 31/Jul/15 8:03 AM ]

The updated reader spec says that a symbol can contain a single / to separate the namespace. It also mentions a bare / to be the division function.
So what about clojure.core//? That still got to be a readable symbol right? So is that an exception to the 'single /' rule?
Will foo.bar// also be readable? What about foo//bar?





[CLJ-1610] Unrolled small maps Created: 08/Dec/14  Updated: 31/Jul/15

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

Type: Enhancement Priority: Critical
Reporter: Alex Miller Assignee: Zach Tellman
Resolution: Unresolved Votes: 6
Labels: collections

Approval: Vetted

 Description   

Placeholder for unrolled small maps enhancement (companion for vectors at CLJ-1517).



 Comments   
Comment by Andy Fingerhut [ 09/Jul/15 10:59 PM ]

Is there an expectation that these would perform better that PersistentArrayMap?

Comment by Zach Tellman [ 09/Jul/15 11:53 PM ]

Yes, in some cases significantly so, for three reasons (in rough order of importance):

  • positional constructors, without any need for array instantiation/population
  • short-circuiting equality checks using hash comparisons
  • no iteration on any operation

There are a series of benchmarks at https://github.com/ztellman/cambrian-collections/blob/master/test/cambrian_collections/map_test.clj#L64-L148, which compare operations against maps with both keywords (which don't benefit from the hash comparisons) and symbols (which do). The 7-entry map cases cause the unrolled maps to overflow, so they only exist to test the overflow mechanism.

I've run the benchmark suite on my laptop, and the results are at https://gist.github.com/ztellman/961001e1a77e4f76ee1d. Some notable results:

The rest of the benchmarks are marginally faster due to unrolling, but most of the performance benefits are from the above behaviors. In a less synthetic benchmark, I found that Cheshire JSON decoding (which is 33% JSON lexing and 66% data structure building) was sped up roughly 30-40%.





[CLJ-1453] Most Iterator implementations do not correctly implement next failing to throw the required NoSuchElementException Created: 24/Jun/14  Updated: 30/Jul/15

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

Type: Defect Priority: Major
Reporter: Meikel Brandmeyer Assignee: Andrew Rosa
Resolution: Unresolved Votes: 3
Labels: interop

Attachments: Text File 0001-Fix-iterator-implementations-to-throw-NSEE-when-exha.patch     Text File 0001-Throw-NSEE-in-gvec-iterator.patch     Text File clj-1453-2.patch     Text File CLJ-1453.patch     Text File CLJ-1453-tests.patch    
Patch: Code and Test
Approval: Screened

 Description   

Iterators on Clojure's collections should follow the expected JDK behavior of throwing NoSuchElementException on next() when an iterator is exhausted. Current collections have a variety of other behaviors.

Issue encountered in real world code using http://pipes.tinkerpop.com.

To reproduce:

(-> [] .iterator .next)

This throws a NPE instead of NSEE.

(doto (.iterator [1 2]) .next .next .next)

This throws an ArrayIndexOutOfBoundsException instead of NSEE.

The attached patch fixes the methods by adding a check for hasNext before actually trying to provide the next element. If there is no next element the correct exception is thrown.

Patch: clj-1453-2.patch and tests: CLJ-1453-tests.patch

Screened by: Alex Miller



 Comments   
Comment by Tim McCormack [ 15/Jul/14 9:56 PM ]

To establish a baseline, this piece of code checks all the iterators I could find within Clojure 1.6.0 and makes sure they throw an appropriate exception:

iterator-checker.clj
(defn next-failure
  []
  (let [ok (atom [])]
    (doseq [[tp v]
            (sorted-map
             :vec-0 []
             :vec-n [1 2 3]
             :vec-start (subvec [1 2 3 4] 0 1)
             :vec-end (subvec [1 2 3 4] 3 4)
             :vec-ls-0 (.listIterator [])
             :vec-ls-n (.listIterator [1 2 3])
             :vec-start-ls (.listIterator (subvec [1 2 3 4] 0 1))
             :vec-end-ls (.listIterator (subvec [1 2 3 4] 3 4))
             :seq ()
             :list-n '(1 2 3)
             :set-hash-0 (hash-set)
             :set-hash-n (hash-set 1 2 3)
             :set-sor-0 (sorted-set)
             :set-sor-n (sorted-set 1 2 3)
             :map-arr-0 (array-map)
             :map-arr-n (array-map 1 2, 3 4)
             :map-hash-0 (hash-map)
             :map-hash-n (hash-map 1 2, 3 4)
             :map-sor-n (sorted-map)
             :map-sor-n (sorted-map 1 2, 3 4)
             :map-sor-ks-0 (.keys (sorted-map))
             :map-sor-ks-n (.keys (sorted-map 1 2, 3 4))
             :map-sor-vs-0 (.vals (sorted-map))
             :map-sor-vs-n (.vals (sorted-map 1 2, 3 4))
             :map-sor-rev-0 (.reverseIterator (sorted-map))
             :map-sor-rev-n (.reverseIterator (sorted-map 1 2, 3 4))
             :map-ks-0 (.keySet {})
             :map-ks-n (.keySet {1 2, 3 4})
             :map-vs-0 (.values {})
             :map-vs-n (.values {1 2, 3 4})
             :gvec-int-0 (vector-of :long)
             :gvec-int-n (vector-of :long 1 2 3))]
      (let [it (if (instance? java.util.Iterator v)
                 v
                 (.iterator v))]
        (when-not it
          (println "Null iterator:" tp))
        (try (dotimes [_ 50]
               (.next it))
             (catch java.util.NoSuchElementException nsee
               (swap! ok conj tp))
             (catch Throwable t
               (println tp "threw" (class t))))))
    (println "OK:" @ok)))

The output as of current Clojure master (201a0dd970) is:

:gvec-int-0 threw java.lang.IndexOutOfBoundsException
:gvec-int-n threw java.lang.IndexOutOfBoundsException
:map-arr-0 threw java.lang.ArrayIndexOutOfBoundsException
:map-arr-n threw java.lang.ArrayIndexOutOfBoundsException
:map-hash-0 threw java.lang.ArrayIndexOutOfBoundsException
:map-ks-0 threw java.lang.ArrayIndexOutOfBoundsException
:map-ks-n threw java.lang.ArrayIndexOutOfBoundsException
:map-sor-ks-0 threw java.util.EmptyStackException
:map-sor-ks-n threw java.util.EmptyStackException
:map-sor-n threw java.util.EmptyStackException
:map-sor-rev-0 threw java.util.EmptyStackException
:map-sor-rev-n threw java.util.EmptyStackException
:map-sor-vs-0 threw java.util.EmptyStackException
:map-sor-vs-n threw java.util.EmptyStackException
:map-vs-0 threw java.lang.ArrayIndexOutOfBoundsException
:map-vs-n threw java.lang.ArrayIndexOutOfBoundsException
:vec-0 threw java.lang.NullPointerException
:vec-end threw java.lang.ArrayIndexOutOfBoundsException
:vec-end-ls threw java.lang.IndexOutOfBoundsException
:vec-ls-0 threw java.lang.IndexOutOfBoundsException
:vec-ls-n threw java.lang.IndexOutOfBoundsException
:vec-n threw java.lang.ArrayIndexOutOfBoundsException
:vec-start threw java.lang.ArrayIndexOutOfBoundsException
:vec-start-ls threw java.lang.IndexOutOfBoundsException
OK: [:list-n :map-hash-n :seq :set-hash-0 :set-hash-n :set-sor-0 :set-sor-n]
Comment by Tim McCormack [ 15/Jul/14 9:57 PM ]

0001-Fix-iterator-implementations-to-throw-NSEE-when-exha.patch missed one thing: clojure.gvec. With the patch in place, my checker outputs the following:

:gvec-int-0 threw java.lang.IndexOutOfBoundsException
:gvec-int-n threw java.lang.IndexOutOfBoundsException
OK: [:list-n :map-arr-0 :map-arr-n :map-hash-0 :map-hash-n :map-ks-0 :map-ks-n :map-sor-ks-0 :map-sor-ks-n :map-sor-n :map-sor-rev-0 :map-sor-rev-n :map-sor-vs-0 :map-sor-vs-n :map-vs-0 :map-vs-n :seq :set-hash-0 :set-hash-n :set-sor-0 :set-sor-n :vec-0 :vec-end :vec-end-ls :vec-ls-0 :vec-ls-n :vec-n :vec-start :vec-start-ls]

That should be a quick fix.

Comment by Michał Marczyk [ 15/Jul/14 10:01 PM ]

CLJ-1416 includes a fix for gvec's iterator impls (and some other improvements to interop).

Comment by Tim McCormack [ 15/Jul/14 10:17 PM ]

Attaching a fix for the gvec iterator. Combined with the existing patch, this fixes all broken iterators that I could find.

Comment by Andy Fingerhut [ 07/Aug/14 10:25 AM ]

I believe this Clojure commit: https://github.com/clojure/clojure/commit/e7215ce82215bda33f4f0887cb88570776d558a0

introduces more implementations of the java.util.Iterator interface where next() returns null instead of throwing a NoSuchElementException

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

Would love to have a patch that:
1) combined patches to date
2) updated to master
3) reviewed for new iterators since this ticket was written
4) added the tests in the comments

Comment by Alex Miller [ 18/Jun/15 3:30 PM ]

Bump - would be happy to screen this for 1.8 if my last comments were addressed.

Comment by Andrew Rosa [ 18/Jul/15 4:38 PM ]

Alex Miller Addressed you comments on the first patch. On the process of applying them to master, I've changed the code to follow a single "format" of bound checking and throw, so everything will be more consistent with the code style of Clojure codebase. The commit themselves still maintain the original authors, to give correct credit.

The second patch includes only the tests for these features, which I used test.check to do them, as I talked to you on clojure-dev channel. If you think they are too much complex or prefer a different style of testing, please let me know - that's why I made a separate patch only for the tests.

Comment by Alex Miller [ 18/Jul/15 5:04 PM ]

Thanks Andrew. It will likely be a couple weeks before I have time to look at this.

Comment by Alex Miller [ 30/Jul/15 11:02 PM ]

clj-1453-2.patch squashes CLJ-1453.patch and updates to current master.





[CLJ-1077] thread-bound? returns true (implying set! should succeed) even for non-binding thread Created: 26/Sep/12  Updated: 30/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Paul Stadig Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None

Attachments: File thread-bound.diff    
Patch: Code
Approval: Triaged

 Description   

thread-bound? returns true for a non-binding thread, this result (according to the docstring) implies that set! should succeed. However, thread-bound? does not check that any binding that might exist was created by the current thread, and calling set! fails with an exception when it is called from a non-binding thread, even though thread-bound? returns true.

thread-bound? should return false if there is a binding, and that binding was not established by the current thread.

Here is an example REPL session where a thread establishes a binding, those bindings are conveyed to a second thread, the second thread checks thread-bound? to see if it can set the binding, thread-bound? returns true indicating that the binding can be set, the second thread tries to set the binding, and the second thread gets an IllegalStateException:

    Clojure 1.5.1
    user=> (def ^:dynamic *set-me* nil)
    #'user/*set-me*
    user=> (defn try-to-set [] (binding [*set-me* 1] (doall (pcalls #(if (thread-bound? #'*set-me*) (set! *set-me* (inc *set-me*)))))))
    #'user/try-to-set
    user=> (try-to-set)
    IllegalStateException Can't set!: *set-me* from non-binding thread  clojure.lang.Var.set (Var.java:230)
    user=> 


 Comments   
Comment by Paul Stadig [ 01/Oct/12 10:07 AM ]

I have attached a patch that changes clojure.lang.Var and clojure.core/thread-bound? to only return true if a Var is set!-able.

Comment by Alex Miller [ 19/Aug/13 12:16 PM ]

REPL example?

Comment by Joe Gallo [ 20/Aug/13 7:55 AM ]

Sure thing, Alex – here's a repl example I just ran this morning.

; nREPL 0.1.7
user> (def ^:dynamic *set-me* nil)
#'user/*set-me*
user> (defn try-to-set [] (binding [*set-me* 1] (doall (pcalls #(if (thread-bound? #'*set-me*) (set! *set-me* (inc *set-me*)))))))
#'user/try-to-set
user> (try-to-set)
IllegalStateException Can't set!: *set-me* from non-binding thread  clojure.lang.Var.set (Var.java:230)
user>




[CLJ-1381] Improve support for extending protocols to primitive arrays Created: 13/Mar/14  Updated: 30/Jul/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 Miller Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: protocols

Approval: Triaged

 Description   

It is possible to extend protocols to primitive arrays but specifying the class for the type is a little tricky:

(defprotocol P (p [_]))
(extend-protocol P (Class/forName "[B") (p [_] "bytes"))
(p (byte-array 0))   ;; => "bytes"

However, things go bad if you try to do more than one of these:

(extend-protocol P 
  (Class/forName "[B") (p [_] "bytes")
  (Class/forName "[I") (p [_] "ints"))
CompilerException java.lang.UnsupportedOperationException: nth not supported on this type: Character, compiling:(NO_SOURCE_PATH:1:1)
	clojure.lang.Compiler.analyze (Compiler.java:6380)
	clojure.lang.Compiler.analyze (Compiler.java:6322)
	clojure.lang.Compiler$MapExpr.parse (Compiler.java:2879)
	clojure.lang.Compiler.analyze (Compiler.java:6369)
	clojure.lang.Compiler.analyze (Compiler.java:6322)
	clojure.lang.Compiler$InvokeExpr.parse (Compiler.java:3624)
	clojure.lang.Compiler.analyzeSeq (Compiler.java:6562)
	clojure.lang.Compiler.analyze (Compiler.java:6361)
	clojure.lang.Compiler.analyze (Compiler.java:6322)
	clojure.lang.Compiler$BodyExpr$Parser.parse (Compiler.java:5708)
	clojure.lang.Compiler$FnMethod.parse (Compiler.java:5139)
	clojure.lang.Compiler$FnExpr.parse (Compiler.java:3751)
Caused by:
UnsupportedOperationException nth not supported on this type: Character
	clojure.lang.RT.nthFrom (RT.java:857)
	clojure.lang.RT.nth (RT.java:807)
	clojure.core/emit-hinted-impl/hint--5951/fn--5953 (core_deftype.clj:758)
	clojure.core/map/fn--4207 (core.clj:2487)
	clojure.lang.LazySeq.sval (LazySeq.java:42)
	clojure.lang.LazySeq.seq (LazySeq.java:60)
	clojure.lang.RT.seq (RT.java:484)
	clojure.lang.RT.countFrom (RT.java:537)
	clojure.lang.RT.count (RT.java:530)
	clojure.lang.Cons.count (Cons.java:49)
	clojure.lang.Compiler.analyze (Compiler.java:6352)
	clojure.lang.Compiler.analyze (Compiler.java:6322)

The code in {parse-impls} is seeing the second {(Class/forName "[I")} as a function, not as a new type. One workaround for this is to only extend the protocol to one type at a time.

It would be even better (moving into enhancement area) if there was a syntax here to specify primitive array types - we already have the syntax of {bytes, ints, longs}, etc for type hints and those seem perfectly good to me.



 Comments   
Comment by Nahuel Greco [ 18/Sep/14 6:08 PM ]

It also breaks when extending only one array type:

(extend-protocol P
  String               (p [_] "string")
  (Class/forName "[B") (p [_] "ints") 
  )

;=> CompilerException java.lang.UnsupportedOperationException: nth not supported on this type ...

But changing the declaration order fixes it:

(extend-protocol P
  (Class/forName "[B") (p [_] "ints") 
  String               (p [_] "string")
  )

;=> OK




[CLJ-1778] let-bound namespace-qualified bindings should throw (if not map destructuring) Created: 16/Jul/15  Updated: 30/Jul/15

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Ragnar Dahlen
Resolution: Unresolved Votes: 0
Labels: compiler

Attachments: Text File clj-1778-2-with-tests.patch     Text File clj-1778.patch    
Patch: Code and Test
Approval: Vetted

 Description   

Seen in a tweet...

user=> (let [a/x 42] x) ; throws CompilerException "Can't let qualified name ..."
user=> (let [a/x 42, [y] [1]] x) ;=> 42

The second one should throw like the first one.

Presume linkage to CLJ-1318.

Current patch: clj-1778-2-with-tests.patch



 Comments   
Comment by Ragnar Dahlen [ 16/Jul/15 11:41 AM ]

I can confirm that this is a regression from CLJ-1318. After that change, namespaces were removed from symbols regardless of whether they were used as part of a map destruring or not. The only reason the exception is caught in the first test case is because that binding form has no destructuring at all, in which case the destructuring logic is bypassed.

I've attached a patch that moves the namespace removal (and keyword handling) into `pmap` instead as it's really a special case for map destructuring only.

Comment by Ragnar Dahlen [ 16/Jul/15 3:53 PM ]

Updated patch with two changes:

1. Removed initial check for keywords in binding keys as that only looked for keywords at top-level and failed to catch cases like:

(let [[:x] [1]] x)
;; => 1

Keywords in non-map destructuring binding positions (like the example) will now fail with "Unsupported binding form: :x" instead. This is a change from the current behaviour where only top-level keywords would be caught, but the current exception is the slightly more specific "Unsupported binding key: :x". If a better, more specific exception is required, I'd be happy to update the patch.

2. Tests: added test for regression, updated/amended existing tests.





[CLJ-1613] :or defaults should refer to enclosing scope in map destructuring Created: 12/Dec/14  Updated: 30/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Michał Marczyk Assignee: Michał Marczyk
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File 0001-CLJ-1613-evaluate-or-defaults-in-enclosing-scope-in-.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Michael Blume noticed that :or defaults can depend on the values of other keys, see https://groups.google.com/d/msg/clojure/6kOhpPOpHWM/ITjWwQFS_VQJ

Michael's Gist https://gist.github.com/MichaelBlume/4891dafdd31f0dcbc727 displays a case where an associative form involving :keys and :or compiles or not depending on the order of symbols in :keys. By tweaking that case one can arrive at expressions which always compile, but produce different values depending on :keys:

(let [foo 1
       bar 2
       {:keys [bar foo]
        :or {foo 3 bar (inc foo)}} {}]
  {:foo foo :bar bar})
;= {:foo 3, :bar 4}

(let [foo 1
      bar 2
      {:keys [foo bar]
       :or {foo 3 bar (inc foo)}} {}]
  {:foo foo :bar bar})
;= {:foo 3, :bar 2}

I believe that the most natural solution is to demand that :or defaults be evaluated in an enclosing scope where none of the destructuring-introduced locals are present. This approach is taken by the 0001 patch.



 Comments   
Comment by Michael Blume [ 12/Dec/14 2:27 AM ]

I suspect that this is the right thing to do but I think it's important to note that this will break existing code https://github.com/ngrunwald/ring-middleware-format/blob/master/src/ring/middleware/format_params.clj#L214

Comment by Michael Blume [ 06/Apr/15 4:43 PM ]

Update on my previous comment – ring-middleware-params has updated so that it doesn't depend on this behavior. I think we should definitely merge this patch so no one else depends on it.

Comment by Max Penet [ 08/Apr/15 10:46 AM ]

Since this involves :or keys evaluation, this might be worth checking if this should/could have an impact on http://dev.clojure.org/jira/browse/CLJ-1676 as well.

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

This is a behavior change, the docs do not promise the requested behavior and existing code may depend on the current behavior.

Comment by Andy Fingerhut [ 30/Jul/15 12:47 PM ]

Isn't this a case where if existing code works, it works by accident of the seq order of an unordered map? If so, any code that depends upon the existing behavior sometimes breaks, sometimes does not break, when the Clojure seq order on maps changes, which occurred Clojure 1.5.1 to Clojure 1.6.0, and again from 1.6.0 to 1.7.0.

Comment by Michael Blume [ 30/Jul/15 1:08 PM ]

Yes, it does, and I've seen existing code break due to those changes, hence the discussion that lead to this ticket.





[CLJ-1750] There should be a way to observe platform features at runtime Created: 08/Jun/15  Updated: 30/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Luke VanderHart Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reader

Approval: Triaged

 Description   

Reader conditionals let the reader emit code conditionally based upon a set of platform features.

This is a closed set - however, currently it is baked in as an implementation detail of the reader. Runtime code cannot access the current platform feature set.

This is problematic when writing a macro that needs to emit code conditionally based upon the platform of the code being compiled. Reader conditionals themselves won't work since macros are always themselves read in Clojure.

We should enable some mechanism for retrieving the current platform at runtime, or at least at macro expansion time.

For example, this is the kind of thing it should be possible to do:

(defmacro mymacro []
    (if (*platforms* :clj)
      `(some-clojure-thing)
      `(some-cljs-thing)))


 Comments   
Comment by Micah Martin [ 19/Jun/15 1:46 PM ]

+1 - Would very much like to see this in 1.7. Currently I have to use an ugly hack.

(def ^:private ^:no-doc cljs? (boolean (find-ns 'cljs.analyzer)))





[CLJ-1242] get/= on sorted collections when types don't match result in a ClassCastException Created: 31/Jul/13  Updated: 30/Jul/15

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

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: 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

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

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

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





[CLJ-1780] Test OOME from locals clearing change Created: 17/Jul/15  Updated: 30/Jul/15

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

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

1.8.0-alpha2


Attachments: Text File strengthen-clearing-test.patch    
Approval: Triaged

 Description   

IBM JDK 1.6 in test matrix is throwing errors from the new test for CLJ-1250 in 1.8.0-alpha2.

[java] ERROR in (test-closed-over-clearing) (Range.java:133)
     [java] expected: (number? (reduce + 0 (r/map identity (range 1.0E8))))
     [java]   actual: java.lang.OutOfMemoryError: null
     [java]  at clojure.lang.Range.forceChunk (Range.java:133)
     [java]     clojure.lang.Range.chunkedFirst (Range.java:150)
     [java]     clojure.core$chunk_first.invoke (core.clj:667)
     [java]     clojure.core.protocols/fn (protocols.clj:136)
     [java]     clojure.core.protocols$fn__6478$G__6473__6487.invoke (protocols.clj:19)
     [java]     clojure.core.protocols$seq_reduce.invoke (protocols.clj:31)
     [java]     clojure.core.protocols/fn (protocols.clj:95)
     [java]     clojure.core.protocols$fn__6452$G__6447__6465.invoke (protocols.clj:13)
     [java]     clojure.core.reducers$folder$reify__21452.coll_reduce (reducers.clj:126)
     [java]     clojure.core$reduce.invoke (core.clj:6519)
     [java]     clojure.test_clojure.reducers/fn (reducers.clj:95)
     [java]     clojure.test$test_var$fn__7669.invoke (test.clj:703)
     [java]     clojure.test$test_var.invoke (test.clj:703)
     [java]     clojure.test$test_vars$fn__7691$fn__7696.invoke (test.clj:721)
     [java]     clojure.test$default_fixture.invoke (test.clj:673)
     [java]     clojure.test$test_vars$fn__7691.invoke (test.clj:721)
     [java]     clojure.test$default_fixture.invoke (test.clj:673)
     [java]     clojure.test$test_vars.invoke (test.clj:717)
     [java]     clojure.test$test_all_vars.invoke (test.clj:727)
     [java]     clojure.test$test_ns.invoke (test.clj:746)
     [java]     clojure.core$map$fn__4553.invoke (core.clj:2624)
     [java]     clojure.lang.LazySeq.sval (LazySeq.java:40)
     [java]     clojure.lang.LazySeq.seq (LazySeq.java:49)
     [java]     clojure.lang.Cons.next (Cons.java:39)
     [java]     clojure.lang.RT.next (RT.java:681)
     [java]     clojure.core/next (core.clj:64)
     [java]     clojure.core$reduce1.invoke (core.clj:909)
     [java]     clojure.core$reduce1.invoke (core.clj:900)
     [java]     clojure.core$merge_with.doInvoke (core.clj:2936)
     [java]     clojure.lang.RestFn.applyTo (RestFn.java:139)
     [java]     clojure.core$apply.invoke (core.clj:632)
     [java]     clojure.test$run_tests.doInvoke (test.clj:761)
     [java]     clojure.lang.RestFn.applyTo (RestFn.java:137)
     [java]     clojure.core$apply.invoke (core.clj:630)
     [java]     user$eval28810.invoke (run_test.clj:8)
     [java]     clojure.lang.Compiler.eval (Compiler.java:6850)


 Comments   
Comment by Nicola Mometto [ 18/Jul/15 12:02 AM ]

I don't understand how forcing a 32 element chunk could consume the memory. If locals clearing worked properly there should be no other part of that sequence in memory but I might be missing some detail.

Might there be something going on with the recent change in impl for Range?

Comment by Ghadi Shayban [ 18/Jul/15 12:19 AM ]

An interesting note is that (reduce + 0 (r/map identity (range 1e8))) is going through the seq path, despite reducers && reducible range. This is because coll-reduce doesn't prefer IReduceInit.

The CLJ-1250 test should be modified to intentionally hold the head of a seq in order to exercise the locals clearing. A good hypothesis from Alex is that GC is a bit slower on the archaic IBM JDK.

Comment by Ghadi Shayban [ 18/Jul/15 1:21 AM ]

I can't reproduce this on Linux x86-64 with IBM JDK and an artificially tiny heap.

[ghadi@amdhex ibm-java-x86_64-60]$ ./bin/java -Xmx128m -cp $HOME/jsr166y-1.7.0.jar:$HOME/clojure-1.8.0-master-SNAPSHOT.jar clojure.main
Clojure 1.8.0-master-SNAPSHOT
user=> (require '[clojure.core.reducers :as r])
nil
user=> (number? (reduce + 0 (r/map identity (range 1e8))))
true

Can you post details on the IBM JDK environment in CI? Need point release, heap size, kernel, distro, and jvm opts.

I've strengthened the CLJ-1250 test case by relying on neither reducer impl nor range impl, and I reverified that the bug is in fact present on <1.7 and gone on -master using Oracle JDK and 128m heap.

Comment by Alex Miller [ 18/Jul/15 8:52 AM ]

OS is CentOS 5.5 afaict

JDK details:

java version "1.6.0"
Java(TM) SE Runtime Environment (build pxa6460sr9fp2-20110625_01(SR9 FP2))
IBM J9 VM (build 2.4, JRE 1.6.0 IBM J9 2.4 Linux amd64-64 jvmxa6460sr9-20110624_85526 (JIT enabled, AOT enabled)
J9VM - 20110624_085526
JIT - r9_20101028_17488ifx17
GC - 20101027_AA)
JCL - 20110530_01

As far as I can tell, nothing is being done to alter the default heap size or other jvm opts during the build.

Comment by Ghadi Shayban [ 18/Jul/15 2:27 PM ]

The IBM JDK6 version I used was:

java version "1.6.0"
Java(TM) SE Runtime Environment (build pxa6460sr16fp7-20150708_01(SR16 FP7))
IBM J9 VM (build 2.4, JRE 1.6.0 IBM J9 2.4 Linux amd64-64 jvmxa6460sr16fp7-20150701_255724 (JIT enabled, AOT enabled)
J9VM - 20150701_255724
JIT - r9_20150630_95420
GC - GA24_Java6_SR16_20150701_1008_B255724)
JCL - 20150628_01

Seems like SR16 FP7 == 6.0.16.7, and the one on the CI build is SR9 FP2 == 6.0.9.2, a four or five year difference between point releases.

Comment by Ghadi Shayban [ 18/Jul/15 2:51 PM ]

OK I found a download for the (old) IBM JDK 6.0.9.2 and installed it on Linux x86-64. I cannot reproduce the bug.

Comment by Alex Miller [ 18/Jul/15 3:53 PM ]

I'd be happy to update the ibm jdk 1.6 version and n the build box too to see what happens.





[CLJ-1472] The locking macro fails bytecode verification on ART runtime Created: 23/Jul/14  Updated: 30/Jul/15

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

Type: Defect Priority: Minor
Reporter: Adam Clements Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: None
Environment:

Android ART runtime


Attachments: Text File 0001-CLJ-1472-Locking-macro-without-explicit-monitor-ente.patch    
Patch: Code
Approval: Triaged

 Description   

Android ART runs compile time verification on bytecode and was failing on any usage of the locking macro. Examination of the bytecode as compared to a java synchronized block shows up a number of differences:
https://gist.github.com/AdamClements/2ae6c4919964b71eb470

Having the monitor-enter inside the try block seems wrong to me, as surely if the lock fails to be acquired, it shouldn't be released with monitor-exit. Moving the monitor enter outside the try block seems to have resolved the issue and android no longer complains about usages of locking and all clojure tests still pass.

Java's generated code goes further and catches any exceptions generated by the monitor-exit itself and retries indefinitely (I believe the logic is that then at least your deadlock is in the right place, and not next time something else attempts to acquire a lock on the same object). I don't think that this can be replicated in clojure without getting down to the bytecode emitting level though and it doesn't seem to be an issue for the ART verifier.



 Comments   
Comment by Adam Clements [ 24/Jul/14 11:17 AM ]

After using this a little more, I've found that moving this outside the try block breaks nREPL.

Looking at the bytecode, the monitorenter for the locking in clojure.tools.nrepl.middleware.session/session-out and in a few other places ends up in an entirely different method definition and we now get a JVM IllegalMonitorStateException as well as an ART verification error for this function.

Comment by Andy Fingerhut [ 01/Aug/14 9:08 PM ]

Adam, I cannot comment on whether your patch is of interest or not, but it is true that no patch will be committed to Clojure if the author has not signed a Contributor Agreement, which can now be done on-line at http://clojure.org/contributing

Comment by Adam Clements [ 04/Aug/14 4:24 PM ]

Uploaded a new patch (and signed the contributor agreement). This passes both the JVM and ART bytecode verification, The extra try/catch around the monitor exit is optional (verification passes with or without it) but where the java version retries monitor-exit indefinitely and shows the deadlock at the right time, without catching errors in the monitor-exit an undetermined monitor-enter in the future might fail, not showing up the actual bug.

It's not very pretty, but without finer grained control of the generated bytecode, this is the best I could do.

Comment by Adam Clements [ 25/Nov/14 8:31 AM ]

Have just tested with Lollipop, and this patch might no longer be sufficient.

Getting in touch with the ART guys to see if they can shed a little more light and verify whether it will work on the current master branch of AOSP

Comment by Adam Clements [ 25/Nov/14 9:49 AM ]

Bug filed with AOSP project, hopefully they can shed some light on whether it is our problem and if so how we can fix it.

https://code.google.com/p/android/issues/detail?id=80823

Comment by Adam Clements [ 28/Nov/14 11:03 AM ]

I have uploaded an alternative implementation of the locking macro (0001-CLJ-1472-Locking-macro-without-explicit-monitor-ente.patch) which cheats a little - the synchronized block is actually implemented in Java and so guarantees compatibility. This is at the cost of a little extra indirection and the naming/location could probably be better.

But it does fix the bug and work on all versions of android, android + art and the jvm. Would this approach be acceptable?

Comment by Kevin Downey [ 08/Dec/14 1:12 PM ]

I have yet to see any evidence that the bytecode clojure is generating in some way violates the jvm spec, so I suspect the issue is clojure requires a jvm to run, and android doesn't provide a jvm, just something that looks like one if you don't tread outside the beaten path.

Comment by Kevin Downey [ 08/Dec/14 1:27 PM ]

given the structured locking verbiage in https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-2.html#jvms-2.11.10, `(locking nil)` may generate bytecode whose runtime behavior violates structured locking. the first patch on this issue can cause the compiler to emit monitorenter/exit instructions in different methods, which definitely violates structured locking

Comment by Adam Clements [ 09/Dec/14 10:45 AM ]

Yes, the first patch was definitely wrong, I left it for some context to the conversation, but it's probably best to just remove it for clarity.

For anyone following this conversation who doesn't want to decompile and observe the bytecode, here's a gist with the difference between a java synchronized block and clojure locking https://gist.github.com/AdamClements/2ae6c4919964b71eb470

I'm finding it hard to work out where the deviation from the spec occurs too, though I can see the difference with the Java version, if anything, the Clojure version looks closer to what's described in the spec than the Java one!

If someone with more knowledge than me on the subject could engage on the AOSP bug https://code.google.com/p/android/issues/detail?id=80823 then perhaps we could settle this as an android bug which is too focussed on the java implementation rather than the JVM spec, or perhaps they'll find something that's wrong with the Clojure implementation. I have uploaded the original clojure behaviour and asked them for some more explanation on why it is failing.

Comment by Adam Clements [ 09/Dec/14 11:09 AM ]

The response from the ART guys about what they think we're violating is:

The section on "Structured locking" contains the following:

"[...] implementations [...] are permitted but not required to enforce
both of the following two rules guaranteeing structured locking. [...]"

ART currently enforces both rules at verification time, including

"At no point during a method invocation may the number of monitor exits
performed by T on M since the method invocation exceed the number of
monitor entries performed by T on M since the method invocation."

Comment by Adam Clements [ 09/Dec/14 11:32 AM ]

If for example instruction https://gist.github.com/AdamClements/2ae6c4919964b71eb470#file-test_locks-class-L24 or the monitor-enter itself on the next line were to fail, couldn't it could end up in the finally clause and attempt to release the lock even though it has never been captured?

I think this violates the structured locking rules in the jvm spec you linked to.

Comment by Kevin Downey [ 09/Dec/14 1:15 PM ]

an interesting question about structured locking, is does the specification refer to the static bytecode or the runtime behavior of the bytecode. given the bytecode linked(https://gist.github.com/AdamClements/2ae6c4919964b71eb470#file-test_locks-class-L24), the static bytecode has the same number of entries and exits, but the dynamic behavior may be different. I wonder which one the art guys claim to be enforcing at verification time (it seems like it would have to be the static bytecode, not the dynamic properties, but then they shouldn't be failing to verify this). looking at the google code issue, the comment https://code.google.com/p/android/issues/detail?id=80823#c6 was made by the same dev as https://code.google.com/p/android/issues/detail?id=80823#c3, so I sort of suspect there is some miscommunication going on. It is not clear in what context the dev is replying in, since in the previous comment you mention splitting monitor-enter and exit across methods. I think things would be much clearer if all patches, specialized clojure android builds, etc, were gotten rid of, then with a vanilla clojure jar you get a javap dump of what fails to verify, then just take that over to the android issue tracker and ask "hey, this fails to verify, why?"

Comment by Adam Clements [ 11/Dec/14 9:15 AM ]

Yeah, I shouldn't have confused it with the patched versions. The gist and the currently uploaded version use the vanilla clojure version of the locking macro now though.

I think the issue comes from the exception table and the instructions that covers. If line 24 can throw for example, you would end up at runtime with a monitor-exit, having never encountered a monitor-enter.





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

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

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

Approval: Triaged

 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.

Comment by Stuart Halloway [ 19/Jul/15 8:03 AM ]

This is really gross, and the original developer has been punched in the neck. (Ow.)

I hate the Java leakage, but given that this is already out there, and that people are likely already relying on both the default and the negative-arg behavior, I think the least bad bet is to document precisely the semantics we have.





[CLJ-1676] map destructuring: prevent evaluation of values in :or when they are not used/needed Created: 14/Mar/15  Updated: 30/Jul/15

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

Type: Enhancement Priority: Trivial
Reporter: Max Penet Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: destructuring

Approval: Triaged

 Description   

The name :or implies this should behave as "or" and be "lazy" but it's not the case currently.
The following gist shows the issue. :x is present in the map but we eval the default value:

(defn foo
  [{:keys [x]
    :or {x (println :set-default)}}] 
  x)
 
 
 
user> (foo {:x 1})
:set-default
1


 Comments   
Comment by Ghadi Shayban [ 15/Mar/15 2:40 PM ]

1.2 - current all behave this way, doesn't seem like a recent change.

Comment by Max Penet [ 15/Mar/15 2:55 PM ]

Right, I thought it might have been a regression, but wasn't sure at all.
It seems it would be safe to change the current behavior, I doubt it would break any ones code.





[CLJ-1763] clojure.core/sort is not thread-safe on Java collections with backing arrays Created: 19/Jun/15  Updated: 30/Jul/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: 0
Labels: collections

Attachments: Text File 0001-CLJ-1763-make-sort-thread-safe.patch    
Patch: Code
Approval: Triaged

 Description   

If a (mutable) Java collection that exposes it's backing array is passed to c.c/sort in multiple threads, the collection will be concurrently modified in multiple threads.

user=> (def q (java.util.concurrent.ArrayBlockingQueue. 1))
#'user/q
user=> (future (loop [] (.add q 1) (.remove q 1) (recur)))
#object[clojure.core$future_call$reify__4393 0x4769b07b {:status :pending, :val nil}]
user=> (take 3 (distinct (repeatedly #(sort q))))
((1) () nil)

Approach: Convert coll to a seq before converting it to an array, thus preserving the original collection.

Patch: 0001-CLJ-1763-make-sort-thread-safe.patch

Alternate approaches:

1. Document in sort that, like Java arrays, Java collections backed by arrays are modified in-place.
2. Change RT.toArray() to defensively copy the array returned from a (non-IPersistentCollection) Java collection. This has a number of potential ramifications as this method is called from several paths.
3. For non-Clojure collections, could also use Collections.sort() instead of dumping to array and using Arrays.sort().



 Comments   
Comment by Alex Miller [ 19/Jun/15 10:55 AM ]

The docstring says "If coll is a Java array, it will be modified. To avoid this, sort a copy of the array." which also seems like solid advice in this case.

Creating a sequence view of the input collection would significantly alter the performance characteristics.

Comment by Alex Miller [ 19/Jun/15 10:59 AM ]

I guess what I'm saying is, we should not make the performance worse for persistent collections in order to make it safer for arbitrary Java collections. But it may still be useful to make it safer without affecting persistent collection performance and/or updating the docstring.

Comment by Nicola Mometto [ 19/Jun/15 11:02 AM ]

Alex, no additional sequence is being created, the seq call was already there

Comment by Alex Miller [ 19/Jun/15 11:53 AM ]

Well, that's kind of true. The former use did not force realization of the whole seq, just the first element. That said, from a quick test the extra cost seems small on a set (vector seqs are actually faster due to their structure).

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

I think this should be a docstring change, if anything at all.





[CLJ-1790] Problem with 1.8.0-alpha3 handling of protocols with Java arrays Created: 29/Jul/15  Updated: 30/Jul/15

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

Type: Defect Priority: Major
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

1.8.0-alpha3 is breaking core.matrix at the moment.

Root cause appears to be related to how protocols are being handled when used with Java arrays:

e.g. for the protocol implementation:

(extend-protocol mp/PImplementation
  (Class/forName "[Ljava.lang.Object;")
    (implementation-key [m] :object-array)
    (meta-info [m]
      {:doc "Clojure.core.matrix implementation for Java Object arrays"})
    (new-vector [m length] (construct-object-vector (long length)))
    (new-matrix [m rows columns]
      (let [columns (long columns)
            m (object-array rows)]
        (dotimes [i rows]
          (aset m i (construct-object-vector columns)))
        m))
    (new-matrix-nd [m shape]
      (construct-nd shape))
    (construct-matrix [m data]
      (construct-object-array data))
    (supports-dimensionality? [m dims]
      (>= dims 1)))

When called as:

(clojure.core.matrix.protocols/construct-matrix (object-array 1) [1])

Gives exception:

VerifyError (class: clojure/core/matrix$eval10586, method: invokeStatic signature: ()Ljava/lang/Object;) Incompatible object argument for function call  java.lang.Class.getDeclaredConstructors0 (:-2)





[CLJ-1789] Use transients with select-keys if possible Created: 28/Jul/15  Updated: 28/Jul/15

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

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


 Description   

Currently select-keys uses conj to add entries. If the map is editable, conj! could be used instead to improve select-keys performance.

Additionally keyseq is traversed as a seq but could be traversed via reduce instead, which might be faster.






[CLJ-1784] Reflector.getMethods should be cached Created: 21/Jul/15  Updated: 28/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Vladimir Sitnikov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Currently Reflector.getMethods performs expensive logic that includes java.lang.reflect.Method copying.
See: https://github.com/clojure/clojure/blob/b8607d5870202034679cda50ec390426827ff692/src/jvm/clojure/lang/Reflector.java#L373

In our application I see the following back-traces:

at Reflector.copyMethods
at Reflector.invokeInstanceMethod
at ...

These kind of backtraces are second top consumers of all the heap allocation.

JDK cannot cache Methods / Fields since they are mutable (e.g. user can call setAccessible here and there).
However, for the purposes of Clojure, I believe it should be fine to cache Methods and Fields.

What do you think?
E.g. WeakHashMap<Class, WeakReference<List<Method>>> or more sophisticated structure to account String name.



 Comments   
Comment by Alex Miller [ 23/Jul/15 8:19 AM ]

If you are seeing Reflector as a hot spot in your application, you should probably turn on warn-on-reflection and use type hints to avoid reflection.

Comment by Vladimir Sitnikov [ 28/Jul/15 6:10 AM ]

Do you mean there is absolutely no reason to use reflection in Clojure ever?
I do understand that if developer gives enough type hints the reflection would go away.

However:
1) I just do not know if it is easily doable (in other words, if it is possible at all, maintainable, etc)
2) I'm not sure if "always use type hints" is considered a best practice. For instance, warn-on-reflection documentation page says nothing like "always use type hints"
3) Caching copyMethods seems to be a low-hanging fruit here, so it would shave cpu cycles for those who omitted type hints

PS. I'm a java performance engineer, not a Clojure engineer (as in "my Clojure knowledge is somewhere near (+ x y)"), so I kindly beg on your forgiveness for me not doing RTFM.

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

No, I'm saying that if reflection is a hotspot in your application, usually it's worth investing a few minutes to add type hints in those hotspot areas and this is common advice for Clojure apps. Once that minimal work is done, few Clojure apps are bound by reflection.

Caching seems like an easy solution until you consider all of the management aspects. How does the cache get cleaned? Are the instances mutable and able to be reused? Are there cases where class loaders or code reloading create unexpected side effects? What are the concurrency effects of putting a shared resource in the invocation path? What is the memory impact of a cache and is it configurable?

Those are all things that would need to be investigated, meaning that this is not low-hanging fruit.





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

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

Type: Enhancement Priority: Major
Reporter: Abe Fettig Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: aot, compiler, interop

Attachments: Text File 0001-Avoid-compile-time-class-initialization-when-using-g.patch     Text File clj-1743-2.patch    
Patch: Code
Approval: Triaged

 Description   

I'm working on a project using Clojure and RoboVM. We use AOT compilation to compile Clojure to JVM classes, and then use RoboVM to compile the JVM classes to native code. In our Clojure code, we call Java APIs provided by RoboVM, which wrap the native iOS APIs.

But we've found an issue with inheritance and class-level static initialization code. Many iOS APIs require inheriting from a base object and then overriding certain methods. Currently, Clojure runs a superclass's static initialization code at compile time, whether using ":gen-class" or "proxy" to create the subclass. However, RoboVM's base "ObjCObject" class [1], which most iOS-specific classes inherit from, requires the iOS runtime to initialize, and throws an error at compile time since the code isn't running on a device.

CLJ-1315 addressed a similar issue by modifying "import" to load classes without running static initialization code. I've written my own patch which extends this behavior to work in ":gen-class" and "proxy" as well. The unit tests pass, and we're using this code successfully in our iOS app.

Patch: clj-1743-2.patch

Here's some sample code that can be used to demonstrate the current behavior (Full demo project at https://github.com/figly/clojure-static-initialization):

Demo.java
package clojure_static_initialization;

public class Demo {
  static {
    System.out.println("Running static initializers!");
  }
  public Demo () {
  }
}
gen_class_demo.clj
(ns clojure-static-initialization.gen-class-demo
  (:gen-class :extends clojure_static_initialization.Demo))
proxy_demo.clj
(ns clojure-static-initialization.proxy-demo)

(defn make-proxy []
  (proxy [clojure_static_initialization.Demo] []))

[1] https://github.com/robovm/robovm/blob/master/objc/src/main/java/org/robovm/objc/ObjCObject.java



 Comments   
Comment by Alex Miller [ 18/Jun/15 3:01 PM ]

No changes from previous, just updated to apply to master as of 1.7.0-RC2.

Comment by Alex Miller [ 18/Jun/15 3:03 PM ]

If you had a sketch to test this with proxy and gen-class, that would be helpful.

Comment by Abe Fettig [ 22/Jun/15 8:31 AM ]

Sure, what form would you like for the sketch code? A small standalone project? Unit tests?

Comment by Alex Miller [ 22/Jun/15 8:40 AM ]

Just a few lines of Java (a class with static initializer that printed) and Clojure code (for gen-class and proxy extending it) here in the test description that could be used to demonstrate the problem. Should not have any dependency on iOS or other external dependencies.

Comment by Abe Fettig [ 01/Jul/15 8:49 PM ]

Sample code added, let me know if I can add anything else!

Comment by Abe Fettig [ 27/Jul/15 2:21 PM ]

Just out of curiosity, what are the odds this could make it into 1.8?

Comment by Alex Miller [ 27/Jul/15 6:06 PM ]

unknown.





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

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

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

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

 Description   

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

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

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

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

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

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

The method bodies should instead be simply:

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

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

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

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

More discussion at Clojure dev.



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

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

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

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

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

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

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

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

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

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

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

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

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

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

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





[CLJ-1787] Add executables to distribution (suggested changes included) Created: 27/Jul/15  Updated: 27/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Morten Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Linix, OS X and Windows


Patch: Code

 Description   

Being new to clojure I was surprised that there was not a executable included in the download. It makes Clojure appear less proff then it is and is unnecessary complex to remember the exact java cmd line to use so I suggest that shell executable files are added to the distibution for the next version of Clojure. They could look as suggest below for Windows and OS X:

1) Linux and OS X:
#!/bin/bash
java -jar `dirname "$0"`/clojure-1.8.0.jar $@

2) Windows:
java -jar %~dp0\clojure-1.8.0.jar %1 %2 %3 %4 %5 %6 %7 %8






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

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

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

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

 Description   

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

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

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

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



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

Also see CLJ-764

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

Needs a perf check when done.

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

patch file to fix clj-1647

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

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

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

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

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

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

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

I will supply a patch file without the whitespace changes.

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

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

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

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

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

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

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

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

Any help would be greatly appreciated.

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

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

Did you mvn clean? Or rm target?

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

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

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

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

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

Comment by Alex Miller [ 13/Jul/15 1:17 PM ]

What's the status of this?

Comment by Kevin Woram [ 16/Jul/15 10:20 PM ]

Alex, I moved to Seattle and took a permanent position with Microsoft recently. This has kept me very busy and I haven't been able to spend time on Clojure at all. I probably won't be able to devote time to Clojure for another month or two.

Comment by Alex Miller [ 16/Jul/15 10:46 PM ]

Thanks for the heads up.

Comment by Matthew Gilliard [ 23/Jul/15 2:51 PM ]

Kevin, Alex, I could pick this up if you like?

Comment by Alex Miller [ 23/Jul/15 8:40 PM ]

Go for it

Comment by Alex Miller [ 23/Jul/15 8:40 PM ]

Go for it

Comment by Alex Miller [ 23/Jul/15 8:40 PM ]

Go for it

Comment by Alex Miller [ 23/Jul/15 8:40 PM ]

Go for it

Comment by Alex Miller [ 23/Jul/15 8:41 PM ]

Sorry, browser fail

Comment by Kevin Woram [ 25/Jul/15 7:03 PM ]

Thanks for picking it up Matthew, I appreciate it!

Comment by Kevin Woram [ 25/Jul/15 7:03 PM ]

Thanks for picking it up Matthew, I appreciate it!

Comment by Matthew Gilliard [ 27/Jul/15 11:30 AM ]

New patch: clj-1647.patch

Includes tests, fewer whitespace changes, manually thrown IAEs. I'll do some basic benchmarking soon, although I expect the overhead to be quite low as we're only checking the arguments once.

Kevin, the reason your patch was working for partition-all but not partition is that partition is defined early-ish in the bootstrapping process and the {:pre .. :post ..} maps aren't read by defn until it's enhanced later on.

Comment by Kevin Woram [ 27/Jul/15 12:10 PM ]

Thanks for solving that mystery Matthew!





[CLJ-1771] Support for multiple key(s)-value pairs in assoc-in Created: 29/Jun/15  Updated: 23/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Griffin Smith Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

All


Attachments: Text File clj-1771.patch    
Approval: Triaged

 Description   

It would be nice if assoc-in supported multiple key(s)-to-value pairs (and threw an error when there were an even number of arguments, just like assoc):

user=> (assoc-in {} [:a :b] 1 [:c :d] 2)
{:a {:b 1}, :c {:d 2}}
user=> (assoc-in {} [:a :b] 1 [:c :d])
IllegalArgumentException assoc-in expects even number of arguments after map/vector, found odd number


 Comments   
Comment by Matthew Gilliard [ 23/Jul/15 2:15 PM ]

Simple patch attached. I did not find any existing tests for assoc-in but I could add them if wanted.





[CLJ-1764] partition-by runs infinite loop when one element of infinite partition is accessed Created: 19/Jun/15  Updated: 23/Jul/15

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

Type: Defect Priority: Minor
Reporter: Leon Grapenthin Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: newbie

Attachments: Text File clj-1764.patch    
Approval: Triaged

 Description   
(def x (partition-by zero? (range)))
(first x)
;-> (0)
(first (second x))
;-> infinite loop

The reason is that partition-by counts and thus realizes each current partition to call itself recursively.

It seems like unexpected behavior, since the user may not intend to realize the entire partition.

It can be fixed by changing seq to lazy-seq in its last line.



 Comments   
Comment by Matthew Gilliard [ 23/Jul/15 2:36 PM ]

Patch as suggested by Leon, + test.





[CLJ-1550] Classes generated by deftype and defrecord don't play nice with .getPackage Created: 07/Oct/14  Updated: 22/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 15
Labels: classloader, deftype

Attachments: Text File 0001-CLJ-1550-define-package-for-class-in-DynamicClassLoa.patch     Text File CLJ-1550-v2.patch    
Patch: Code and Test

 Description   

Classes generated loaded by DynamicClassLoader return nil for .getPackage

(.getPackage String)
;; => #<Package package java.lang, Java Platform API Specification, version 1.7>
(deftype T [])
(.getPackage T)
;; => nil

This seems like a bug to me as it's not obvious why the class generated by deftype should exhibit different behaviour.

Patch: CLJ-1550-v2.patch



 Comments   
Comment by Alex Miller [ 07/Oct/14 8:54 AM ]

According to http://docs.oracle.com/javase/7/docs/api/java/lang/Class.html#getPackage() this method returns the package information found by the class loader or null if there is none. Its not clear to me that the current behavior is wrong per the spec. I would need to experiment more to see if this is unusual or not.

Comment by Bozhidar Batsov [ 07/Oct/14 9:05 AM ]

A bit of background for the issue. I'm no expert on the topic, but being able to procure all the class information except its package definitely looks strange to me.

Comment by Kevin Downey [ 07/Oct/14 11:46 AM ]

if you AOT compile(generate a class file on disk for a deftype), getPackage works fine, which suggests to me it is a jvm issue

Comment by Kevin Downey [ 07/Oct/14 11:49 AM ]

actually, it must just be that dynamicclassloader doesn't define a package for classes it loads

Comment by Alex Miller [ 07/Oct/14 12:13 PM ]

Yep, I believe that's correct.

Comment by Stuart Halloway [ 21/Jul/15 8:01 AM ]

There is no problem statement here. What is package information needed for?

Comment by Bozhidar Batsov [ 21/Jul/15 8:05 AM ]

I've linked the problem above. Basically tools like CIDER and vim-fireplace are relying on this information to implement things like completion hints.
This might not be problem when running your apps, but it's definitely a problem when inspecting their state...

Comment by Michael Blume [ 22/Jul/15 12:32 PM ]

s/Packate/Package





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

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

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

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


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

 Description   

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

Here's a simple example:

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

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



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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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





[CLJ-929] Accessing Java property starting with _ has issues in 1.4 Created: 07/Feb/12  Updated: 21/Jul/15

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

Type: Defect Priority: Minor
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: defrecord

Approval: Triaged

 Description   

When attempting to use interop syntax with symbols which aren't legal Java names (such as deleted?), the names are mangled a bit. That's necessary, of course, and the method of munging can be internal to the compiler. However, the behavior when munging changed a little between 1.3 and 1.4 beta1. Obviously the specifics of munging are something I should avoid relying on, but the way it changed looks like an accident or a bug even so.

The use-case I ran into is that defrecords contain a field named __meta for tracking their metadata. In both 1.3 and 1.4 you can get at that field with (. record __meta), which avoids munging. But on 1.3 (. record --meta) also accesses it (translating each - to a _), while on 1.4 (. record -meta) works and (. record --meta) doesn't.

Actually, looking at line 883 of Compiler.java, it looks like this may be related to the (. foo -property) syntax ported from CLJS, and indeed (. record ---meta) works, I guess by reducing to an "old style" (. record --meta). So that clears up why --meta fails: it's looking for __meta. I'm still not clear on why (. record -meta) works, though.

So it looks like the - prefix for properties is not 100% backwards-compatible like it seemed to be. Is this an issue we need to fix, or is the recommendation simply to never have fields that start with - or _?



 Comments   
Comment by Fogus [ 09/Feb/12 2:33 PM ]

Is this a general problem with fields starting with _ or just fields named __meta as in (defrecord [__meta] ...)

Comment by Alan Malloy [ 09/Feb/12 3:01 PM ]

It's a general issue. (defrecord [__meta]) actually breaks immediately, because the record mechanism itself generates a field named __meta, but any field named with a - or _ prefix has this issue.

user=> (defrecord Foo [-blah])
user.Foo
user=> (.-blah (Foo. 1))
IllegalArgumentException No matching field found: blah for class user.Foo clojure.lang.Reflector.getInstanceField (Reflector.java:289)





[CLJ-1300] take-while with n<1 behaves like (repeat 1) Created: 22/Nov/13  Updated: 21/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Yaron Peleg Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   
user=> (take-nth -1 [1 3 4])
; hangs
user=> (take 5 (take-nth -1 [5 9 14]))
(1 1 1 1 1)

I understand this behavior may be intentionally undefined,
but raising the issue on IRC didn't yield an answer on whether
this is a bug or grey area.



 Comments   
Comment by Stuart Halloway [ 21/Jul/15 5:06 PM ]

This is probably even worse now – i.e. likely different behavior with transducer arity.





[CLJ-842] clojure.pprint uses the old-style metadata. Created: 26/Sep/11  Updated: 21/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Baishampayan Ghose Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: print

Attachments: Text File 0001-Migrate-the-metadata-in-clojure.pprint.-to-the-new-s.patch     Text File clj-842-update-clojure.pprint-metadata-v2.txt    
Patch: Code

 Description   

I was looking through the implementation of clojure.pprint.* and found that it still uses the old-style metadata to mark vars as private - ^{:private true} instead of ^:private.

I have migrated the metadata to the new style in the attached trivial patch, which can be used in case this is deemed to be an issue.

FWIW, there are some other namespaces which use the old-style metadata as well; I am willing to fix those as well.



 Comments   
Comment by Andy Fingerhut [ 14/Nov/12 1:19 PM ]

clj-842-update-clojure.pprint-metadata-v2.txt dated Nov 14 2012 is identical to Baishampayan Ghose's 0001-Migrate-the-metadata-in-clojure.pprint.-to-the-new-s.patch dated Sep 26, 2011, except it applies cleanly to latest master.

Comment by Stuart Halloway [ 21/Jul/15 1:27 PM ]

Recategorizing as enhancement since the old form continues to be legal.





[CLJ-1131] Importing a non-existent class causes an exception that does not fully identify the source file Created: 17/Dec/12  Updated: 21/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs


 Description   

I'm in the process of stripping out some OSGi support, and I missed one import.

The exception identifies "init.clj", but I'd prefer to see the full path there, as I have a few different "init.clj" files in my overall project.

:core-services:compileClojure
Reflection warning, com/annadaletech/nexus/services/registry.clj:37 - call to unregisterAll can't be resolved.
Reflection warning, com/annadaletech/nexus/services/registry.clj:131 - call to getConfiguration can't be resolved.
Reflection warning, com/annadaletech/nexus/services/registry.clj:150 - call to getConfiguration can't be resolved.
Exception in thread "main" java.lang.ClassNotFoundException: org.osgi.framework.ServiceRegistration, compiling:(init.clj:1)
	at clojure.lang.Compiler$InvokeExpr.eval(Compiler.java:3387)
	at clojure.lang.Compiler.compile1(Compiler.java:7035)
	at clojure.lang.Compiler.compile1(Compiler.java:7025)
	at clojure.lang.Compiler.compile(Compiler.java:7097)
	at clojure.lang.RT.compile(RT.java:387)
	at clojure.lang.RT.load(RT.java:427)
	at clojure.lang.RT.load(RT.java:400)
	at clojure.core$load$fn__4890.invoke(core.clj:5415)
	at clojure.core$load.doInvoke(core.clj:5414)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5227)
	at clojure.core$compile$fn__4895.invoke(core.clj:5426)
	at clojure.core$compile.invoke(core.clj:5425)
	at clojuresque.tasks.compile$main$fn__64.invoke(compile.clj:23)
	at clojuresque.cli$with_command_line_STAR_.invoke(cli.clj:92)
	at clojuresque.tasks.compile$main.doInvoke(compile.clj:6)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:601)
	at clojure.lang.Var.invoke(Var.java:419)
	at clojuresque.Driver.main(Driver.java:39)
Caused by: java.lang.ClassNotFoundException: org.osgi.framework.ServiceRegistration


 Comments   
Comment by Gabriel Horner [ 17/May/13 3:56 PM ]

While it's reasonable to want this for your case, having long path names in a stacktrace could be inconvenient for others. I'd recommend posting your desired change on the dev list - https://groups.google.com/forum/?fromgroups#!forum/clojure-dev . If they're ok with it, then I'd recommend submitting a patch.

Comment by Stuart Halloway [ 21/Jul/15 1:17 PM ]

Enhanced error reporting is always an enhancement request, not a defect.





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

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

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

Approval: Triaged

 Description   

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

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


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

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

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

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





[CLJ-1159] clojure.java.io/delete-file doesn't return the status of the deletion(true/false) Created: 10/Feb/13  Updated: 21/Jul/15

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

Type: Defect Priority: Minor
Reporter: AtKaaZ Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs, io
Environment:

any


Approval: Triaged

 Description   

initially reported it here(somehow):
https://groups.google.com/d/msg/clojure/T9Kvr0IL0kg/wcKBfR9w_1sJ

Basically clojure.java.io/delete-file doesn't ever return false (even when silently is true, it returns the value of silently), it's due to how it's implemented - but it's obvious from the code, so I'll stop here.

Thanks.

PS: this is what I'm using as my current workaround:
(defn delete-file
"
an implementation that returns the true/false status
which clojure.java.io/delete-file doesn't do(tested in 1.5.0-RC14)
"
[f & [silently]]
(let [ret (.delete (clojure.java.io/file f))]
(cond (or ret silently)
ret
:else
(throw (java.io.IOException. (str "Couldn't delete " f)))
)
)
)

I'm sure you guys can find a better way, but as a clojure newbie(really!) that's what I have.



 Comments   
Comment by AtKaaZ [ 10/Feb/13 2:01 PM ]

I kinda just realized it affects all versions since and including 1.2, because it appears that its implementation was the same since then.

If it's not meant to return the result of the delete, maybe it should specifically return nil and/or the doc say something?

Comment by Sean Corfield [ 10/Feb/13 2:21 PM ]

As noted in a thread on the Clojure ML, you can pass a known value in the second argument position to detect a delete that failed:

(clojure.java.io/delete-file some-file :not-deleted)

This returns true on success and :not-deleted on failure.

However the docstring could be better worded to make that intention clear. Perhaps:

Delete file f. Return true if it succeeds. If silently is nil or false, raise an exception if it fails, else return the value of silently.
This allows you to detect whether the delete succeeded without catching an exception by passing a non-true truthy value as the second argument.

Comment by Stuart Halloway [ 21/Jul/15 8:20 AM ]

I think 'If silently is nil or false, raise an exception if it fails, else return the value of silently.' is sufficient





[CLJ-1167] repl value of *file* is "NO_SOURCE_PATH", of *source-path* is "NO_SOURCE_FILE" Created: 19/Feb/13  Updated: 21/Jul/15

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

Type: Defect Priority: Trivial
Reporter: Brian Marick Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: repl

Approval: Triaged

 Comments   
Comment by Brian Marick [ 19/Feb/13 4:22 PM ]

Forgot to mention: I think it's intended to be the other way around, given the names.





[CLJ-1463] Providing own ClassLoader for eval is broken Created: 10/Jul/14  Updated: 21/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Volkert Oakley Jurgens Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler
Environment:

Clojure 1.6.0



 Description   

clojure.lang.Compiler has a method with the signature

public static Object eval(Object form, boolean freshLoader)

but the freshLoader argument is ignored since https://github.com/clojure/clojure/commit/2c2ed386ed0f6f875342721bdaace908e298c7f3

Is there a good reason this still needs to be "hotfixed" like this?

We would like to provide our own ClassLoader for eval to manage the lifecycle of the generated classes.



 Comments   
Comment by Stuart Halloway [ 21/Jul/15 8:04 AM ]

This is not part of the public API of Clojure. We would need to understand more about the use case.





[CLJ-888] defprotocol should throw error when signatures include variable number of parameters Created: 29/Nov/11  Updated: 21/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Greg Chapman Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: errormsgs, protocols

Attachments: Text File 0001-Forbid-vararg-declaration-in-defprotocol-definterfac.patch    
Patch: Code and Test
Approval: Triaged

 Description   

I tried to use & in the signature for a method in defprotocol. Apparently (see below), this is compiled so that & becomes a simple parameter name, and there is no special handling for variable number of parameters. I think the use of & in a protocol signature ought to be detected and immediately cause an exception (I also think this restriction on the signatures ought to be documented; I couldn't find it specified in the current documentation, though of course it is implied (as I later realized) by the fact that defprotocol creates a Java interface).

user=> (defprotocol Applier (app [this f & args]))
Applier
user=> (deftype A [] Applier (app [_ f & args] (prn f & args) (apply f args)))
user.A
user=> (app (A.) + 1 2)
#<core$PLUS clojure.core$PLUS@5d9d0d20> 1 2
IllegalArgumentException Don't know how to create ISeq from: java.lang.Long
clojure.lang.RT.seqFrom (RT.java:487)



 Comments   
Comment by Alex Coventry [ 21/Oct/13 4:21 PM ]

Patch with test code attached. I have it throwing a CompilerException so that it shows source code location. Not sure whether this is kosher in clojure code, but I wish more macros provided this in their error handling.

Comment by Tassilo Horn [ 22/Oct/13 6:26 AM ]

This issue has already been discussed in CLJ-1024. There I provided a patch that forbids varargs and destructuring forms at various places including defprotocol/definterface. My patch had been applied shortly before clojure 1.5 was released, but it had a bug (forbid too many uses), so it got reverted and the bug closed and declined.

I was told to bring up the issue again after 1.5 has been released.

So here is my patch again. This time it's much more relaxed and only forbids varargs in defprotocol/definterface method declarations, and in deftype/defrecord and reify method implementations.

Comment by Alex Coventry [ 22/Oct/13 7:30 AM ]

Thanks, Tassilo. If there's anywhere in the JIRA system where I could check for prior work like that for other similar issues, I'd be grateful for a pointer.

Best regards,
Alex

Comment by Tassilo Horn [ 22/Oct/13 7:39 AM ]

New version of my patch.

Now I use a CompilerException with proper file/line/column information like Alex did. I also added his test case (which passes).

Concerning your question, Alex: a search for "varargs" would have listed CLJ-1024, but probably you wouldn't have looked into it anyway, because it's a closed issue...

Comment by Tassilo Horn [ 22/Oct/13 7:44 AM ]

Alex, if you don't object could we remove your patch in favor of mine which covers a bit more cases?

Comment by Alex Coventry [ 22/Oct/13 10:57 AM ]

Yep. Just read through 1024 and the associated mailing list discussion. You should totally get the credit: Your patch is more comprehensive and you have been on this a long time. Thanks for folding in the good parts of my patch.

Best regards,
Alex

Comment by Tassilo Horn [ 22/Oct/13 12:15 PM ]

Ok, great.

It seems I don't have the permissions to delete other peoples' attachments, so could you please delete your patch yourself?

Comment by Alex Coventry [ 23/Oct/13 2:44 PM ]

Sure, Tassilo. It's done.

I think this also needs a regression test for the case hugod originally pointed out. I initially made the same mistake as you there, but amalloy pointed it out[1] before I submitted the patch, so it is a natural mistake to make and should probably be documented in the source code.

Best regards,
Alex

[1] http://logs.lazybot.org/irc.freenode.net/%23clojure/2013-10-21.txt search for 14:48:34.

Comment by Tassilo Horn [ 24/Oct/13 2:00 AM ]

Alex, I've added the regression test you suggested. Thanks for pointing that out.

Also, I added tests checking definterface method declarations, and tests checking inline method implementations made with defrecord, deftype, and reify.

However, there's a problem with the tests for deftype and reify I don't know how to fix. When I eval the macroexpand forms used in the tests in a REPL, I can see that the CompilerException is successfully thrown and printed. But it also seems to be caught somewhere in the middle, so that the macroexpand returns a form and the exception doesn't make it to the (is (thrown? ...)). Therefore, I've commented the these tests and added a big FIXME.

Comment by Tassilo Horn [ 24/Oct/13 2:28 AM ]

New version of the patch with now all tests uncommented and passing. Andy Fingerhut made me aware that for the 4 deftype and reify tests, I need eval instead of just macroexpand.

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

I have not investigated the reason yet, but patch 0001-Forbid-vararg-declaration-in-defprotocol-definterfac.patch no longer applies cleanly after the latest commits to Clojure master on Oct 25 2013.

Comment by Tassilo Horn [ 28/Oct/13 2:21 AM ]

I've rebased the patch onto the current master so that it applies cleanly again.

Comment by Tassilo Horn [ 28/Oct/13 2:25 AM ]

Stu, I've assigned this issue to you because you've been assigned to CLJ-1165 which I have closed as duplicate of this issue.

One minor difference between my patch to this issue and CLJ-1165 is that here I use a CompilerException with file/line/column info whereas in CLJ-1165 I've used `ex-info`. I think the CE is more appropriate/informative, as the error is already triggered during macro expansion.





[CLJ-1161] sources jar has bad versions.properties resource Created: 11/Feb/13  Updated: 21/Jul/15

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

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

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

 Description   

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

version=${version}

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

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

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



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

Notes from the dev mailing list:

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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





[CLJ-1130] When unable to match a static method, report arity caller was looking for, avoid misleading field error Created: 17/Dec/12  Updated: 21/Jul/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: Unassigned
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-1689] Provide a default kv-reduce path for IPersistentVector Created: 31/Mar/15  Updated: 21/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Michael Blume Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File clj-1689-v1.patch     Text File clj-1689-v2.patch     Text File clj-1689-v3.patch    
Patch: Code

 Description   

Same as we do with IPersistentMap, make life a bit easier for implementors of IPersistentVector (though they can and should still provide a fast path)



 Comments   
Comment by Mike Anderson [ 21/Jul/15 1:07 AM ]

Would it potentially be better (or as an addition?) to add "reduce" and "kvreduce" methods to APersistentVector in the Java source?

This would allow better performance on the fast path, including the ability to make use of specialised implementations in subclasses (e.g. Tuples) in cases where these can do much better than the use of a volatile! box.





[CLJ-1782] Hide local IDE files in .gitignore Created: 21/Jul/15  Updated: 21/Jul/15

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

Type: Enhancement Priority: Trivial
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File clj-1782.patch    

 Description   

Several IDEs (e.g. Eclipse/CCW, IntelliJ IDEA/Cursive) create local files in project workspaces which should normally be ignored for the purposes of source control.

This patch proposes to add some common files that should be ignored to the .gitignore






[CLJ-1473] Badly formed pre/post conditions silently passed Created: 24/Jul/14  Updated: 19/Jul/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-274] cannot close over mutable fields (in deftype) Created: 23/Feb/10  Updated: 19/Jul/15

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

Type: Defect Priority: Major
Reporter: Anonymous Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: deftype

Approval: Vetted

 Description   

Simplest case:

user=>
(deftype Bench [#^{:unsynchronized-mutable true} val]
  Runnable
  (run [_]
    (fn [] (set! val 5))))

java.lang.IllegalArgumentException: Cannot assign to non-mutable: val (NO_SOURCE_FILE:5)

Functions should be able to mutate mutable fields in their surrounding deftype (just like inner classes do in Java).

Filed as bug, because the loop special form expands into a fn form sometimes:

user=>
(deftype Bench [#^{:unsynchronized-mutable true} val]
  Runnable
  (run [_]
    (let [x (loop [] (set! val 5))])))
java.lang.IllegalArgumentException: Cannot assign to non-mutable: val (NO_SOURCE_FILE:9)


 Comments   
Comment by Assembla Importer [ 01/Oct/10 9:35 AM ]

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

Comment by Assembla Importer [ 01/Oct/10 9:35 AM ]

donmullen said: Updated each run to [_] for new syntax.

Now gives exception listed.

Comment by Assembla Importer [ 01/Oct/10 9:35 AM ]

richhickey said: We're not going to allow closing over mutable fields. Instead we'll have to generate something other than fn for loops et al used as expressions. Not going to come before cinc

Comment by Nicola Mometto [ 30/Jan/15 7:37 AM ]

The patch for CLJ-1226 makes this work:

(deftype Bench [#^{:unsynchronized-mutable true} val]
Runnable
(run [_]
(let [x (loop [] (set! (.val _) 5))])))

If there's interest, I could provide a patch that converts closed over mutable field access by generated fns (for loop/try) into field access on closed over "this", i.e. val -> (.val this)

Comment by Nicola Mometto [ 30/Jan/15 7:39 AM ]

Related tickets: CLJ-1075 CLJ-1023





[CLJ-1570] Core clojure code mixes tabs with spaces Created: 20/Oct/14  Updated: 19/Jul/15

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

Type: Enhancement Priority: Trivial
Reporter: Michael Blume Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

A handful of functions in clojure.core, clojure.core-proxy, clojure.inspector, clojure.xml, clojure.pprint, clojure.stacktrace, clojure.set, and clojure.test switch partway through from indenting with spaces to indenting with tabs. This may cause them to display incorrectly depending on how the developer's editor is configured.

(not sure if this should be marked defect or task)



 Comments   
Comment by Andy Fingerhut [ 20/Oct/14 1:41 PM ]

Some similarities to CLJ-1026, although this problem does not cause the same issues with warnings on git patches as CLJ-1026 does, as far as I know.

One similarity is that if it is of interest (I don't know if it is), Alex or other Clojure screeners may want a procedure to clean them all up, and perhaps repeat that process periodically, e.g. before each major release.





[CLJ-1532] pr-str captures stdout from printing side-effects of lazily evaluated expressions. Created: 23/Sep/14  Updated: 19/Jul/15

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

Type: Defect Priority: Minor
Reporter: Silas Davis Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: print
Environment:

Linux



 Description   

Because clojure.core/pr-str uses with-out-str to capture the output of pr (and pr cannot be parsed a writable thing - just uses out).

If you pr-str the result of something lazy you can get side-effects written to stdout with println interspersed with the output. For example in my case I was extracting benchmarks from the library criterium and trying to print the data structure to the file. The solution would be to provide an overload of pr/pr-str that takes a writer. I note that pr-on provides some of the functionality but it is private.

This is an ugly bug when you're trying to persist program output in EDN, because the randomly interspersed stdout messages make it invalid for read-string. We shouldn't need our functions to be pure for pr-str to work as expected.

I've omitted a patch because although I think a fix is straight-forward I'm not sure quite where it should go (e.g. make pr-on public, change pr, change pr-str)



 Comments   
Comment by Stuart Halloway [ 19/Jul/15 7:48 AM ]

as a workound for this, use print-dup or print-method





[CLJ-1452] clojure.core/*rand* for seedable randomness Created: 20/Jun/14  Updated: 19/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: math

Attachments: Text File CLJ-1452.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Clojure's random functions currently use Math.random and related features, which makes them impossible to seed. This seems like an appropriate use of a dynamic var (compared to extra arguments), since library code that wants to behave randomly could transparently support seeding without any extra effort.

I propose (def ^:dynamic *rand* (java.util.Random.)) in clojure.core, and that rand, rand-int, rand-nth, and shuffle be updated to use *rand*.

I think semantically this will not be a breaking change.

Criterium Benchmarks

I did some benchmarking to try to get an idea of the performance implications of using a dynamic var, as well as to measure the changes to concurrent access.

The code used is at https://github.com/gfredericks/clj-1452-tests; the raw output is in a comment.

rand is slightly slower, while shuffle is insignificantly faster. Using shuffle from 8 threads is insignificantly slower, but switching to a ThreadLocalRandom manually in the patched version results in a 2.5x speedup.

Running on my 8 core Linode VM:

Benchmark Clojure Runtime mean Runtime std dev
rand 1.6.0 61.3ns 7.06ns
rand 1.6.0 + *rand* 63.7ns 1.80ns
shuffle 1.6.0 12.9µs 251ns
shuffle 1.6.0 + *rand* 12.8µs 241ns
threaded-shuffling 1.6.0 151ms 2.31ms
threaded-shuffling 1.6.0 + *rand* 152ms 8.77ms
threaded-local-shuffling 1.6.0 N/A N/A
threaded-local-shuffling 1.6.0 + *rand* 64.5ms 1.41ms

Approach: create a dynamic var *rand* and update rand, rand-int, rand-nth, and shuffle to use *rand*

Patch: CLJ-1452.patch

Screened by:



 Comments   
Comment by Gary Fredericks [ 21/Jun/14 7:50 PM ]

Attached CLJ-1452.patch, with the same code used in the benchmarks.

Comment by Gary Fredericks [ 23/Jun/14 8:34 AM ]

Should we be trying to make Clojure's random functions thread-local by default while we're mucking with this stuff? We could have a custom subclass of Random that has ThreadLocal logic in it (avoiding ThreadLocalRandom because Java 6), and make that the default value of *rand*.

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

I think the ThreadLocal question is interesting, not sure re answer.

It would be nice if the description summarized the results of the tests in a table and the criterium output was in the comments instead.

Comment by Gary Fredericks [ 30/Dec/14 1:26 PM ]

Full output from the test repo (which is summarized in the table in the description):

$ echo "Clojure 1.6.0"; lein with-profile +clj-1.6 run; echo "Clojure 1.6.0 with *rand*"; lein with-profile +clj-1452 run
Clojure 1.6.0

;;;;;;;;;;;;;;;;;;
;; Testing rand ;;
;;;;;;;;;;;;;;;;;;
WARNING: Final GC required 1.261632096547911 % of runtime
Evaluation count : 644646900 in 60 samples of 10744115 calls.
             Execution time mean : 61.297605 ns
    Execution time std-deviation : 7.057249 ns
   Execution time lower quantile : 56.872437 ns ( 2.5%)
   Execution time upper quantile : 84.483045 ns (97.5%)
                   Overhead used : 16.319772 ns

Found 6 outliers in 60 samples (10.0000 %)
    low-severe   1 (1.6667 %)
    low-mild     5 (8.3333 %)
 Variance from outliers : 75.5119 % Variance is severely inflated by outliers

;;;;;;;;;;;;;;;;;;;;;
;; Testing shuffle ;;
;;;;;;;;;;;;;;;;;;;;;
Evaluation count : 4780800 in 60 samples of 79680 calls.
             Execution time mean : 12.873832 µs
    Execution time std-deviation : 251.388257 ns
   Execution time lower quantile : 12.526871 µs ( 2.5%)
   Execution time upper quantile : 13.417559 µs (97.5%)
                   Overhead used : 16.319772 ns

Found 3 outliers in 60 samples (5.0000 %)
    low-severe   3 (5.0000 %)
 Variance from outliers : 7.8591 % Variance is slightly inflated by outliers

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; Testing threaded-shuffling ;;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
Evaluation count : 420 in 60 samples of 7 calls.
             Execution time mean : 150.863290 ms
    Execution time std-deviation : 2.313755 ms
   Execution time lower quantile : 146.621548 ms ( 2.5%)
   Execution time upper quantile : 155.218897 ms (97.5%)
                   Overhead used : 16.319772 ns
Clojure 1.6.0 with *rand*

;;;;;;;;;;;;;;;;;;
;; Testing rand ;;
;;;;;;;;;;;;;;;;;;
Evaluation count : 781707720 in 60 samples of 13028462 calls.
             Execution time mean : 63.679152 ns
    Execution time std-deviation : 1.798245 ns
   Execution time lower quantile : 61.414851 ns ( 2.5%)
   Execution time upper quantile : 67.412204 ns (97.5%)
                   Overhead used : 13.008428 ns

Found 3 outliers in 60 samples (5.0000 %)
    low-severe   3 (5.0000 %)
 Variance from outliers : 15.7596 % Variance is moderately inflated by outliers

;;;;;;;;;;;;;;;;;;;;;
;; Testing shuffle ;;
;;;;;;;;;;;;;;;;;;;;;
Evaluation count : 4757940 in 60 samples of 79299 calls.
             Execution time mean : 12.780391 µs
    Execution time std-deviation : 240.542151 ns
   Execution time lower quantile : 12.450218 µs ( 2.5%)
   Execution time upper quantile : 13.286910 µs (97.5%)
                   Overhead used : 13.008428 ns

Found 1 outliers in 60 samples (1.6667 %)
    low-severe   1 (1.6667 %)
 Variance from outliers : 7.8228 % Variance is slightly inflated by outliers

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; Testing threaded-shuffling ;;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
Evaluation count : 420 in 60 samples of 7 calls.
             Execution time mean : 152.471310 ms
    Execution time std-deviation : 8.769236 ms
   Execution time lower quantile : 147.954789 ms ( 2.5%)
   Execution time upper quantile : 161.277200 ms (97.5%)
                   Overhead used : 13.008428 ns

Found 3 outliers in 60 samples (5.0000 %)
    low-severe   3 (5.0000 %)
 Variance from outliers : 43.4058 % Variance is moderately inflated by outliers

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; Testing threaded-local-shuffling ;;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
Evaluation count : 960 in 60 samples of 16 calls.
             Execution time mean : 64.462853 ms
    Execution time std-deviation : 1.407808 ms
   Execution time lower quantile : 62.353265 ms ( 2.5%)
   Execution time upper quantile : 67.197368 ms (97.5%)
                   Overhead used : 13.008428 ns

Found 1 outliers in 60 samples (1.6667 %)
    low-severe   1 (1.6667 %)
 Variance from outliers : 9.4540 % Variance is slightly inflated by outliers
Comment by Gary Fredericks [ 30/Dec/14 1:28 PM ]

I think using a ThreadLocal is logically independent from adding *rand*, so it could be a separate ticket. I just suggested it here since it would for some uses mitigate any slowdown from *rand* but now that I'm looking at the benchmark results again the slowdown might be insignificant.

Comment by Gary Fredericks [ 30/Dec/14 5:44 PM ]

Also worth noting that (as I did in the benchmark code) with just the patch's changes (i.e., no ThreadLocal involved) users still gain the ability to do ThreadLocal manually, which is not currently possible.

Comment by Stuart Halloway [ 19/Jul/15 7:42 AM ]

workaround: data.generators provides seedable random





[CLJ-1005] Use transient map in zipmap Created: 30/May/12  Updated: 19/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Michał Marczyk Assignee: Unassigned
Resolution: Unresolved Votes: 13
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-997] max-key and min-key to return the first entry in case of several candidates Created: 18/May/12  Updated: 19/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Oleksandr Shyshko Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Consider the following code:

(def values [[1 2] [3 4] [5] [6 7] [8]])
(apply max-key count values)
; => [6 7]

Which returns the last max entry [6 7]. Why its not the first max entry [1 2]?
Well, truth is "max-key" gives no warranty on which max value will be returned.

Consider the following example in Scala:

println(List(List(0, 1, 2), List(2, 3, 4), List(1), List(1, 2, 3)).maxBy(_.length))
> List(0, 1, 2)

The very same function in Scala returns the first max entry (by default).

The code from relase 1.4 file "clojure/core.clj#4419-4426" looks is:
=======================
4419: (defn max-key
4420: "Returns the x for which (k x), a number, is greatest."
4421: {:added "1.0"
4422: :static true}
4423: ([k x] x)
4424: ([k x y] (if (> (k x) (k y)) x y))
4425: ([k x y & more]
4426: (reduce1 #(max-key k %1 %2) (max-key k x y) more)))
=======================

I am unsure what is the motivation in returning the last candidate, but

I suggest two following things:

1. Make "max-key" and "min-key" return the first max/min entry if there are several candidates.

This behavior seems more natural/convenient (to me), because in most cases you want to get first "winner".
(e.g. find the first biggest vector in a sequence) and less often you need to get the last entry – in those cases
you can do "reverse" before feeding a sequence to "max-key", thus it seems having "return first max" behavior more useful.

Line #4424 should have ">=" instead of ">".

2. Make "max-key" and "min-key" make warranty on order, which max/min entry will be returned (either first or last).

Line #4420 should say "Returns the x for which (k x), a number, is greatest. In case of several matches, the first max entry will be returned" or the same doc, but saying "the last max entry will returned".



 Comments   
Comment by Steve Miner [ 18/May/12 4:03 PM ]

The current behavior matches the documentation so I wouldn't consider it a "defect". There doesn't seem to be a compelling reason to impose a tie-breaker rule on the implementation.





[CLJ-938] Output of clojure.reflect is not suitable for type hints Created: 23/Feb/12  Updated: 19/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reflection, typehints


 Description   

I'm trying to write a macro which generate reify forms using some reflection.

clojure.reflect produces type symbols similar to 'java.util.Iterator<>

Unfortunately, the compiler doesn't accept the <> syntax in type hints:

(reify Iterable (^{:tag java.util.Iterator} iterator [this] nil)) ; works

(reify Iterable (^{:tag java.util.Iterator<>} iterator [this] nil)) ;; fails
CompilerException java.lang.RuntimeException: java.lang.ClassNotFoundException: java.util.Iterator<>, compiling:(NO_SOURCE_PATH:1325)

It seems like the compiler should understand the <> just enough to strip it, rather than reject it. This would make it much easier to write correct macros involving type hinting and reflection.

The workaround I have been using is:

(defn hint
"clojure.reflect demarks generics with angle brackets, but
the compiler does not support generics in type hints"
[obj tag]
(let [tag (-> tag .toString (.replace "<>" "") symbol)]
(with-meta obj {:tag tag}))



 Comments   
Comment by Brandon Bloom [ 24/Feb/12 1:37 AM ]

I'm sorry, I got this wrong earlier. The problem is real, but it's arrays, not generics.

My workaround is useless... :-/





[CLJ-1560] Forbid closing over mutable fields completely Created: 10/Oct/14  Updated: 19/Jul/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: None


 Description   

Closing over mutable fields should be forbidden completely (and generate compiler exception), not just when trying to set! them. As the change of the mutable field does not propagate into closed over ones, this leads to surprising bugs:

(defprotocol P 
  (-set [this]) 
  (-get [this]) 
  (-get-fn [this]))

(deftype T [^:unsynchronized-mutable val] 
  P 
  (-set [this] (set! val :bar)) 
  (-get [this] val) 
  (-get-fn [this] (fn [] val)))

(def x (->T :foo))

(def xf (-get-fn x))

user> (-set x)
:bar
user> (-get x)
:bar
user> (xf)
:foo ;; should be :bar !!!


 Comments   
Comment by Jozef Wagner [ 10/Oct/14 1:42 PM ]

related issue CLJ-274

Comment by Ghadi Shayban [ 28/Dec/14 10:42 PM ]

In the given example, the close-over happens before the set!, so the closure gets the value, not an assignable container. This is consistent with the rest of the language (pass-by-value not by mutable container)

Comment by Jozef Wagner [ 29/Dec/14 2:21 AM ]

Thanks for explanation. The ticket is about a proposal that closing over a mutable field should result in error being thrown, an not in a value. If value is desired, an explicit let binding will have to be used. So far, I haven't found a valid use case where closing over mutable field and getting the value closed over is the intended and wanted behavior.





[CLJ-1779] Optimise compiler usage of getMethod calls Created: 17/Jul/15  Updated: 17/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Currently the Clojure compiler makes multiple redundant calls to Method.getMethod(...) while emitting code, e.g.

gen.invokeStatic(Type.getType(Long.class), Method.getMethod("Long valueOf(long)"));

It seems to be the case that that:

a) These getMethod calls are effectively returning equivalent, immutable constant values
b) getMethod is moderately expensive (performs string analysis and quite a few object allocations)
c) These calls are very common during compilation of typical Clojure code

The proposed enhancement is to replace all of these getMethod calls with constant static values. This should improve compilation performance noticeably with no effect on behaviour.






[CLJ-1226] set! of a deftype field using field-access syntax causes ClassCastException Created: 26/Jun/13  Updated: 16/Jul/15

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

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

Attachments: Text File 0001-CLJ-1226-fix-set-of-instance-field-expression-that-r.patch     Text File 0001-CLJ-1226-fix-set-of-instance-field-expression-that-r-v2.patch    
Patch: Code and Test
Approval: Vetted

 Description   

Clojure 1.6.0-master-SNAPSHOT

user=> (defprotocol p (f [_]))
p
user=> (deftype t [^:unsynchronized-mutable x] p (f [_] (set! (.x _) 1)))
user.t
user=> (f (t. 1))
ClassCastException user.t cannot be cast to compile__stub.user.t user.t (NO_SOURCE_FILE:1

After patch:
Clojure 1.6.0-master-SNAPSHOT
user=> (defprotocol p (f [_]))
p
user=> (deftype t [^:unsynchronized-mutable x] p (f [_] (set! (.x _) 1)))
user.t
user=> (f (t. 1))
1

Patch: 0001-CLJ-1226-fix-set-of-instance-field-expression-that-r-v2.patch



 Comments   
Comment by Nicola Mometto [ 27/Jun/13 5:30 AM ]

This patch offers a better workaround for CLJ-1075, making it possible to write
(deftype foo [^:unsynchronized-mutable x] MutableX (set-x [this v] (try (set! (.x this) v)) v))

Comment by Nicola Mometto [ 25/Mar/15 4:39 PM ]

Updated patch to apply to current master





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

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

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

Approval: Triaged

 Description   

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

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

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

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

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



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

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

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

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

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

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

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

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





[CLJ-1777] Add function version of vswap! Created: 13/Jul/15  Updated: 14/Jul/15

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

Type: Enhancement Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None

Attachments: Text File 0001-CLJ-1777-Change-vswap-from-a-macro-to-an-inlineable-.patch    
Patch: Code

 Description   

Volatiles and vswap! were added in 1.7 as performant mechanisms to achieve uncoordinated mutation to the language.
given the fact that their addition was a performance-centric one, vswap! was implemented as a macro rather than a normal function to avoid runtime dereference of the function var and the optional apply overhead in case of multiple args.

However this:
-is not necessary
-breaks the api parallelism between volatile/atom swap!/vswap! reset!/vreset!
-makes impossible certain use cases (vswaps! in update-in forms)
-is potentially confusing given that swap! is a function

Infact the macro can be replaced with a function with :inline metadata.
This is a strictly additive change that will make so that for all the current valid usages of vswap! nothing will change, it will still be macroexpanded by the inliner and additionally since it is now a function it can be used in HOF contexts where it's not unusual to see swap! used.



 Comments   
Comment by Alex Miller [ 13/Jul/15 8:07 PM ]

Nicola, please don't set the fix version on tickets.

Comment by Nicola Mometto [ 13/Jul/15 8:10 PM ]

Sorry, I mixed the fix version with the affected field





[CLJ-1282] The quote special form should throw an exception if passed more than one form to quote Created: 23/Oct/13  Updated: 13/Jul/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: 5
Labels: errormsgs, ft, reader

Attachments: Text File CLJ-1282-p1.patch     Text File CLJ-1282-p2.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Quote currently ignores all but the first argument. In the case of being called accidentally with multiple values, it should throw an exception specifying the error.

user> (quote 1 2 3)
1

Patch: CLJ-1282-p2.patch

Screened by: Alex Miller

------- Original: --------

Every once in a while, you can just go down the rabbit hole.

I had an errant expression in my code:

(-> message get-message-values 'DESTINATION_MERCHANT_ID)

One would think this would work; it certainly would if the key was a keyword and not a symbol.

One would expect this to expand to:

('DESTINATION_MERCHANT_ID (get-message-values message))

however, the reader is involved, so it is as if the source were:

(-> message get-message-values (quote DESTINATION_MERCHANT_ID))

which expands to:

(quote (-> message get-message-values) DESTINATION_MERCHANT_ID))

... hilarity ensues! Because quote currently ignores extra parameters, my code gets the quoted value '(clojure.core/-> message get-message-values) rather than the expected string from the map; this shifts us from the "there's a bug in my code" to "the nature of reality is broken".

The correct expression is:

(-> message get-message-values (get 'DESTINATION_MERCHANT_ID))

This took quite a while to track down; if the

special form checked that it was passed exactly one form to quote and threw an exception otherwise, I think I would have caught this much earlier. It could even identify the expression it is quoting, which would provide a lot better understanding of where I went wrong.



 Comments   
Comment by Howard Lewis Ship [ 23/Oct/13 2:02 PM ]

Sorry, can't edit the description now to correct the formatting errors.

Comment by Howard Lewis Ship [ 24/Oct/13 6:09 PM ]

I just wanted to point out that your description is shorter, but makes it appear that such a use is unlikely and therefore unimportant; the detail of my description is to point out a reasonable situation where something explicable, but completely counterintuitive and confusing, does occur.

Comment by Alex Miller [ 24/Oct/13 8:28 PM ]

That's why I left the original in there too.

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

(quote) currently returns nil. Do we have an opinion about that?

Comment by Gary Fredericks [ 09/Dec/13 9:32 AM ]

Attached p1, which throws an IllegalArgumentException (wrapped in a CompilerException of course) for anything but 1 arg, and includes the number of args that were passed.

I can't think of any reason why (quote) would be useful, so I decided to throw on that too. Very easy to change of course.

Also added a test that (eval '(quote 1 2 3)) throws.

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

I recommend the following changes:

  • throw an ex-info that includes the offending form in its map {:form ...}
  • check only for the map data, not exception type or message, in the tests
Comment by Andy Fingerhut [ 31/Jan/14 6:31 PM ]

Patch CLJ-1282-p1.patch no longer applies cleanly after commits made to Clojure master on Jan 31 2014, probably due to the patch committed for CLJ-1318, and probably only because some lines of context changed in the test file. That would be trivial to update, but Stu's comments above suggest that more significant changes need to be made.

Comment by Gary Fredericks [ 01/Feb/14 9:19 AM ]

Throwing an ex-info is easy enough. I don't know how to avoid at least incidentally checking for the exception type, since the ExceptionInfo is wrapped in a CompilerException. I'll make a patch that keeps the class name in the test but doesn't do any checks on the cause aside from the ex-data. Let me know if I should do anything different.

Comment by Gary Fredericks [ 01/Feb/14 9:58 AM ]

Attached CLJ-1282-p2.patch which is off of the current master and addresses Stu's points.

Comment by Alex Miller [ 04/Feb/14 11:23 PM ]

Moving back to Triaged for more looks.

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

Currently (quote) returns nil, is it intended that this patch makes that an error or was this by accident?

Comment by Gary Fredericks [ 16/Feb/14 12:23 PM ]

I consciously chose to make (quote) an error – I made a comment about that earlier and didn't get any feedback, so I unilaterally decided to make it an error due to the fact that I couldn't think of any possible use for (quote).

It's an easy switch if somebody thinks differently.

Comment by Nicola Mometto [ 16/Feb/14 1:13 PM ]

I'm sorry I did not notice your previois comment.
I'm asking because I need to know whether I should throw on (quote) for tools.analyzer, currently it is allowed but I too think that (quote) should be an error.

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

I also think (quote) should throw an error.

Comment by Stuart Halloway [ 13/Jul/15 5:35 PM ]

This is an API change request, not a defect, and the change recommended here could break (admittedly ill-advised) programs.





[CLJ-1551] Consider transducer support for primitives Created: 07/Oct/14  Updated: 12/Jul/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: transducers

Approval: Vetted

 Description   

Need to consider how we can support primitives for transducers. In particular it may be that IFn needs overloading for L/D in addition to O.






[CLJ-1552] Consider kv support for transducers (similar to reducers fold) Created: 07/Oct/14  Updated: 12/Jul/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: transducers

Approval: Vetted

 Description   

In reducers, fold over a map has special support for kv. Consider whether/how to add this for transducers.



 Comments   
Comment by Marshall T. Vandegrift [ 16/Dec/14 11:13 AM ]

We don't have a JIRA "unvote" feature, but I'd like to register my vote against this proposed enhancement. As a heavy user of clojure.core.reducers, I consider the switch to k-v semantics when reducing a map to be a significant mis-feature. As only an initial transformation function applied directly to a map is able to receive the k-v semantics (a limitation I can’t see how would not carry over to transducers), this behavior crops up most frequently when re-ordering operations and discovering that an intermediate map has now caused an airity error somewhere in the middle of a chain of threaded transformations. I’ve never found cause to invoke it intentionally.





[CLJ-1553] Parallel transduce Created: 07/Oct/14  Updated: 12/Jul/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: 5
Labels: transducers

Approval: Vetted

 Description   

Consider how to create a parallel path for transducers, similar to reducers fold.






[CLJ-1775] Add any? Created: 07/Jul/15  Updated: 09/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Joshua Griffith Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Given the presence of `every?` and `not-every?`, it seems that `any?` should also exist given `not-any?`. While similar to `some`, indeed `(def any? (comp boolean some))`, it would make the interface consistent. Also, having `any?` would be nice when writing typed Clojure, where one often wants a boolean.



 Comments   
Comment by Colin Taylor [ 09/Jul/15 5:34 PM ]

Previous discussion - https://groups.google.com/d/msg/clojure/02msnrXJsSg/BdgfoeyVX7sJ.
My bad example notwithstanding, I still think this is symmetric and useful for interop. The typed Clojure point is valid too..





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

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

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

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

 Description   

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

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

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

Start a socket server by supplying an extra system property:

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

where options are:

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

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

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

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



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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

New -4 patch changes:

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

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

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

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

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

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

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

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

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

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

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

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

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

Marking incomplete, pending at least the repl exit question.

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

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

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

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

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

OK thanks for the update.

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

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

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

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

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

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

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

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

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

Comment by Stuart Halloway [ 08/Jul/15 8:31 AM ]

Lifecycle concerns

1. atoms are weak when actions (starting threads / sockets) are coordinated with recorded state (the map)
2. I see why the plumbing needs access to socket, but what is the motivation for expoosing it to outside code, seems like opportunity to break stuff
3. stating a server has a race condition
4. what happens if somebody wants to call start-server explicitly – how do they know whether that happens before or after config-driven process launches
5. what guarantees about when config-driven launch happens, vis-a-vis other startup-y things
6. is there a use for stop-servers other than shutdown?
7. does Clojure now need a shutdown-clojure-resources? I don't want to have to remember shutdown-agents, plus stop-servers, plus whatever we add next year
8. what happens on misconfiguration (e.g. nonexistent namespace)? will other servers still launch? what thread dies, and where does is report? will the app main process die before even reaching the user?

Comment by Alex Miller [ 08/Jul/15 9:12 AM ]

1. it's not in the patch, but the intention is that in the runtime on startup there is a call to (start-servers (System/getProperties)) and generally you're not starting servers on the fly (although it's broken out to make that possible).
2. just trying to make resources available, not sure how much locking down we want/need
3. I'm expecting this to be a startup thing primarily
4. I'm assuming the config-driven process will start in the runtime and thus it will happen first. Not sure how much we need to support the manual stuff - it just seemed like a good idea to make it possible.
5. dunno, haven't looked at where to do that yet. Probably somewhere similar to data_readers stuff?
6. no
7. the threads are daemons by default meaning that it will shut down regardless. If you set the daemon properties to false, then you're in control and need to call stop-servers where it's appropriate.
8. I thought about these questions and do not have good answers. Lots more of that stuff needs to be handled.





[CLJ-1776] Test that collections are valid statements Created: 08/Jul/15  Updated: 08/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: Michael Blume Assignee: Michael Blume
Resolution: Unresolved Votes: 0
Labels: compiler, test

Attachments: Text File clj-1776-v1.patch    
Patch: Code and Test

 Description   

It's possible to break the compiler such that vectors are emitted incorrectly when they're in statement position (I accidentally did this). This doesn't break any part of the Clojure test suite, but does break valid Clojure code (for me it hit taoensso's encore). Add tests to the test suite so defects of this kind are caught.






[CLJ-1729] Make Counted and count() return long instead of integer Created: 12/May/15  Updated: 08/Jul/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.



 Comments   
Comment by Erik Assum [ 07/Jul/15 9:24 AM ]

Looking at this, there are some problems like in
clojure.lang.RT#toArray line 1658
where you create a new Object array based on the count of a collection.
It seems as if new Object[] takes an int as a param, so one would have to downcast the long to an int for this to work.

Comment by Alex Miller [ 07/Jul/15 9:39 AM ]

If you're creating an Object[] greater than 2147483647, you may have other problems.

But yes, this ticket definitely needs a more thorough analysis as to what is affected. In this case, I think if the count is <= Integer/MAX_VALUE, then it should proceed and otherwise should throw an exception.

Comment by Erik Assum [ 08/Jul/15 8:19 AM ]

hmmm, this also causes problems wrt java.util.Collection size:
clojure.lang.APersistentSet#size line 164
Where size is specified by

http://docs.oracle.com/javase/7/docs/api/java/util/Collection.html#size()





[CLJ-1026] Mixed end-of-line endings in the source code Created: 17/Jul/12  Updated: 07/Jul/15

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

Type: Enhancement Priority: Minor
Reporter: John Szakmeister Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None

Attachments: Text File 0001-Introduce-end-of-line-normalization.patch     File clj-1026-bash-script.sh    
Patch: Code

 Description   

Some source files in Clojure have the DOS/Windows carriage return/line feed line endings, whereas the majority use the Linux/Unix line feed only line endings.

The main ongoing issue with this is that if there is a correctly formatted git patch for such a file, then using the recommended 'git am' command to apply such a patch always gives warnings. Some screeners see such warnings, and immediately disregard the patch, asking for the warnings to be fixed before they will consider it. As far as I can tell, this is impossible. Thus patches modifying files with CR/LF line endings are nearly in a deadlock state, requiring unusual effort from non-screeners to remind screeners that such patches are as good as they can be, and would they please ignore those warnings because there is no way around them.

If someone can find a way to apply git patches to such files without such warnings, please update this description or add a comment.

Stu Halloway asks: Wouldn't eliminating all CR/LF endings in Clojure source files invalidate many unapplied patches? My answer: Please do it anyway. Others will step in to fix those patches, if they become broken.

The attached file clj-1026-bash-script.sh does several things, which should be easy to verify by reading through the script itself, which is quite simple:

1. find all files with CR/LF endings, and remove the CR characters.
2. Does 'git status .' to show the files that have been modified
3. Does 'git diff .' and 'git diff -w .', showing the number of lines that have been modified, and the number of lines that have been modified in a way that changes something besides whitespace, which should be exactly 0 lines, because only whitespace has been changed.

Original description when this ticket was created:

While examining some of the Clojure source code, I discovered that some files had mixed line endings, or CRLF line endings on a non-Windows box. Using .gitattributes, we can correct that so that files have the right endings for the platform that it's on.



 Comments   
Comment by John Szakmeister [ 17/Jul/12 2:26 PM ]

Patch to fix line endings and introduce .gitattributes.

Comment by Stuart Halloway [ 20/Jul/12 4:47 PM ]

This looks like a change to every line in the world, which makes it hard to vet. Also: will it render incompatible all other outstanding patches at the time it is applied?

Comment by John Szakmeister [ 21/Jul/12 6:52 AM ]

You can use git diff -w $(git merge-base HEAD master) to see the actual diff minus the line ending change. Here it is inline:

:: git diff -w $(git merge-base HEAD master)
diff --git a/.gitattributes b/.gitattributes
new file mode 100644
index 0000000..7b89cfa
--- /dev/null
+++ b/.gitattributes
@@ -0,0 +1,6 @@
+*.txt           text
+*.clj           text
+*.html          text
+*.js            text
+*.css           text
+*.java          text diff=java

Also, no, it won't render all the outstanding patches incompatible. For one, it seems like the files that have the eols mixed or in CRLF aren't touched much. I also went through the majority of outstanding patches and couldn't fix one that conflicts. Secondly, format-patch emits the patch inline and is intended to be sent via email. SMTP says that all lines must end with CRLF, so line endings are effectively lost. So git will convert it to the right line ending on application.

It can conflict with any outstanding branches that you may have. Supplying the renormalization option on merge can help:

git merge -X renormalize <branch-name>

Or, you can enable this by default for your repository:

git config --local merge.renormalize true

If you think it's too much trouble, let's just drop it though.

Comment by Stuart Halloway [ 10/Aug/12 1:15 PM ]

Patch does not apply on my working copy of Clojure. I am using

git am -s ...
/Users/stu/repos/clojure/.git/rebase-apply/patch:344: trailing whitespace.
  p {  	
/Users/stu/repos/clojure/.git/rebase-apply/patch:350: space before tab in indent.
  	margin-left: 0.5in;
error: patch failed: epl-v10.html:1
error: epl-v10.html: patch does not apply
error: patch failed: src/jvm/clojure/asm/AnnotationVisitor.java:1
error: src/jvm/clojure/asm/AnnotationVisitor.java: patch does not apply
error: patch failed: src/jvm/clojure/asm/AnnotationWriter.java:1
error: src/jvm/clojure/asm/AnnotationWriter.java: patch does not apply

I am willing to do this, just inept.

Comment by Andy Fingerhut [ 10/Aug/12 1:21 PM ]

Stuart, I updated this page http://dev.clojure.org/display/design/JIRA+workflow a while back when I had trouble applying some patches involving files with carriage return line endings. I did some Googling on git docs and found the option '--keep-cr' that seems to help in such cases. Try that out.

Comment by Alex Miller [ 04/Sep/13 12:36 PM ]

I think in this case, we should not provide a patch but a process. And I would like to see this cleaned up.

Comment by Andy Fingerhut [ 04/Sep/13 1:14 PM ]

I am not sure what kind of process you mean, Alex, but here are some thoughts.

There are some patches attached to tickets right now that modify Java source files that have CR/LF line endings throughout. Typically those patches remove some lines, and add new ones, and those new lines typically have CR/LF line endings, too, because whatever editor the person used while making the patch saw the file have CR/LF line endings, and preserved that on new lines of text they added to the file.

So if you want to get rid of CRs throughout the Clojure source code, and keep them out, one way would be to:

(1) do a big commit to get rid of all of the current CR characters in source files.

(2) update the few pending patches that introduce lines with CRs (I checked. There are only 15 tickets with such patches right now.)

(3) I can automate a process of detecting patches that have CRs and flag them for CR removal.

In fact (2) can be done as a result of (3).

Comment by Alex Miller [ 04/Sep/13 2:29 PM ]

I just mean that any patch for this ticket is doomed to be continuously broken until it is applied, so it would be better to instead have a set of steps that someone can follow to fix all line endings, than a patch. That is #1 on your list should be a set of steps. #2 and 3 would be nice as well.

Comment by Andy Fingerhut [ 04/Sep/13 7:48 PM ]

clj-1026-bash-script.sh is not a patch, but a bash script that can be run in an unmodified Clojure source tree, preferably freshly cloned, that will delete carriage returns characters from all files except those in the .git directory, and directories.

It also prints out the results of a few commands to help you gain confidence that those are the only changes it made.

Tested on Mac OS X 10.8.4 and Ubuntu Linux 12.04.3.

Comment by Andy Fingerhut [ 31/Aug/14 3:32 PM ]

Shell script clj-1026-bash-script.sh has updated 'comments' printed with the latest stats of number of files and lines containing carriage return characters as of Aug 31 2014.

Comment by Michael Blume [ 07/Jul/15 5:16 PM ]

Would now be a good time to come back to this?

(I'd love to see the indentation in the Java source standardized too, but probably that's another ticket)

Comment by Andy Fingerhut [ 07/Jul/15 7:35 PM ]

I must say that it seems that since Alex Miller has been on the job, the times where patches are summarily rejected for various minor reasons like warnings, spurious white space changes, etc. has gone way down, since he often cleans them up himself. Kudos to Alex!





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

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

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

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

 Description   

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

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

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


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

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

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

alexdmiller said: A simple example I tried for illustration:

user=> (let [a 1 b 2] (assert (= a b)))
#<CompilerException java.lang.AssertionError: Assert failed: (= a b)
 a : 1
 b : 2