<< Back to previous view

[CLJ-1890] enhance pprint to print names of defrecord types Created: 05/Feb/16  Updated: 05/Feb/16

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

Type: Enhancement Priority: Minor
Reporter: Daniel Compton Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: pprint, print

Approval: Triaged

 Description   

pprint currently doesn't print the names of defrecord types, instead printing just the underlying map. This is in contrast to pr-str/println. This ticket proposes that the behaviour of pprint is changed to match pr-str and println's.

More discussion at https://groups.google.com/forum/#!topic/clojure-dev/lRDG6a5eE-s

user=> (defrecord myrec [a b])
user.myrec
user=> (->myrec 1 2)
#user.myrec{:a 1, :b 2}
user=> (pr-str (->myrec 1 2))
"#user.myrec{:a 1, :b 2}"
user=> (println (->myrec 1 2))
#user.myrec{:a 1, :b 2}
nil
user=> (pprint (->myrec 1 2))
{:a 1, :b 2}
nil


 Comments   
Comment by Daniel Compton [ 05/Feb/16 1:51 PM ]

The fix for this will needed to be ported to ClojureScript too.





[CLJ-440] java method calls cannot omit varargs Created: 27/Sep/10  Updated: 03/Feb/16

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

Type: Enhancement Priority: Critical
Reporter: Alexander Taggart Assignee: Ragnar Dahlen
Resolution: Unresolved Votes: 8
Labels: interop

Attachments: Text File 0001-CLJ-440-Allow-calling-vararg-Java-methods-without-va.patch    
Approval: Triaged

 Description   

Problem

Clojure calls to Java vararg methods require creating an object array for the final arg. This is a frequent source of confusion when doing interop.

E.g., trying to call java.util.Collections.addAll(Collection c, T... elements):

user=> (Collections/addAll [] (object-array 0))
false
user=> (Collections/addAll [])
IllegalArgumentException No matching method: addAll  clojure.lang.Compiler$StaticMethodExpr.<init> (Compiler.java:1401)

The Method class provides an isVarArg() method, which could be used to inform the compiler to process things differently.

From http://groups.google.com/group/clojure/browse_thread/thread/7d0d6cb32656a621

Latest patch: Removed because incomplete and goal not clear

Varargs in Java

As currently stated, the scope of this ticket is only to omit varargs, but this is only one case where Clojures handling of varargs differs from Java. For completeness, here is a brief survey of how Java handles vararg methods, which could hopefully inform a discussion for how Clojure could do things differently, and what the goal of this ticket should be.

Given the following setup:

VarArgs.java
public class VarArgs {

    public static class SingleVarargMethod {
        public static void m(String arg1, String... args) {}
    }

    public static class MultipleVarargMethods {
        public static void m(String... args) {}
        public static void m(String arg1) {}
        public static void m(String arg1, String... args) {}
    }
}
Java Possible clojure equivalent? Comments
VarArgs.SingleVarargMethod.m("a"); (SingleVarargMethod/m "a")  
VarArgs.SingleVarargMethod.m("a", "b"); (SingleVarargMethod/m "a" "b")  
VarArgs.SingleVarargMethod.m("a", "b", "c"); (SingleVarargMethod/m "a" "b" "c")  
VarArgs.SingleVarargMethod.m("a", new String[]{"b", "c"}); (SingleVarargMethod/m "a" (object-array ["b" "c"]))  
VarArgs.MultipleVarargMethods.m(); (MultipleVarargMethods/m)  
VarArgs.MultipleVarargMethods.m((String) null); (MultipleVarargMethods/m nil) Use type hints to disambiguate?
VarArgs.MultipleVarargMethods.m((String[]) null); (MultipleVarargMethods/m nil) Use type hints to disambiguate?
VarArgs.MultipleVarargMethods.m("a", null); (MultipleVarargMethods/m "a" nil)  
VarArgs.MultipleVarargMethods.m("a", new String[]{}); (MultipleVarargMethods/m "a" (object-array 0))  
VarArgs.MultipleVarargMethods.m(new String[]{"a"}); (MultipleVarargMethods/m (object-array ["a"]))  
VarArgs.MultipleVarargMethods.m("a", new String[]{"b", "c"}); (MultipleVarargMethods/m "a" (object-array ["b" "c"]))  


 Comments   
Comment by Assembla Importer [ 27/Sep/10 8:19 PM ]

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

Comment by Alexander Taggart [ 01/Apr/11 11:16 PM ]

Patch adds support for varargs. Builds on top of patch in CLJ-445.

Comment by Alexander Taggart [ 05/Apr/11 5:45 PM ]

Patch updated to current CLJ-445 patch.

Comment by Nick Klauer [ 29/Oct/12 8:12 AM ]

Is this ticket on hold? I find myself typing (.someCall arg1 arg2 (into-array SomeType nil)) alot just to get the right method to be called. This ticket sounds like it would address that extraneous into-array arg that I use alot.

Comment by Andy Fingerhut [ 29/Oct/12 10:45 AM ]

fixbug445.diff uploaded on Oct 29 2012 was written Oct 23 2010 by Alexander Taggart. I am simply copying it from the old Assembla ticket tracking system to here to make it more easily accessible. Not surprisingy, it doesn't apply cleanly to latest master. I don't know how much effort it would be to update it, but only a few hunks do not apply cleanly according to 'patch'. See the "Updating stale patches" section on the JIRA workflow page here: http://dev.clojure.org/display/design/JIRA+workflow

Comment by Andy Fingerhut [ 29/Oct/12 10:56 AM ]

Ugh. Deleted the attachment because it was for CLJ-445, or at least it was named that way. CLJ-445 definitely has a long comment history, so if one or more of its patches address this issue, then you can read the discussion there to see the history.

I don't know of any "on hold" status for tickets, except for one or two where Rich Hickey has explicitly said in a comment that he wants to wait a while before making the change. There are just tickets that contributors choose to work on and ones that screeners choose to screen.

Comment by Alex Miller [ 02/Feb/16 11:47 AM ]

I would love to see an updated patch on this ticket that specifically addressed the varargs issue without building on the other mentioned ticket and patch (which is of lower priority).

Comment by Ragnar Dahlen [ 03/Feb/16 8:01 AM ]

I had a stab at this, have attached an initial patch, parts of which I'm not too sure/happy about so feedback would be appreciated.

The patch takes the following approach:

  1. Teach Reflector/getMethods how to find matching vararg methods. In addition to the current constraints, a method can also match if it is a varargs method, and the arity of the method is one more than the requested arity. That means it's a varargs method we could call, but the user hasn't provided the varargs argument.
  2. In MethodExpr/emitTypedArgs we handle the case were there is one more argument in the method being called than there were arguments provided. The only case were that should happen is when it is a varargs method and the last argument was not provided. In that case we push a new empty object array to the stack.

I'm not to sure about my implementation of the second part. It could open up for some hard to understand bugs in the future. One option would be to be more defensive, and make sure it's really the last argument for instance, or even pass along the Method object (or a varargs flag) so we know what we can expect and need to do.

Comment by Ragnar Dahlen [ 03/Feb/16 8:49 AM ]

I realised my patch is missing two important cases; the interface handling in Reflector and handling multiple matching methods. I'll look into that too, but would still appreciate feedback on the approach in MethodExpr/emitTypedArgs.

Comment by Alex Miller [ 03/Feb/16 9:00 AM ]

I am in favor of using isVarArg() to explicitly handle this case rather than guessing if we're in this situation. We should check the behavior (and add tests where it seems needed) for calling a var args method with too few args, too many args, etc. And also double-check that non vararg cases have not changed behavior.

Also, keep in mind that as a general rule, existing AOT compiled code may rely on calling into public Reflector methods, so if you change the signatures of public Reflector methods, you should leave a version with the old arity that has some default behavior for backwards compatibility.





[CLJ-1764] partition-by runs infinite loop when one element of infinite partition is accessed Created: 19/Jun/15  Updated: 01/Feb/16

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: None

Attachments: Text File clj-1764.patch    
Patch: Code and Test
Approval: Prescreened

 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.

Approach: Change from using seq to lazy-seq in its last line

Patch: clj-1764.patch

Screened-by: Alex Miller - I did a perf check too and seemed to be about the same, possibly even faster on average (gc effects mean there is a lot of deviation).

(def r (range 100000))
(dotimes [_ 20]
  (time (count (partition-by odd? r))))


 Comments   
Comment by Matthew Gilliard [ 23/Jul/15 2:36 PM ]

Patch as suggested by Leon, + test.





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

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

Type: Defect Priority: Major
Reporter: Paul Stadig Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: None

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

 Description   

thread-bound? returns true for thread-local bindings that were not bound by the current thread. This is at odds with the docstring, which states that if true is returned, set! should succeed.

ere 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=> 

Approach: thread-bound? should return false in the case where there is a binding and that binding was not established by the current thread. The patch adds a new function to Var as core does not have visibility into the non-public Var$TBox.

Patch: thread-bound.diff

Screened by: Alex Miller



 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-1398] Update URLs in javadoc.clj Created: 02/Apr/14  Updated: 01/Feb/16

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

Type: Defect Priority: Minor
Reporter: Eli Lindsey Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

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

 Description   

javadoc.clj contains javadoc base urls for the jdk and several popular open source libraries. Most of these urls are broken.

For example, create a project with the following dependencies:

[org.clojure/clojure "1.8.0"]
[commons-codec "1.10"]
[commons-io "2.4"]
[commons-lang "2.6"]
[org.apache.commons/commons-lang3 "3.4"]
[com.google.guava/guava "19.0"]

And then try the following javadoc checks:

(javadoc java.lang.AutoCloseable)
(javadoc java.util.function.Function)
(javadoc org.apache.commons.codec.Decoder)
(javadoc org.apache.commons.io.IOUtils)
(javadoc org.apache.commons.lang.ArrayUtils)
(javadoc org.apache.commons.lang3.ArrayUtils)
(javadoc com.google.common.collect.BiMap)

Fixes:

  • Update jdk javadoc urls and explicitly include 1.6, 1.7, and 1.8, falling back to 1.8 (most common in use right now)
  • Update commons-codec javadoc url
  • Update commons-io javadoc url
  • Update commons-lang javadoc url (not that commons-lang last release was 2.6)
  • Add commons-lang3 javadoc url (the 3+ version of commons-lang changed the base package name)
  • Add guava javadoc url (another very common Java library)

Patch: clj-1398.patch



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

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

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

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

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

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

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

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

Comment by Alex Miller [ 01/Feb/16 6:15 AM ]

I squashed the patches and made some minor modifications in the jdk urls. Patch attribution retained.





[CLJ-1458] Enhance the performance of map merges Created: 04/Jul/14  Updated: 01/Feb/16

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

Type: Enhancement Priority: Critical
Reporter: Yongqian Li Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: performance

Attachments: Text File 0001-very-simple-test-of-the-merge-function.patch     Text File clj-1458-4.patch     Text File CLJ-1458-5.patch     Text File CLJ-1458-6.patch     Text File CLJ-1458-transient-merge2.patch     Text File CLJ-1458-transient-merge3.patch     Text File CLJ-1458-transient-merge.patch     Text File merge-test-2.patch     File transient-merge.diff    
Patch: Code and Test
Approval: Triaged

 Description   

It would be nice if merge used transients.

Patch

  • clj-1458-6.patch

Approach
Migrate c.c/merge later in core after transients & reduce. Leave older version as merge1 for use in cases the precede the newer definition. Make APersistentMap/conj & ATransientMap/cons aware of IKVReduce.

The attached patch preserves two existing behaviors of merge

  • metadata propagation
  • the right hand side of the merges can be a Map.Entry, an IPersistentVector where size=2, and regular maps.

Screened by:



 Comments   
Comment by Jason Wolfe [ 13/Sep/14 5:09 PM ]

I will take a crack at a patch today.

Comment by Jason Wolfe [ 13/Sep/14 5:42 PM ]

This patch (transient-merge.diff) makes merge, merge-with, and zipmap (since it was right there and could obviously benefit from transients as well) use transients.

Three potential issues:

  • I had to move the functions, since they depend on transient and friends. I assume this is preferable to a forward declaration. This was the best place I could find, but happy to move them elsewhere.
  • I added multiple arities, to avoid potential performance cost of transient-ing a single argument. Happy to undo this if desired.
  • I had to slightly alter the logic in merge-with, since transient maps don't support contains? (or find).
Comment by Michał Marczyk [ 14/Sep/14 12:43 PM ]

I posted a separate ticket for zipmap, with patch, on 30/May/12: CLJ-1005.

Comment by Jason Wolfe [ 14/Sep/14 5:28 PM ]

Ah, sorry if I overstepped then. Happy to remove that change from this patch then if that will simplify things – just let me know.

Comment by Ghadi Shayban [ 28/Dec/14 10:07 PM ]

alternate approach attached delaying merge until after protocols load, and then using transducers.

Comment by Michael Blume [ 28/Dec/14 11:50 PM ]

Looks like you're doing (get m k) twice – shouldn't that be thrown in a local?

Comment by Michael Blume [ 29/Dec/14 1:41 PM ]

um, put, in a local, I mean, 'throw' was a bad choice of word.

Comment by Ghadi Shayban [ 29/Dec/14 2:14 PM ]

Yeah there's that – won't be using get anyways after CLJ-700 gets committed.

We should add performance tests too. merging two maps, three, many maps, also varying the sizes of the maps, and for merge-with, varying the % of collisions.

Need to go back to the (some identity) logic, otherwise metadata is propagated from maps other than the first provided. I'll fix later.

Comment by Michael Blume [ 29/Dec/14 2:49 PM ]

I don't know if this is supposed to be allowed, but this breaks

(merge {} [:foo 'bar])

which is used in the wild by compojure-api

Comment by Michael Blume [ 29/Dec/14 2:49 PM ]

https://github.com/metosin/compojure-api/blob/0.16.6/src/compojure/api/meta.clj#L198

Comment by Michael Blume [ 29/Dec/14 2:54 PM ]

Ghadi, contains? uses get under the covers, so it's still two gets, right? It seems like it'd be more performant to stick with the ::none trick.

Comment by Nicola Mometto [ 29/Dec/14 5:36 PM ]

This calls for if-let + find.

Comment by Ghadi Shayban [ 29/Dec/14 10:37 PM ]

new patch addressing concerns so far

Comment by Ghadi Shayban [ 29/Dec/14 10:48 PM ]

CLJ-1458-transient-merge3.patch removes silly inlining macro, uses singleton fns instead.

Comment by Michael Blume [ 29/Dec/14 11:14 PM ]

Nice =)

This should come with tests. If we want to preserve the ability to merge with a MapEntry, we should test it. This isn't so much a weakness of the patch as of the existing tests. I see merge and merge-with being used a few times in the test suite, but I see no test whose purpose is to test their behavior.

Comment by Michael Blume [ 29/Dec/14 11:17 PM ]

Extremely simple merge test, we need more than this, but this is a start

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

clj-1458-4.patch refreshed to apply to master, no changes.

Comment by Ghadi Shayban [ 09/Jan/16 5:09 PM ]

I'd like to reevaluate the scope of this ticket. Can we address 'merge' only and defer 'merge-with'? It's by far the more common function. I've attached a new simplified patch.

Comment by Ghadi Shayban [ 09/Jan/16 9:50 PM ]

CLJ-1458-6.patch is yet another cleaner approach

Comment by Alex Miller [ 01/Feb/16 5:17 AM ]

Can you update the ticket approach section to discuss the APersistentMap.cons / ASSOC changes. Also, can you add a before / after perf test for one or more common cases?





[CLJ-1889] Add optional predicate to string trim functions that determines if a character should be trimmed Created: 27/Jan/16  Updated: 28/Jan/16

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

Type: Enhancement Priority: Minor
Reporter: Tamas Szabo Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: string

Attachments: Text File CLJ-1889-trim-enhancement.patch    
Patch: Code and Test
Approval: Triaged

 Description   

The proposal is that the trim functions (trim, triml, and trimr) would get a second arity with a function trim?:

[trim? ^CharSequence s]

trim? comes first to support partial.

New doc string would be:

"Removes characters from both ends of string. 
 If trim? is omitted white space is removed. When supplied it accepts 
 a character and returns true if the character should be removed."

Example test:

(deftest t-trim 
  (is (= "foo" (s/trim "  foo  \r\n"))) 
  (is (= "bar" (s/trim "\u2000bar\t \u2002"))) 

  ;; Additional test 
  (is (= "bar" (s/trim "$%#\u2000bar\t \u2002%$#" 
                       #(or (Character/isWhitespace %) ((set "$#%") %))))))

Similar to Python's strip - https://docs.python.org/2/library/stdtypes.html#str.strip

Approach: The proposed solution isn't very DRY but it follows the design guidelines at the top of the file, more exactly point 3:

"3. Functions take advantage of String implementation details to
write high-performing loop/recurs instead of using higher-order
functions. (This is not idiomatic in general-purpose application
code.)"

First I had a solution in which I replaced Character/isWhitespace from the current implementation by calling pred. pred was defaulted to an is-whitespace? function.
That code is of course nicer, even trim-newline could just call into trimr, removing a lot of duplication, but it adds the overhead of always calling a function, instead of calling Character/isWhitespace directly.

The only way I can see to have optimised and DRYer code is to use macros, but I don't think that it will necessary lead to nicer code.

Given the existing design style of the other functions in string.clj I felt that the best solution would be to just simply duplicate in favour of optimised code.



 Comments   
Comment by Tamas Szabo [ 27/Jan/16 2:44 PM ]

Proposed solution. Code + tests.

Comment by Tamas Szabo [ 27/Jan/16 3:42 PM ]

Added new patch that renames pred to trim?

Comment by Andy Fingerhut [ 27/Jan/16 6:27 PM ]

Note that Java, and thus Clojure/Java, uses UTF-16 encoding for strings in memory. Thus if you wanted to trim a set of Unicode code points from the beginning and/or end of a string, the API of trim? taking a single 16-bit Java character is not enough information to determine whether it should be trimmed or not.

If you want to handle that generality, it would require a more complex implementation, which checks whether the first/last character is one half of a code point that is encoded as 2 16-bit Java characters, and pass a 32-bit int to trim?, or something similar to that.

I have no objections if these API enhancements are made without enabling testing against an arbitrary Unicode code point. In the past, similar suggestions have been rejected in Clojure's built-in lib, e.g. CLJ-945

Comment by Tamas Szabo [ 28/Jan/16 1:56 AM ]

Yes, the UTF-16 encoding and Character representing either a codepoint or a half-codepoint is a bit of a mess, isn't it?

In the Java String and Character API's the methods that accept char, handle only characters in the Basic Multilingual Plane.
trim? accepts a character, so following the same behavior it will work only for removing characters in the Basic Multilingual Plane.

I think even this would be fine, but additionally because the high/low surrogates and the BMP characters are disjoint, you could actually use the same implementation to remove Unicode code points that aren't in the BMP. You can just say that both the high and low code unit of the codepoint are "unwanted".

Ex:
𝄞 is "\uD834\uDD1E"

user=> (trimr (set " \uD834\uDD1E") "example string  𝄞  ")
"example string"
Comment by Andy Fingerhut [ 28/Jan/16 5:11 AM ]

Agreed, but probably better to anti-recommend such an implementation of trimr for removing such things, because it would also remove only one UTF-16 Java character out of 2 high/low surrogates if it matched a member of the set, even if the other surrogate didn't match anything in the set, which would leave behind a malformed UTF-16 string.

Again, probably best to either not include this in the implementation at all, and at most warn about it in the docs, or to handle it in the implementation by checking for high/low surrogates in the loop(s).

Comment by Tamas Szabo [ 28/Jan/16 6:00 AM ]

Yes, you're right. That solution won't work in all cases, so it can't be recommended.

I am slightly inclined towards having trim? accept chars and work only for removing BMP characters. This will arguably be enough for the majority of the use cases.
The other solution can be used for all use cases, but then trim? will have to accept int, or 2 chars, or a string, so trim? would be less intuitive (although closer to the real world ), and writing those trim? functions would be less user friendly.

That being said, I am happy to change the implementation to do that if it is required.

Currently, I'm not even sure if the enhancement will be accepted or rejected or what the process for that is.





[CLJ-1293] Portable "catch-all" mechanism Created: 05/Nov/13  Updated: 27/Jan/16

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: 8
Labels: None

Attachments: Text File CLJ-1293-v001.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Design page: http://dev.clojure.org/display/design/Platform+Errors

CLJS ticket/patch: http://dev.clojure.org/jira/browse/CLJS-661

This patch is more permissive than my patch for CLJS: The CLJS patch ensures :default catch blocks occur between non-default catch blocks and finally blocks, if present. This patch just makes (catch :default ...) a synonym for (catch Throwable ...). I wanted to keep the change to the compiler minimum.

Open Question: Catch Throwable (patch v001 does this) or Exception? Alternatively, a more carefully crafted list of "non-fatal" errors. See Scala's NonFatal pattern extractor: http://www.scala-lang.org/api/current/index.html#scala.util.control.NonFatal$



 Comments   
Comment by Brandon Bloom [ 28/Dec/14 11:33 AM ]

Noticed this switched from "Minor" to "Critical", so I figured I should mention that I later realized that we might want :default to catch Exception instead of Throwable, so as to avoid catching Error subclasses. Javadocs say: "An Error is a subclass of Throwable that indicates serious problems that a reasonable application should not try to catch." If that's what we actually want, I can provide an updated patch.

Comment by Alex Miller [ 28/Dec/14 2:19 PM ]

Seems like an open question, might be best just to list it as such in the description.

I don't really expect to reach consensus on the ticket or patch right now, just trying to update priorities and raise visibility for discussion with Rich once we get to 1.8.





[CLJ-1888] AReference#meta() is synchronized Created: 26/Jan/16  Updated: 27/Jan/16

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

Type: Enhancement Priority: Major
Reporter: Roger Kapsi Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance

Attachments: PNG File aref-meta-after.png     PNG File aref-meta.png     Text File clj-1888-2.patch     Text File clj-1888.patch    
Patch: Code
Approval: Triaged

 Description   

We use Clojure for a "rules engine". Each function represents a rule and metadata describes the rule and provides some static configuration for the rule itself. The system is immutable and concurrent.

If two or more Threads invoke the same Var concurrently they end up blocking each other because AReference#meta() is synchronized (see attached screenshot, the red dots).

(defn 
  ^{:rule {:remote-address "127.0.0.1"}}
  example
  [request]
  (let [rule (:rule (meta #'example))]
    (= (:remote-address rule) (:remote-address request))))

Approach: Replace synchronized block with a rwlock for greater read concurrency. This approach removes meta read contention (see real world results in comments). However, it comes with the downsides of:

  • extra field for every AReference (all namespaces, vars, atoms, refs, and agents)
  • adds construction of lock into construction of AReference (affects perf and startup time)

Patch: clj-1888-2.patch replaces synchronized with a rwlock for greater read concurrency

Alternatives:

  • Use volatile for _meta and synchronized for alter/reset. Allow read of _meta just under the volatile - would this be safe enough?
  • Extend AReference from ReentrantReadWriteLock instead of holding one - this is pretty weird but would have a different (potentially better) footprint for memory/construction.


 Comments   
Comment by Alex Miller [ 26/Jan/16 10:19 PM ]

A volatile is not sufficient in alterMeta as you need to read/update/write atomically.

You could however use a ReadWriteLock instead of synchronized. I've attached a patch that does this - if you have a reproducible case I'd be interested to see how it affects what you see in the profiler.

There are potential issues that would need to be looked at - this will increase memory per reference (the lock instance) and slow down construction (lock construction) at the benefit of more concurrent reads.

Comment by Roger Kapsi [ 27/Jan/16 8:34 AM ]

Hey Alex,

I do have a reproducible case. The blocking has certainly disappeared after applying your patch (see attached picture). The remaining blocking code on these "WorkerThreads" is sun.nio.ch.SelectorImpl.select(long) (i.e. not clojure related).

You can repro it yourself by executing something like the code below concurrently in an infinite loop.

(defn 
  ^{:rule {:remote-address "127.0.0.1"}}
  example
  [request]
  (let [rule (:rule (meta #'example))]
    (= (:remote-address rule) (:remote-address request))))

Suggestions for the patch: Make the meta lock a final field and maybe pull the read/write locks into local variables to avoid the double methods calls.

alterMeta(...)
  Lock w = _metaLock.writeLock();
  w.lock();
  try {
    // ...
  } finally {
    w.unlock();
  }
}




[CLJ-706] make use of deprecated namespaces/vars easier to spot Created: 05/Jan/11  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Stuart Halloway Assignee: Andrew Rosa
Resolution: Unresolved Votes: 19
Labels: errormsgs

Attachments: File 706-deprecated-var-warning.diff     Text File 706-deprecated-var-warning-patch-v2.txt     File 706-fix-deprecation-warnings-agents.diff     File 706-fix-deprecation-warnings-on-replicate.diff     File 706-fix-deprecation-warning-test-junit.diff     File 706-warning-on-deprecated-ns.diff    
Patch: Code
Approval: Triaged

 Description   

From the mailing list http://groups.google.com/group/clojure/msg/c41d909bd58e4534. It is easy to use deprecated namespaces without knowing you are doing so. The documentation warnings are small, and there is no compiler warning.

Proposed: stderr warnings when referencing a deprecated thing (per Rich in comments)

Patch: 706-deprecated-var-warning-patch-v2.txt



 Comments   
Comment by Rich Hickey [ 07/Jan/11 9:38 AM ]

I don't mind warning to stderr

Comment by Luke VanderHart [ 26/Oct/12 1:37 PM ]

706-deprecated-var-warning.diff adds warnings when using deprecated vars. The other three patches clean up deprecation warnings.

Comment by Andy Fingerhut [ 26/Oct/12 2:23 PM ]

Great stuff. I looked through the first patch, and didn't see anything in there that lets someone disable deprecation warnings from the command line, the way that warn-on-reflection can today be set to true with a command line option.

Is that something important to have for deprecation warnings?

Comment by Andy Fingerhut [ 28/Oct/12 4:57 PM ]

I was hoping it would be quick and easy to add source file, line, and column info to the deprecation warning messages. It isn't as easy as adding them to the format() call, because the method analyzeSymbol doesn't receive these values as args. Is this deprecation check being done in a place where it is not easy to relate it to the source file, line, and column? Could it be done in a place where that info is easily available?

Comment by Ghadi Shayban [ 29/Oct/12 1:02 AM ]

Another patch - this time to warn on loading deprecated namespaces, instead of vars. This patch requires the first one.

Re: line/column, I'll figure out how to thread the compile context through if it's desired.

Re: Compile flag. I have a patch for this also, but I'm still verifying how to invoke. How is warn-on-reflection set by command line?

Comment by Andy Fingerhut [ 29/Oct/12 1:43 AM ]

Re: Compile flag. Don't hold off on implementing a flag if you want to, but it might be worth hearing from others whether such a command line option is even desired. I was asking in hopes of eliciting such a response.

For the way that it is handled in the Clojure compiler, search for REFLECTION_WARNING_PROP and related code in Compile.java. If you invoke the Clojure compiler directly via a Java command line, use -Dclojure.compile.warn-on-reflection=true (default is false). See the recent email thread sent to Clojure Dev Google group if you want to know how to do it via ant or Maven. Link: https://mail.google.com/mail/?shva=1#label/clojure-dev/13aa0e34530196c3

There is also a separate command-line flag called compiler-options (see Compile.java) that is implemented as a map inside the compiler. It was added more recently than warn-on-reflection, and might be the preferred method to add more such options, to avoid having to continue to add more arguments to the pushThreadBindings calls done in several places.

Comment by Ghadi Shayban [ 29/Oct/12 10:36 AM ]

Thanks, Andy.

Alternately for the last ns patch, it is equivalent to call (print-method msg err), rather than binding out to err, may be more readable. I'll be glad to send that in if it's preferable.

Comment by Andy Fingerhut [ 13/Feb/13 12:38 AM ]

706-deprecated-var-warning-patch-v2.txt dated Feb 12 2013 is identical to 706-deprecated-var-warning.diff dated Oct 26 2012, except it applies cleanly to latest master.

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

For the information of anyone examining this ticket wishing for this feature, the Eastwood lint tool reports calls to deprecated Clojure functions, and also to deprecated Java methods. https://github.com/jonase/eastwood

Comment by Alex Miller [ 25/Jan/16 12:32 PM ]

I'm interested in considering this for Clojure 1.9 but I need some help getting it ready. Some comments I have on the current state: - Ticket needs to have more details about the current approach

  • I prefer *warn-on-deprecated* over *warn-on-deprecation* because it echoes the keyword you use to mark deprecated vars
  • The warning message does not tell you a location, which is grr - should be similar to the reflection messages
  • Needs tests - see test/clojure/test_clojure/compilation.clj and test/clojure/test_helper.clj (should-not-reflect) for examples
  • clojure itself has some instances of deprecated usage - it would be nice to clean those up in the patch too. That may need to be in a separate patch, depends if they are easy to fix or not. If there are cases in test/ that are actually good to leave, can set *warn-on-deprecated* to false in that namespace.
  • Current default is true - should probably be false instead to match the reflection warning default.
Comment by Vijay Kiran [ 26/Jan/16 3:10 AM ]

Alex Miller I can give this a shot.

Comment by Alex Miller [ 26/Jan/16 8:51 AM ]

Hey Vijay, Andrew Rosa assigned it to himself so please coordinate with him as he was starting to work on it.

Comment by Bozhidar Batsov [ 26/Jan/16 10:52 AM ]

Just one small remark - isn't it more common to have deprecation warnings enabled by default? One could argue they are way more important than reflection warnings, as your code might get broken in the future because you didn't notice you were using deprecated stuff.

Comment by Alex Miller [ 26/Jan/16 2:01 PM ]

Bozhidar Batsov I'm on the fence. My main hesitation in making it the default is that people will suddenly have a bunch of new warnings (which could be either good or bad I suppose). Depends how strongly we want people to care about deprecations I guess.

Comment by Phill Wolf [ 26/Jan/16 9:33 PM ]

A deprecation warning that is off by default does not address the first and primary problem given in this ticket: "It is easy to use deprecated namespaces without knowing you are doing so."

It's unlike the reflection warning. You may focus on speed at any time, at your leisure. But the eventual removal of at-risk features will be a sudden, unpleasant surprise; a warning would be helpful.

But - Suppose I wrote 300 lines of Clojure and use a million lines that come from jars. Will any deprecation problems in my own code be buried in a tsunami of warnings about those jars? Worse, the tsunami will likely linger for weeks or months, until the libraries' respective authors catch up. Inasmuch as the jars are covered (much more expediently) by 'lein ancient' and similar, I would prefer to be able to limit deprecation warnings to just my stuff, perhaps by namespace prefix if from-a-jar-or-not is inconvenient from the compiler's point of view.

Comment by Alex Miller [ 26/Jan/16 10:35 PM ]

There is a middle ground here to turn it off by default in the compiler, but to turn it on by default in the tools (like lein). But there's a reasonable chance that whatever I prefer, Rich will have a preference that overrules it when it gets to him.

I think creating more complexity around namespace prefixes is unlikely to help this ticket move forward.





[CLJ-1790] Error extending protocols to Java arrays Created: 29/Jul/15  Updated: 26/Jan/16

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

Type: Defect Priority: Major
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: compiler, protocols

Attachments: Text File 0001-CLJ-1790-emit-a-cast-to-the-interface-during-procol-.patch    
Patch: Code
Approval: Triaged

 Description   

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)

Also see: CLJ-1381

Patch: 0001-CLJ-1790emit-a-cast-to-the-interface-during-procol.patch



 Comments   
Comment by Nicola Mometto [ 06/Nov/15 3:53 PM ]

Mike Anderson does 1.8.0-beta2 fix this issue?
Alex Miller if core.matrix is still affected this must be fixed before 1.8.0 as it'd mean that direct linking is still broken

Comment by Nicola Mometto [ 06/Nov/15 6:26 PM ]

I could reproduce the bug with 1.8.0-beta2 btw

Comment by Nicola Mometto [ 06/Nov/15 7:27 PM ]

Apparently this is not a 1.8 regression.

At least 1.6 and 1.7 both manifest the same issue:

Clojure 1.6.0
user=> (defprotocol p (f [_]))
p
user=> (fn [] (f (object-array [])))

VerifyError (class: user$eval15920$fn__15921, method: invoke signature: ()Ljava/lang/Object;) Incompatible object argument for function call  user/eval15920 (form-init9183379085801704163.clj:1)

Comment by Michael Blume [ 06/Nov/15 8:24 PM ]

Do we know why core.matrix works with Clojure 1.6/1.7 then?

Comment by Nicola Mometto [ 06/Nov/15 9:09 PM ]

It doesn't.

Clojure 1.7.0
Java HotSpot(TM) 64-Bit Server VM 1.8.0_60-b27

user=> (require 'clojure.core.matrix.protocols)
nil
user=> (clojure.core.matrix.protocols/construct-matrix (object-array 1) [1])

VerifyError (class: user$eval6935, method: invoke signature: ()Ljava/lang/Object;) Incompatible object argument for function call  java.lang.Class.getDeclaredConstructors0 (Class.java:-2)
user=>

I attached a patch that fixes this issue.
It's caused by the jvm verifier understanding that the object on the stack is an array and thus can never be an instance of the protcol interface, but not being able to understant that the code path leading to the direct protocol interface method invocation can never be reached because of a branch guided by an instance check for that interface on the target

Comment by Mike Anderson [ 06/Nov/15 10:10 PM ]

Apologies, it is possible I just hadn't tested this code path thoroughly before.

It only seems to get triggered in certain circumstances, the following behaviour is interesting:

=> (let [o (identity (object-array 1))]
     (clojure.core.matrix.protocols/dimensionality o))
1
=> (let [o (object-array 1)]
     (clojure.core.matrix.protocols/dimensionality o))
VerifyError (class: clojure/core/matrix$eval17775, method: invokeStatic signature: ()Ljava/lang/Object;) Incompatible object argument for function call  java.lang.Class.getDeclaredConstructors0 (:-2)

Perhaps it only happens when the callsite has type information about the protocol parameter?

Comment by Nicola Mometto [ 07/Nov/15 4:53 AM ]

Correct, apparently the jvm verifier doesn't like situations where we have an array on the stack typed as such, and on a later codepath it is used as target for an invokeinterface even if that path is unreachable because of a previous instance check.

here's an explaination of exactly our case in pseudo bytecode:

..
 load obj // Object[]
 dup
 instanceof SomeInterface
 iftruejmp label1
 pop
 jmp end
label1:
 // here is where the verifier chokes.
 // it can figure out that the target is of type Object[] which can never be a SomeInterface
 // but it cannot figure out that this code path can never be reached because of the previous
 // instance check with jump
 // to fix this we need to insert an explicit checkcast to SomeInterface on the target
 invokeinterface SomeInterface/someMethod
end:
 return




[CLJ-1289] aset-* and aget perform poorly on multi-dimensional arrays even with type hints. Created: 01/Nov/13  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Michael O. Church Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: arrays, performance
Environment:

Clojure 1.5.1.

Dependencies: criterium


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

 Description   

Here's a transcript of the behavior. I don't know for sure that reflection is being done, but the performance penalty (about 1300x) suggests it.

user=> (use 'criterium.core)
nil
user=> (def b (make-array Double/TYPE 1000 1000))
#'user/b
user=> (quick-bench (aget ^"[[D" b 304 175))
WARNING: Final GC required 3.5198021166354323 % of runtime
WARNING: Final GC required 29.172288684474303 % of runtime
Evaluation count : 63558 in 6 samples of 10593 calls.
             Execution time mean : 9.457308 µs
    Execution time std-deviation : 126.220954 ns
   Execution time lower quantile : 9.344450 µs ( 2.5%)
   Execution time upper quantile : 9.629202 µs (97.5%)
                   Overhead used : 2.477107 ns

One workaround is to use multiple agets.

user=> (quick-bench (aget ^"[D" (aget ^"[[D" b 304) 175))
WARNING: Final GC required 40.59820310542545 % of runtime
Evaluation count : 62135436 in 6 samples of 10355906 calls.
             Execution time mean : 6.999273 ns
    Execution time std-deviation : 0.112703 ns
   Execution time lower quantile : 6.817782 ns ( 2.5%)
   Execution time upper quantile : 7.113845 ns (97.5%)
                   Overhead used : 2.477107 ns

Cause: The inlined version only applies to arity 2, and otherwise it reflects.



 Comments   
Comment by Gary Fredericks [ 08/Dec/13 9:28 PM ]

A glance at the source makes it obvious that the hypothesis is correct – the inlined version only applies to arity 2, and otherwise it reflects.

I thought this would be as simple as converting the inline function to be variadic (using reduce), but after trying it I realized this is tricky as you have to generate the correct type hints for each step. E.g., given ^"[[D" the inline function needs to type-hint the intermediate result with ^"[D". This isn't difficult if we're just dealing with strings that begin with square brackets, but I don't know for sure that those are the only possibilities.

Comment by Yaron Peleg [ 13/Feb/14 4:44 AM ]

Bump. I just got bitten bad by this.

There are two seperate issues here:
1) (aget 2d-array-doubles 0 0 ) doesn't emit a reflection warning.
2) It seems like the compiler has enough information to avoid the reflective call here.

Note this gets exp. worse as number of dimensions grows, i.e (get doubles3d 0 0 0)
will be 1M slower, etc' Not true, unless you iterate over all elements. it's
simply n_dims*1000x per lookup.

Nasty surprise, especially considering you often go to primitive arrays for speed,
and a common use case is an inner loop(s) that iterate over arrays.

Comment by Gary Fredericks [ 13/Feb/14 7:08 AM ]

I can probably take a stab at this.

Comment by Gary Fredericks [ 13/Feb/14 8:34 PM ]

I think the reflection warning problem is pretty much impossible to solve without changing code elsewhere in the compiler, because the reflection done in aget is a different kind than normal clojure reflection – it's explicitly in the function body rather than emitted by the compiler. Since the compiler isn't emitting it, it doesn't reasonably know it's even there. So even if aget is fixed for other arities, you still won't get the warning when it's not inlined.

I can imagine some sort of metadata that you could put on a function telling the compiler that it will reflect if not inlined. Or maybe a more generic not-inlined warning?

The global scope of adding another compiler flag seems about balanced by the seriousness of array functions not being able to warn on reflection.

Comment by Gary Fredericks [ 13/Feb/14 8:52 PM ]

Attached CLJ-1289-p1.patch which simply inlines variadic calls to aget. It assumes that if it sees a :tag on the array arg that is a string beginning with [, it can assume that the return value from one call to aget can be tagged with the same string with the leading [ stripped off.

I'm not a jvm expert, but having read through the spec a little bit I think this is a reasonable assumption.

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

I think this probably is actually true, but a more official way to ask that question would be to get the array class and ask for Class.getComponentType() (and less janky than string munging).

Comment by Gary Fredericks [ 14/Feb/14 3:40 PM ]

How would you get the array class based on the :tag type hint?

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

I see (-> s (Class/forName) (.getComponentType) (.getName)) does the same thing – is that route preferred, or is there another one?





[CLJ-1599] Add a reset! that returns old value Created: 24/Nov/14  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Steven Yi Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: atom

Attachments: File get-and-set.diff    
Patch: Code
Approval: Triaged

 Description   

DESCRIPTION

This patch adds the equivalent of reset!, here called get-and-set!, to core to allow getting the last value from an atom and setting it to a new value. This is useful for atomically draining an atom of its value and setting to a new value. The implementation delegates to Java's AtomicReference.getAndSet().

The equivalent operation in Clojure code would be:

(defn get-and-set! [atm newv]
(loop [oldv @atm]
(if (compare-and-set! atm oldv newv)
oldv
(recur @atm))))

This is close to a 1:1 translation of the Java code in sun.misc.Unsafe's getAndSetObject, used by AtomicReference (as of current JDK9 source code).

APPLICATIONS

  • User may want to check if an operation has occurred before by using an atom as a flag. I.e.,

(def has-run-once (atom false))
...
(when-not (get-and-set! has-run-once true)
(do something))

  • User may want to use an atom similarly to a java.util.concurrent.LinkedTransferQueue, for the case of pairing up adds by writers and drainTo by readers:

Thread 1: (swap! atm conj item1)
Thread 2: (swap! atm conj item2)
Thread 3: (let [new-vals (get-and-set! atm [])]
(do-something new-vals))

ALTERNATIVES

  • For case of atom as flag, user can use existing compare-and-set!:

(def has-run-once (atom false))
...
(when-not (compare-and-set! has-run-once false true)
(do something))

Argument: get-and-set! is a little clearer in intent as it is using the value of the atom, rather than the success of the cas operation. Also, this would not be applicable to situations where the value stored is not a boolean.

  • User can just go ahead and use LinkedTransferQueue.

Argument: User not fluent in Java may not be readily able to use this.

==

Argument for: This seems like a sufficiently primitive operation to include in core for atoms. I am unsure of the rationale, but assume it was vetted to include into Java's AtomicReference for a reason. Also, if users are using atoms and have this available, they are less likely to try to do this incorrectly, such as:

(let [vals @some-atom]
(reset! some-atom [])
(do-something-with vals))

Argument against: This may not be sufficiently primitive enough to include in core. Users have a workaround to implement the get-and-set! operation in user-code as given above.

Note: This request is similar to CLJ-1454 (http://dev.clojure.org/jira/browse/CLJ-1454), but differs in that this is not a swap! operation that accepts an IFn argument. Also, I looked to add a test in test/clojure/test_clojure/atoms.clj, but saw that the other operations weren't tested. (I assume this is due to the other operations delegating to AtomicReference and hence not deemed test-worthy.)



 Comments   
Comment by Michael Blume [ 02/Mar/15 5:03 PM ]

I tend to wind up inevitably adding this to my projects, be nice to have it in core

Comment by Alex Miller [ 02/Mar/15 7:27 PM ]

So CLJ-1454 is swap! and return old and CLJ-1599 is reset! and return old?

Comment by Steven Yi [ 02/Mar/15 7:48 PM ]

Yes, I think that's an accurate interpretation of the two tickets.





[CLJ-1451] Add take-until Created: 20/Jun/14  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Alexander Taggart Assignee: Unassigned
Resolution: Unresolved Votes: 5
Labels: None

Attachments: Text File 0001-CLJ-1451-add-take-until.patch     Text File 0002-CLJ-1451-add-drop-until.patch     Text File 0003-let-take-until-and-drop-until-return-transducers.patch     Text File CLJ-1451-drop-until.patch     Text File CLJ-1451-take-until.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Discussion: https://groups.google.com/d/topic/clojure-dev/NaAuBz6SpkY/discussion

It comes up when I would otherwise use (take-while pred coll), but I need to include the first item for which (pred item) is false.

(take-while pos? [1 2 0 3]) => (1 2)
(take-until zero? [1 2 0 3]) => (1 2 0)

Impl:

(defn take-until
  "Returns a lazy sequence of successive items from coll until
  (pred item) returns true, including that item. pred must be
  free of side-effects."
  [pred coll]
  (lazy-seq
    (when-let [s (seq coll)]
      (if (pred (first s))
        (cons (first s) nil)
        (cons (first s) (take-until pred (rest s)))))))

List of other suggested names: take-upto, take-to, take-through. It is not easy to find something in English that is short and unambiguously means "up to and including". That is one of the dictionary definitions for "through".



 Comments   
Comment by Alex Miller [ 20/Jun/14 10:21 AM ]

Patch welcome (w/tests).

Comment by Alexander Taggart [ 20/Jun/14 2:00 PM ]

Impl and tests for take-until and drop-until, one patch for each.

Comment by Jozef Wagner [ 20/Jun/14 3:01 PM ]

Please change :added metadata to "1.7".

Comment by Alexander Taggart [ 20/Jun/14 3:12 PM ]

Updated to :added "1.7"

Comment by John Mastro [ 21/Jun/14 6:26 PM ]

I'd like to propose take-through and drop-through as alternative names. I think "through" communicates more clearly how these differ from take-while and drop-while.

Comment by Andy Fingerhut [ 06/Aug/14 2:27 PM ]

Both patches CLJ-1451-drop-until.patch and CLJ-1451-take-until.patch dated Jun 20 2014 no longer apply cleanly to latest Clojure master due to some changes committed earlier today. I haven't checked whether they are straightforward to update, but would guess that they merely require updating a few lines of diff context.

See the section "Updating stale patches" at http://dev.clojure.org/display/community/Developing+Patches for suggestions on how to update patches.

Comment by Ghadi Shayban [ 13/Nov/14 11:19 PM ]

Would be nice to cover the transducer case too.

Comment by Michael Blume [ 13/Nov/14 11:54 PM ]

rerolled patches

Comment by Michael Blume [ 14/Nov/14 12:11 AM ]

Covered transducer case =)

Comment by Michael Blume [ 14/Nov/14 12:12 AM ]

Actually I like take/drop-through as well

Comment by Ghadi Shayban [ 16/Nov/14 12:41 PM ]

Michael, no volatile/state is necessary in the transducer, like take-while. Just wrap in 'reduced to terminate

Comment by Michael Blume [ 17/Dec/14 6:47 PM ]

a) you're clearly right about take-until

b) seriously I don't know what I was thinking with my take-until implementation, I'm going to claim lack of sleep.

c) I'm confused about how to make drop-until work without a volatile

Comment by Michael Blume [ 18/Dec/14 1:52 AM ]

Ghadi and I discussed this and couldn't think of a use case for drop-until. Are there any?

Here's a new take-until patch, generative tests included.

Open questions:

Is take-until a good name? My biggest concern is that take-until makes it sound like a slight modification of take, but this function reverses the sense of the predicate relative to take.

Comment by Andy Fingerhut [ 08/Jan/15 6:06 PM ]

Michael, while JIRA can handle multiple attachments for the same ticket with the same name, it can get confusing for people trying to determine which one with the same name is meant. Could you remove or rename one of your identically-named attachments? Instructions for deleting patches are in the "Removing patches" section on this wiki page: http://dev.clojure.org/display/community/Developing+Patches





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

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

Type: Enhancement Priority: Critical
Reporter: Abe Fettig Assignee: Unassigned
Resolution: Unresolved Votes: 5
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.

Comment by Didier A. [ 20/Nov/15 7:11 PM ]

I'm affected by this bug too. A function in a namespace calls a static Java variable which is initialized in place. Another namespace which is genclassed calls that function. Now at compile time, the static java is initialized and it makes building fail, because that static java initialization needs resources which don't exist on the build machine.





[CLJ-401] Add seqable? predicate Created: 13/Jul/10  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 7
Labels: None

Approval: Triaged

 Description   

Many people have found a need for this function and one exists in clojure.core.incubator that is sometimes used and/or copied elsewhere:

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

This predicate would be valuable to have as it is not a simple check on Seqable since RT.seq() covers a number of additional cases. Alternatively, there could be a protocol for this that could be extended to both Seqable as well as other supported Java use cases turning this into a satisfies? check.

Old prior discussion (although this also comes up regularly on #clojure):



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

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

Comment by Jeremy Heiler [ 26/Jul/14 5:37 PM ]

A reference to the implementation in contrib: https://github.com/clojure/clojure-contrib/blob/master/modules/core/src/main/clojure/clojure/contrib/core.clj#L78

It seems like that the only thing that is inconsistent with RT.seqFrom is that seqable? checks for String instead of CharSequence.

Comment by Alex Miller [ 03/Aug/15 10:11 AM ]

In the proposed patch referenced in the ticket above, if seqable? could be used in place of sequential? flatten could be more powerful and work with maps/sets/java collections. Here's how it would look:

(defn flatten [coll] 
  (lazy-seq 
    (when-let [coll (seq coll)] 
      (let [x (first coll)] 
        (if (seqable? x) 
          (concat (flatten x) (flatten (next coll))) 
          (cons x (flatten (next coll))))))))

And an example:

user=> (flatten #{1 2 3 #{4 5 {6 {7 [8 9 10 #{11 12 (java.util.ArrayList. [13 14 15]) (int-array [16 17 18])}]}}}}) 
(1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18)




[CLJ-1117] partition docstring should be more explicit about dropped or partial trailing elements Created: 29/Nov/12  Updated: 26/Jan/16

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

Type: Enhancement Priority: Minor
Reporter: Timothy Baldridge Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: docstring
Environment:

OS X, 10.8


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

 Description   

The doc for partition states "In case there are not enough padding elements, return a partition with less than n items." However, the behavior of this function is as follows:

user=> (partition 3 (range 10))
((0 1 2) (3 4 5) (6 7 8))
user=> (partition 4 (range 10))
((0 1 2 3) (4 5 6 7))

Proposed: The docstring should be updated to make it clear that not providing a pad means that items are dropped, and to also see partition-all.

Patch: clj-1117.patch



 Comments   
Comment by Andy Fingerhut [ 29/Nov/12 2:15 PM ]

That would be a potentially breaking change for some people's code that uses partition. partition-all behaves as you wish.

Also, your concern with the documentation is for when there are padding elements specified as an argument, but your examples don't specify any padding elements.

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

I agree, but I think the docs should then explicitly state: "if no padding is given, not all input elements may be returned in the output partitions" or something to that line.

Comment by Andy Fingerhut [ 29/Nov/12 4:43 PM ]

More precise documentation of current behavior is always welcome in my opinion.

Comment by Gabriel Horner [ 17/May/13 10:14 AM ]

I've uploaded a patch that calls out when and how partition drops tail elements:
"If a pad collection is not supplied, any tail elements that remain from dividing the input collection length by n will not be included in a partition."

Comment by Andrea Richiardi [ 11/Apr/15 10:38 AM ]

I was a little bit baffled by this as well...in particular, I could not figure out why (partition 2 3 [1 2 3 4 5 6 7]) is not '((1 2) (4 5) (7))...it would really be better to rewrite that comment.





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

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

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

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

 Description   

Whereas

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

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

and

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

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

[1] The matter was discussed on Google Groups:

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

with a reference to an earlier thread

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

[2] CLJ-82 and the 2009 message thread



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

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

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

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

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

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

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

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

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

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





[CLJ-308] protocol-ize with-open Created: 21/Apr/10  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: io

Attachments: Text File 0001-Added-ClosableResource-protocol-for-with-open.patch    
Patch: Code
Approval: Triaged

 Description   

Good use (and documentation example) of protocols: make with-open aware of a Closable protocol for APIs that use a different close convention. See http://groups.google.com/group/clojure/browse_thread/thread/86c87e1fc4b1347c



 Comments   
Comment by Assembla Importer [ 24/Aug/10 4:39 PM ]

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

Comment by Tassilo Horn [ 23/Dec/11 5:11 AM ]

Added a CloseableResource protocol and extended it on java.io.Closeable (implemented by all Readers, Writers, Streams, Channels, Sockets). Use it in with-open.

All tests pass.

Comment by Tassilo Horn [ 23/Dec/11 7:14 AM ]

Seems to be related to Scopes (http://dev.clojure.org/jira/browse/CLJ-2).

Comment by Tassilo Horn [ 08/Mar/12 3:59 AM ]

Updated patch.

Comment by Andy Fingerhut [ 02/Apr/12 12:11 PM ]

Patch 0001-Added-ClosableResource-protocol-for-with-open.patch dated 08/Mar/12 applies, builds, and tests cleanly on latest master as of Apr 2 2012. Tassilo has signed a CA.

Comment by Tassilo Horn [ 13/Apr/12 11:23 AM ]

Updated patch to apply cleanly against master again.

Comment by Brandon Bloom [ 22/Jul/14 9:00 PM ]

I looked up this ticket because I ran in to a reflection warning: with-open does not hint it's binding with java.io.Closeable

Some feedback on the patch:

1) This is a breaking change for anyone relying on the close method to be duck-typed.

2) CloseableResource is a bit long. clojure.core.protocols.Closeable is plenty unambiguous.

3) Rather than extending CloseableResource to java.io.Closeable, you can use the little known (undocumented? unsupported?) :on-interface directive:

(defprotocol Closeable
  :on-interface java.io.Closeable
  (close [this]))

That would perform much better than the existing patch.

Comment by Tassilo Horn [ 23/Jul/14 7:12 AM ]

Hi Brandon, two questions:

Could 1) be circumvented somehow by providing a default implementation somehow? I guess the protocol could be extended upon Object with implementation (.close this), but that would give a reflection warning since Object has no close method. Probably one could extend upon Object and in the implementation search a "close" method using java.lang.reflect and throw an exception if none could be found?

Could you please tell me a bit more about the :on-interface option? How does that differ from extend? And how do I add the implementation, i.e., (.close this) with that option?





[CLJ-1239] faster, more flexible dispatch for clojure.walk Created: 29/Jul/13  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Unresolved Votes: 8
Labels: walk

Attachments: Text File 0001-CLJ-1239-protocol-dispatch-for-clojure.walk.patch     Text File 0002-CLJ-1239-protocol-dispatch-for-clojure.walk.patch    
Patch: Code
Approval: Triaged

 Description   

The conditional dispatch in clojure.walk is slow and not open to extension. Prior to CLJ-1105 it did not support records.

Approach: Reimplement clojure.walk using protocols. The public API does not change. Users can extend the walk protocol to other types (for example, Java collections) if desired. As in CLJ-1105, this version of clojure.walk supports records.

Patch: 0002-CLJ-1239-protocol-dispatch-for-clojure.walk.patch

Performance: My tests indicate this is 1.5x-2x the speed of the original clojure.walk. See https://github.com/stuartsierra/clojure.walk2 for benchmarks.

Risks: This approach carries some risk of breaking user code that relied on type-specific behavior of the old clojure.walk. When running the full Clojure test suite, I discovered (and fixed) some breakages that did not show up in clojure.walk's unit tests. See, for example, commit 730eb75 in clojure.walk2



 Comments   
Comment by Vjeran Marcinko [ 19/Oct/13 12:32 PM ]

It looks, as it is now, that walking the tree and replacing forms doesn't preserve original meta-data contained in data structures.

Comment by Andy Fingerhut [ 23/Nov/13 1:11 AM ]

Patch 0001-CLJ-1239-protocol-dispatch-for-clojure.walk.patch no longer applies cleanly to latest Clojure master since the patch for CLJ-1105 was committed on Nov 22, 2013. From the description, it appears the intent was either that patch or this one, not both, so I am not sure what should happen with this patch, or even this ticket.

Comment by Alex Miller [ 23/Nov/13 1:52 AM ]

This patch and ticket are still candidates for future release.

Comment by Stuart Sierra [ 20/Dec/13 9:14 AM ]

Added new patch that applies on latest master after CLJ-1105.

Comment by Chouser [ 27/Feb/14 10:26 AM ]

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

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

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

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

Here's an implementation that doesn't suffer from that problem, though it does scary class name munging instead: https://github.com/LonoCloud/synthread/blob/a315f861e04fd33ba5398adf6b5e75579d18ce4c/src/lonocloud/synthread/impl.clj#L66

Perhaps we could add to the defrecord abstraction to support well the kind of things that synthread code is doing clumsily, and then walk could take advantage of that.

Comment by Alex Miller [ 27/Feb/14 2:11 PM ]

@Chouser, can you file a new ticket related to this? It's hard to manage work on something from comments on a closed ticket.

Comment by Alex Miller [ 27/Feb/14 3:54 PM ]

@Chouser - Never mind! I was thinking this was the change that went into 1.6. Carry on.

Comment by Nicola Mometto [ 27/Feb/14 5:17 PM ]

Alex, for what it matters clojure-1.6.0 after CLJ-1105 exibits the same behaviour as described by Chouser for this patch





[CLJ-1454] Add a swap! that returns old value Created: 28/Jun/14  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Philip Potter Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: atom

Approval: Triaged

 Description   

Sometimes, when mutating an atom, it's desirable to know what the value before the swap happened. The existing swap! function returns the new value, so is unsuitable for this use case. Currently, the only option is to roll your own using a loop and compare-and-set!

An example of this would be where the atom contains a PersistentQueue and you want to atomically remove the head of the queue and process it: if you run (swap! a pop), you have lost the reference to the old head of the list so you can't process it.

It would be good to have a new function swap-returning-old! which returned the old value instead of the new.



 Comments   
Comment by Philip Potter [ 28/Jun/14 4:00 PM ]

Overtone already defines functions like this in overtone.helpers.ref, which get used by overtone.libs.event. These return both the old and the new value, although in all existing use cases only the old value gets used.

flatland/useful defines a trade! fn which returns the old value, although the implementation is less clean than a compare-and-set! based solution would be.

Comment by Philip Potter [ 29/Jun/14 6:23 AM ]

Chris Ford suggested "swap-out!" as a name for this function. I definitely think "swap-returning-old!" isn't the ideal name.

Comment by Jozef Wagner [ 30/Jun/14 1:33 AM ]

I propose a switch! name. The verb switch is defined as "substitute (two items) for each other; exchange.", and as you get the old value back, it evokes slightly the exchange of items.

Comment by Philip Potter [ 30/Jun/14 3:03 AM ]

Medley also has a deref-swap! which does the same thing.

Comment by Alex Miller [ 30/Jun/14 8:20 AM ]

I think deref-swap! seems like a morally equivalent name to Java's AtomicReference.getAndSet() which is the same idea.

Comment by Philip Potter [ 30/Jun/14 1:19 PM ]

Funny you say that Alex, because prismatic/plumbing defines a get-and-set! (also defined by other projects), equivalent to deref-reset! in medley. Plumbing also defines swap-pair! which returns both old and new values, like the overtone fn, although once again the only usage I can find only uses the old value.

Comment by Alex Miller [ 30/Jun/14 3:37 PM ]

I think it's important to retain the notion that you are not switching/exchanging values but applying the update model of applying a function to the old value to produce the new value. I don't even particularly like "swap!" as I think that aspect is lost in the name (alter and alter-var-root are better). I like that "deref-swap!" combines two words with existing connotations and orders them appropriately.

Comment by Timothy Baldridge [ 30/Jun/14 3:43 PM ]

except that that naming doesn't fit well compared to functions like nfirst which are defined as (comp next first). This function is not (comp deref swap!).





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

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

Type: Enhancement Priority: Critical
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 12
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?

Comment by Francis Avila [ 10/Sep/15 9:26 AM ]

Another source of ambiguity I see is that it's unclear whether the first colon of a keyword is the first character of the keyword (and therefore of the symbol) or whether it is something special and the spec really describes what happens from the second character onward. This matters because the specification for a keyword is (in both edn and reader specs) given in terms of differences from symbols. I think many of the strange keyword edge cases (including legality of :1 vs :a/1) stem from this ambiguity, and different tickets/patches seem to choose one or the other underlying assumption. See this comment for more examples.

Possibly we can use tagged literals for keywords and symbols to create or print these forms when they are not readable and simplify the reader spec for their literal forms. E.g. instead of producing complicated parse rules to ensure clojure.core// or :1 are legal, just make the literal form simple and have users write something like #sym["clojure.core" "/"] or #kyw "1" (and have the printer print these) when they hit these edge cases.

Comment by Alex Miller [ 10/Sep/15 9:44 AM ]

I would say : (and : are syntactic markers and the spec describes the characters following it. But I agree it would be nice for this to be more explicit. The (incorrect) regex in LispReader does not help either.

The tagged literal idea is an interesting alternative to the | | syntax that has been reserved for possible future support for invalid characters in keywords and symbols. But I think the idea is out of scope for this ticket, which is really about clarifying the spec.





[CLJ-1522] Enhance multimethods metadata Created: 08/Sep/14  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 17
Labels: metadata

Approval: Triaged

 Description   

I think that multimethod metadata can be extended a bit with some property indicating the var in question is referring to a multimethod (we have something similar for macros) and some default arglists property.

I'm raising this issue because as a tool writer (CIDER) I'm having hard time determining if something is a multimethod (I have to resort to code like (instance? clojure.lang.MultiFn obj) which is acceptable, but not ideal I think (compared to macros and special forms)). There's also the problem that I cannot provide the users with eldoc (function signature) as it's not available in the metadata (this issue was raised on the mailing list as well https://groups.google.com/forum/#!topic/clojure/crje_RLTWdk).

I feel that we really have a problem with the missing arglist and we should solve it somehow. I'm not sure I'm suggesting the best solution and I'll certainly take any solution.



 Comments   
Comment by Bozhidar Batsov [ 09/Sep/14 4:24 AM ]

Btw, I failed to mention this as I thought it was obvious, but I think we should use the dispatch function's arglist in the multimethod metadata.





[CLJ-1298] Add more type predicate fns to core Created: 21/Nov/13  Updated: 26/Jan/16

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

Type: Enhancement Priority: Critical
Reporter: Alex Fowler Assignee: Unassigned
Resolution: Unresolved Votes: 18
Labels: None

Approval: Triaged

 Description   

Add more built-in type predicates:

1) Definitely missing: (atom? x), (ref? x), (deref? x), (named? x), (map-entry? x), (lazy-seq? x), (boolean?).
2) Very good to have: (throwable? x), (exception? x), (pattern? x).

The first group is especially important for writing cleaner code with core Clojure.



 Comments   
Comment by Alex Miller [ 21/Nov/13 8:42 AM ]

In general many of the existing predicates map to interfaces. I'm guessing these would map to checks on the following types:

atom? = Atom (final class)
ref? = IRef (interface)
deref? = IDeref (interface)
named? = Named (interface, despite no I prefix)
map-entry? = IMapEntry (interface)
lazy-seq? = LazySeq (final class)

throwable? = Throwable
exception? = Exception, but this seems less useful as it feels like the right answer when you likely actually want throwable?
pattern? = java.util.regex.Pattern

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

Yes, they do, and sometimes the code has many checks like (instance? clojure.lang.Atom x). Ok, you can write a little function (atom? x) but it has either to be written in all relevant namespaces or required/referred there from some extra namespace. All this is just a burden. For example, we have predicates like (var? x) or (future? x) which too map to Java classes, but having them abbreviated often makes possible to write a cleaner code.

I feel the first group to be especially significant for it being about core Clojure concepts like atom and ref. Having to fall to manual Java classes check to work with them feels inorganic. Of course we can, but why then do we have (var? x), (fn? x) and other? Imagine, for example:

(cond
(var? x) (...)
(fn? x) (...)
(instance? clojure.lang.Atom x) (...)
(or (instance? clojure.lang.Named x) (instance? clojure.lang.LazySeq x)) (...))

vs

(cond
(var? x) (...)
(fn? x) (...)
(atom? x) (...)
(or (named? x) (lazy-seq? x)) (...))

The second group is too, essential since these concepts are fundamental for the platform (but you're right with the (exception? x) one).

Comment by Alex Fowler [ 22/Nov/13 6:35 AM ]

Also, obviously I missed the (boolean? x) predicate in the original post. Did not even guess it is absent too until I occasionally got into it today. Currently the most clean way we have is to do (or (true? x) (false? x)). Needles to say, it looks weird next to the present (integer? x) or (float? x).

Comment by Brandon Bloom [ 22/Jul/14 1:02 AM ]

Predicates for core types are also very useful for portability to CLJS.

Comment by Brandon Bloom [ 22/Jul/14 1:05 AM ]

I'd be happy to provide a patch for this, but I'd prefer universal interface support where possible. Therefore, this ticket, in my mind, is behind http://dev.clojure.org/jira/browse/CLJ-803 etc.

Comment by Alex Miller [ 22/Jul/14 6:12 AM ]

I don't think it's worth making a ticket for this until Rich has looked at it and determined which parts are wanted.

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

Someone asked about a boolean? predicate, so throwing this one on the list...

Comment by Reid McKenzie [ 02/Dec/14 4:51 PM ]

uuid? maybe. UUIDs have a bit of a strange position in that we have special printer handling for them built into core implying that they are intentionally part of Clojure, but there is no ->UUID constructor and no functions in core that operate on them so I could see this one being specifically declined.

Comment by Brandon Bloom [ 03/May/15 2:50 PM ]

This has been troubling me again with my first cljc project. So, I've added a whole bunch of tickets (with patches!) for individual predicates in both CLJ and CLJS.

Comment by Alex Miller [ 03/May/15 5:35 PM ]

As I said above, I don't want to mess with specific patches or tickets on this until Rich gets a look at this and we decide which stuff should and should not be included. So I'm going to ignore your other tickets for now...

Comment by Nicola Mometto [ 24/Nov/15 4:08 PM ]

map-entry? is included since 1.8





[CLJ-304] clojure.repl/source does not work with deftype Created: 20/Apr/10  Updated: 26/Jan/16

Status: In Progress
Project: Clojure
Component/s: None
Affects Version/s: None
Fix Version/s: None

Type: Enhancement Priority: Major
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 20
Labels: repl

Approval: Triaged

 Description   

clojure.repl/source does not work on a deftype

user> (deftype Foo [a b])
user.Foo
user> (source Foo)
Source not found

Cause: deftype creates a class but not a var so no file/line info is attached anywhere.

Approach:

Patch:

Screened by:



 Comments   
Comment by Assembla Importer [ 24/Aug/10 4:38 PM ]

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

Comment by Assembla Importer [ 24/Aug/10 4:38 PM ]

chouser@n01se.net said: That's a great question. get-source just needs a file name and line number.

If IMeta were a protocol, it could be extended to Class. That implementation could look for a "well-known" static field, perhaps? __clojure_meta or something? Then deftype would just have to populate that field, and get-source would be all set.

Does that plan have any merit? Is there a better place to store a file name and line number?

Comment by Assembla Importer [ 24/Aug/10 4:38 PM ]

stu said: Seems like a reasonable idea, but this is going to get back-burnered for now, unless there is a dire use case we have missed.

Comment by Gary Trakhman [ 19/Feb/14 10:31 AM ]

I could use this for cider's file/line jump-around mechanism as well.

With records, I can work around it by deriving and finding the corresponding constructor var, but it's a bit nasty.

Comment by Bozhidar Batsov [ 03/Mar/14 6:37 AM ]

I'd also love to see this fixed.

Comment by Andy Fingerhut [ 03/Mar/14 8:33 AM ]

Bozhidar, voting on a ticket (clicking the Vote link in the right of the page when viewing the ticket) can help push it upwards on listings of tickets by # of votes.

Comment by Bozhidar Batsov [ 19/Sep/14 1:17 PM ]

Andy, thanks for the pointer. They should have made this button much bigger, I hadn't noticed it all until now.

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

If someone did some work on this (like a patch), I would push it harder.





[CLJ-1629] Improve error message when defn form omits parameter declaration Created: 29/Dec/14  Updated: 26/Jan/16

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

Type: Enhancement Priority: Major
Reporter: Sanel Zukan Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs
Environment:

Reproducible on all platforms and all clojure versions.


Approval: Triaged

 Description   

When defn form is malformed, Clojure compiler will report meaningless error and in combination with function body, can cause really bad experience. Here is the sample:

(defn foo
  "This is docstring."
  (let [i 1]
    (+ i 1)))

It will report:

IllegalArgumentException Parameter declaration "let" should be a vector  clojure.core/assert-valid-fdecl (core.clj:7123)

However, if is written:

(defn foo "bla")

error report makes more sense:

IllegalArgumentException Parameter declaration missing  clojure.core/assert-valid-fdecl (core.clj:7107)


 Comments   
Comment by Michael Blume [ 29/Dec/14 1:39 PM ]

I don't think this is really meaningless – if you replace the symbol let with a vector, say, [i], you get a perfectly valid function definition

(defn foo
  "This is docstring."
  ([i] [i 1]
    (+ i 1)))
Comment by Sanel Zukan [ 29/Dec/14 2:41 PM ]

Yes and maybe make sense for this case. But in general, the report is misleading for common defn forms (how often you will see function definitions written this way, unless you want multi-arity function) and should have the same report as for second sample; in both cases it is the same cause.





[CLJ-1107] 'get' should throw exception on non-Associative argument Created: 13/Nov/12  Updated: 26/Jan/16

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

Type: Enhancement Priority: Major
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Unresolved Votes: 16
Labels: checkargs

Attachments: Text File 0001-CLJ-1107-Throw-exception-for-get-called-on-unsupport.patch     Text File 0003-CLJ-1107-Throw-exception-for-get-on-unsupported-type.patch     Text File clj-1107-throw-on-unsupported-get-v4.patch    
Patch: Code and Test
Approval: Triaged

 Description   

The implementation of clojure.core/get returns nil if its argument is not an associative collection.

This behavior can obscure common programmer errors such as:

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

(:foo a)   ; forgot to deref a
;;=> nil

Calling get on something which is neither nil nor an Associative collection is almost certainly a bug, and should be indicated by an exception.

CLJ-932 was accepted as a similar enhancement to clojure.core/contains?

Patch: 0003-CLJ-1107-Throw-exception-for-get-on-unsupported-type.patch

Approach: Throw IllegalArgumentException as final fall-through case in RT.getFrom instead of returning nil.

Also see: CLJ-969



 Comments   
Comment by Andy Fingerhut [ 24/May/13 12:31 PM ]

Patch clj-1107-throw-on-get-for-unsupported-types-patch-v2.txt dated May 24 2013 is identical to 0001-CLJ-1107-Throw-exception-for-get-called-on-unsupport.patch dated Nov 13 2012, except it applies cleanly to latest master. A recent commit for CLJ-1099 changed many IllegalArgumentException occurrences to Throwable in the tests, which is the only thing changed in this updated patch.

Comment by Andy Fingerhut [ 30/Jan/14 5:01 PM ]

Patch clj-1107-throw-on-get-for-unsupported-types-patch-v2.txt applied cleanly to latest Clojure master as of Jan 23 2014, but no longer does with commits made to Clojure between then and Jan 30 2014. I have not checked to see how difficult or easy it may be to update this patch.

Comment by Stuart Sierra [ 11/Feb/14 7:23 AM ]

New patch 0003-CLJ-1107-Throw-exception-for-get-on-unsupported-type.patch created from master at 5cc167a.

Comment by Andy Fingerhut [ 26/Mar/14 11:55 AM ]

Patch clj-1107-throw-on-unsupported-get-v4.patch dated Mar 26 2014 is identical to Stuart Sierra's patch 0003-CLJ-1107-Throw-exception-for-get-on-unsupported-type.patch, and retains his authorship. The only difference is in one line of diff context required in order to make it apply cleanly to latest master.

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

This would be a breaking change

Comment by Stuart Sierra [ 17/Jun/14 6:59 PM ]

Arguably so was CLJ-932 (contains?), which did "break" some things that were already broken.

This is a more invasive change than CLJ-932, but I believe it is more likely to expose hidden bugs than to break intentional behavior.

Comment by Andy Sheldon [ 07/Oct/14 5:40 AM ]

Is it more idiomatic to use "({:a 1}, :a)" and a safe replacement to boot? E.g. could you mass replace "(get " with "(" in a code base, in order to find bugs? I am still learning the language, and not young anymore, and couldn't reliably remember the argument order. So, I found it easier to avoid (get) with maps anyways. Without it I can put the map first or second.





[CLJ-1159] Improve docstring of clojure.java.io/delete-file to be clearer about intent of silently arg Created: 10/Feb/13  Updated: 26/Jan/16

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

Type: Enhancement Priority: Minor
Reporter: AtKaaZ Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs, io
Environment:

any


Attachments: Text File clj-1159.patch    
Patch: Code
Approval: Prescreened

 Description   

clojure.java.io/delete-file docstring takes an optional second arg `silently` and says:

Raise an exception if it fails unless silently is true.

This is confusing. The intent is that in the case of a failure, it will by default throw an exception. Or if the optional second arg is supplied and truthy, it will be returned instead.

Initially reported: https://groups.google.com/d/msg/clojure/T9Kvr0IL0kg/wcKBfR9w_1sJ

Approach: Make docstring sentence clearer per Stu's suggestion in comments:

If silently is nil or false, raise an exception on failure, else return the value of silently.

Patch: clj-1159.patch



 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-1886] AOT compilation can cause java.lang.NoSuchFieldError: __thunk__0__ Created: 25/Jan/16  Updated: 26/Jan/16

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

Type: Defect Priority: Minor
Reporter: Ryan Fowler Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: aot


 Description   

In some very specific situation that I don't understand, the aot compiler can create class files with an inconsistent idea of a field called _thunk0_.

I've created a project at https://github.com/ryfow/weird-aot that reproduces the problem with `lein run`.

The ingredients for reproduction seem to be slf4j-timbre, tools.analyzer, and core.async.

I suspect that slf4j-timbre being aot compiled but not directly loaded by clojure code is a factor.

Note that the weird-aot timbre version differs from the version compiled in slf4j-timbre.

It's unclear to me why tools.analyzer and core.async are required to exhibit the problem.

Here's the stacktrace I get when I run `lein run` on the weird-aot project.

Exception.txt
Exception in thread "main" java.lang.NoSuchFieldError: __thunk__0__, compiling:(/private/var/folders/2q/tk7cywk93217_d4pxn_5kft40000gn/T/form-init7490372454812250103.clj:1:125)
        at clojure.lang.Compiler.load(Compiler.java:7239)
        at clojure.lang.Compiler.loadFile(Compiler.java:7165)
        at clojure.main$load_script.invoke(main.clj:275)
        at clojure.main$init_opt.invoke(main.clj:280)
        at clojure.main$initialize.invoke(main.clj:308)
        at clojure.main$null_opt.invoke(main.clj:343)
        at clojure.main$main.doInvoke(main.clj:421)
        at clojure.lang.RestFn.invoke(RestFn.java:421)
        at clojure.lang.Var.invoke(Var.java:383)
        at clojure.lang.AFn.applyToHelper(AFn.java:156)
        at clojure.lang.Var.applyTo(Var.java:700)
        at clojure.main.main(main.java:37)
Caused by: java.lang.NoSuchFieldError: __thunk__0__
        at clojure.tools.analyzer.jvm.utils__init.load(Unknown Source)
        at clojure.tools.analyzer.jvm.utils__init.<clinit>(Unknown Source)
        at java.lang.Class.forName0(Native Method)
        at java.lang.Class.forName(Class.java:340)
        at clojure.lang.RT.classForName(RT.java:2154)
        at clojure.lang.RT.classForName(RT.java:2163)
        at clojure.lang.RT.loadClassForName(RT.java:2182)
        at clojure.lang.RT.load(RT.java:436)
        at clojure.lang.RT.load(RT.java:412)
        at clojure.core$load$fn__5448.invoke(core.clj:5866)
        at clojure.core$load.doInvoke(core.clj:5865)
        at clojure.lang.RestFn.invoke(RestFn.java:408)
        at clojure.core$load_one.invoke(core.clj:5671)
        at clojure.core$load_lib$fn__5397.invoke(core.clj:5711)
        at clojure.core$load_lib.doInvoke(core.clj:5710)
        at clojure.lang.RestFn.applyTo(RestFn.java:142)
        at clojure.core$apply.invoke(core.clj:632)
        at clojure.core$load_libs.doInvoke(core.clj:5749)
        at clojure.lang.RestFn.applyTo(RestFn.java:137)
        at clojure.core$apply.invoke(core.clj:632)
        at clojure.core$require.doInvoke(core.clj:5832)
        at clojure.lang.RestFn.invoke(RestFn.java:703)
        at clojure.tools.analyzer.jvm$loading__5340__auto____1677.invoke(jvm.clj:9)
        at clojure.tools.analyzer.jvm__init.load(Unknown Source)
        at clojure.tools.analyzer.jvm__init.<clinit>(Unknown Source)
        at java.lang.Class.forName0(Native Method)
        at java.lang.Class.forName(Class.java:340)
        at clojure.lang.RT.classForName(RT.java:2154)
        at clojure.lang.RT.classForName(RT.java:2163)
        at clojure.lang.RT.loadClassForName(RT.java:2182)
        at clojure.lang.RT.load(RT.java:436)
        at clojure.lang.RT.load(RT.java:412)
        at clojure.core$load$fn__5448.invoke(core.clj:5866)
        at clojure.core$load.doInvoke(core.clj:5865)
        at clojure.lang.RestFn.invoke(RestFn.java:408)
        at clojure.core$load_one.invoke(core.clj:5671)
        at clojure.core$load_lib$fn__5397.invoke(core.clj:5711)
        at clojure.core$load_lib.doInvoke(core.clj:5710)
        at clojure.lang.RestFn.applyTo(RestFn.java:142)
        at clojure.core$apply.invoke(core.clj:632)
        at clojure.core$load_libs.doInvoke(core.clj:5749)
        at clojure.lang.RestFn.applyTo(RestFn.java:137)
        at clojure.core$apply.invoke(core.clj:632)
        at clojure.core$require.doInvoke(core.clj:5832)
        at clojure.lang.RestFn.invoke(RestFn.java:421)
        at weird_aot.core$loading__5340__auto____81.invoke(core.clj:1)
        at weird_aot.core__init.load(Unknown Source)
        at weird_aot.core__init.<clinit>(Unknown Source)
        at java.lang.Class.forName0(Native Method)
        at java.lang.Class.forName(Class.java:340)
        at clojure.lang.RT.classForName(RT.java:2154)
        at clojure.lang.RT.classForName(RT.java:2163)
        at clojure.lang.RT.loadClassForName(RT.java:2182)
        at clojure.lang.RT.load(RT.java:436)
        at clojure.lang.RT.load(RT.java:412)
        at clojure.core$load$fn__5448.invoke(core.clj:5866)
        at clojure.core$load.doInvoke(core.clj:5865)
        at clojure.lang.RestFn.invoke(RestFn.java:408)
        at clojure.core$load_one.invoke(core.clj:5671)
        at clojure.core$load_lib$fn__5397.invoke(core.clj:5711)
        at clojure.core$load_lib.doInvoke(core.clj:5710)
        at clojure.lang.RestFn.applyTo(RestFn.java:142)
        at clojure.core$apply.invoke(core.clj:632)
        at clojure.core$load_libs.doInvoke(core.clj:5749)
        at clojure.lang.RestFn.applyTo(RestFn.java:137)
        at clojure.core$apply.invoke(core.clj:632)
        at clojure.core$require.doInvoke(core.clj:5832)
        at clojure.lang.RestFn.invoke(RestFn.java:408)
        at user$eval65$fn__67.invoke(form-init7490372454812250103.clj:1)
        at user$eval65.invoke(form-init7490372454812250103.clj:1)
        at clojure.lang.Compiler.eval(Compiler.java:6782)
        at clojure.lang.Compiler.eval(Compiler.java:6772)
        at clojure.lang.Compiler.load(Compiler.java:7227)
        ... 11 more


 Comments   
Comment by Kevin Downey [ 26/Jan/16 1:58 AM ]

run.sh in the linked github repo throws:

Exception in thread "main" java.lang.RuntimeException: Method code too large!, compiling:(weird_aot/jetty.clj:4:1)

and fails to compile the required java source

EDIT it does compile the java source, but doesn't create the default compiler output directory for clojure or create it

Comment by Kevin Downey [ 26/Jan/16 2:03 AM ]

`lein compile` with a checkout of the linked github project completes without error for me

Comment by Kevin Downey [ 26/Jan/16 2:20 AM ]

fiddling a little, a number of deps, and their transient dependencies seem to be AOT compiled, likely with different versions of Clojure, which is not intended to work as far as I am aware. Code aot compiled with Clojure version A will fail to link with code being compiled with Clojure version B

Comment by Nicola Mometto [ 26/Jan/16 2:55 AM ]

I agree with Kevin here. The issue is highly likely caused by dependencies being distributed AOT and a dependency clash.

Comment by Kevin Downey [ 26/Jan/16 3:01 AM ]

com.fzakaria/slf4j-timbre "0.2.2" is the issue. the library is aot compiled, which transitively aot compiles its dependencies, which are older versions of a bunch of timbre libraries, which in turn depend on an old version of tools.reader, so the jar for com.fzakaria/slf4j-timbre "0.2.2" contains an old compiled version of `tools.reader`. org.clojure/tools.analyzer.jvm "0.6.9" was aot compiled against a newer version of `tools.reader` so everything explodes

Comment by Alex Miller [ 26/Jan/16 8:54 AM ]

Publishing a jar with AOT'ed dependencies is for sure a problem. I realize this is a bit painful due to CLJ-322 (which I'm hoping to actually make some headway on this year).

Is there something else that should be done on this ticket?

Comment by Nicola Mometto [ 26/Jan/16 8:56 AM ]

I don't think there's anything that we can do other than pushing CLJ-322 and discouraging users to publish AOT compiled libs

Comment by Ryan Fowler [ 26/Jan/16 9:11 AM ]

The problem for me is the error message. It's fine that I can't depend on AOT compiled libraries. It doesn't seem ok that the error message when I do this is "java.lang.NoSuchFieldError: _thunk0_" or "java.lang.RuntimeException: Method code too large!"

Comment by Alex Miller [ 26/Jan/16 9:28 AM ]

I hear you. Unfortuantely, I'm not sure there's any way to detect this is what is happening in a generic way and produce a better error. The same kinds of weirdness can happen in Java as well when using a mixture of library versions.





[CLJ-1887] clojure.core.Vec does not fully implement clojure.lang.IPersistentVector Created: 26/Jan/16  Updated: 26/Jan/16

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

Type: Defect Priority: Major
Reporter: Steffen Dienst Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections
Environment:

Windows 7, Ubuntu Linux 14.04


Attachments: Text File CLJ-1887.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

The implementation of `vector-of` in gvec.clj implements the interface clojure.lang.IPersistentVector, but skips the method `int length()`(see https://github.com/clojure/clojure/blob/bc186508ab98514780efbbddb002bf6fd2938aee/src/clj/clojure/gvec.clj#L240).

user=> (.length [1 2 3])
3
user=> (.length (vector-of :long 1 2 3))
AbstractMethodError Method clojure/core/Vec.length()I is abstract  clojure.core.Vec (gvec.clj:-1)

This was encountered while trying to use core.matrix -https://github.com/mikera/core.matrix/issues/266

Approach: Implement length in gvec

Patch: CLJ-1887.patch

Screened by: Alex Miller



 Comments   
Comment by Steffen Dienst [ 26/Jan/16 3:47 AM ]

The attached patch adds a .length method for primitive type vectors. Now it fully satisfies the interface clojure.lang.IPersistentVector

Comment by Alex Miller [ 26/Jan/16 8:50 AM ]

Good find and good fix.





[CLJ-1403] ns-resolve might throw ClassNotFoundException but should return nil Created: 14/Apr/14  Updated: 22/Jan/16

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

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: compiler

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

 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)
nil  ;; expected
user=> (ns-resolve *ns* 'foo.bar)   ;; expect: nil
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

Approach: Catch CNFE when attempting to resolve a dotted symbol as a class and return nil instead.

Patch: clj-1403-2.patch

Screened by: Alex Miller
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

Comment by Alex Miller [ 22/Jan/16 3:03 PM ]

Updated patch to apply to master, no other changes, attribution retained.





[CLJ-1841] core/bean iterator is broken Created: 02/Nov/15  Updated: 22/Jan/16

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

Type: Defect Priority: Minor
Reporter: Timur Sung Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop
Environment:

java version "1.8.0_66"
Java(TM) SE Runtime Environment (build 1.8.0_66-b17)
Java HotSpot(TM) 64-Bit Server VM (build 25.66-b17, mixed mode)


Attachments: Text File clj-1841.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

The bean iterator implementation is returning the wrong data. One place the iterator is used is with transduce, which is under into:

user> (bean "test")  ;; as expected
{:bytes #object["[B" 0x546fe214 "[B@546fe214"], :class java.lang.String, :empty false}
user> (into [] (seq (bean "test")))  ;; seq works as expected
[[:bytes #object["[B" 0x6576fe71 "[B@6576fe71"]] [:class java.lang.String] [:empty false]]
user> (into [] (bean "test"))  ;; BROKEN - should match prior
[[:bytes #object[clojure.core$bean$fn__5742$fn__5743 0x4cdc53ad "clojure.core$bean$fn__5742$fn__5743@4cdc53ad"]] [:class #object[clojure.core$bean$fn__5742$fn__5743 0x55008929 "clojure.core$bean$fn__5742$fn__5743@55008929"]] [:empty #object[clojure.core$bean$fn__5742$fn__5743 0x118e7d04 "clojure.core$bean$fn__5742$fn__5743@118e7d04"]]]

Cause: The iterator impl in bean is incorrectly implemented and exposing some of the inner details instead.

Approach: Pull an iterator from the seq impl. The seq impl uses an embedded fn - the patch pulls that up and invokes from both seq and iterator.

Patch: clj-1841.patch






[CLJ-1553] Parallel transduce Created: 07/Oct/14  Updated: 22/Jan/16

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: 7
Labels: transducers


 Description   

Consider how to create a parallel path for transducers, similar to reducers fold.






[CLJ-1610] Unrolled small maps Created: 08/Dec/14  Updated: 22/Jan/16

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Zach Tellman
Resolution: Unresolved Votes: 9
Labels: collections


 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-1552] Consider kv support for transducers (similar to reducers fold) Created: 07/Oct/14  Updated: 22/Jan/16

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


 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.

Comment by Ghadi Shayban [ 21/Jan/16 9:17 PM ]

Marshall, there really isn't a proposed enhancement, yet. So there's nothing to be against! Your input is valuable. (Regarding c.c.reducers, that is a separate problem – yes that behavior is surprising)

Considering kv-support for transducers:
Is it useful to have some functions that transform reduce-kv style reducing functions (fn [result k v])?

Ignore naming:
map-key
map-val
map-keyval
filter-*

These could be mechanically generated. You wouldn't have to have a kv-version for every transducer currently in core. Some like map or filter could specifically apply to the key and ignore the val, or v.v.

Some things like map's transducer would be arity-incompatible (map's transducer has a varargs arity).





[CLJ-1760] Add `partial` reader macro Created: 17/Jun/15  Updated: 22/Jan/16  Resolved: 22/Jan/16

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

Type: Enhancement Priority: Major
Reporter: Mario T. Lanza Assignee: Unassigned
Resolution: Declined Votes: 1
Labels: reader, tacit-programming


 Description   

One of the most common things one does in functional programming is partial application. Clojure doesn't curry its functions as Haskell does. Instead it offers `partial` and the function macro:

(def hundred-times (partial * 100))
(def hundred-times #(* 100 %))

While the function macro is both terse and flexible it doesn't offer the same feel that partial does when it comes to [tacit style](https://en.wikipedia.org/wiki/Tacit_programming). Using `partial` regularly, however, defeats the brevity one would otherwise enjoy in point-free style. Having a `partial` reader macro, while seemingly a small thing, would better lend itself to the tacit style.

(def hundred-times #%(* 100))

Because most functions list arguments from general to specific, I rarely need to use the function macro to place the optional argument in some position other than last – e.g. normal partial application.



 Comments   
Comment by Mario T. Lanza [ 27/Jun/15 2:08 PM ]

Just wanted to note that others had suggested the same idea albeit using another implementation.

http://stackoverflow.com/questions/18648301/concise-syntax-for-partial-in-clojure

Comment by Alex Miller [ 22/Jan/16 9:58 AM ]

I talked to Rich and he's not interested in this change, so declining.





[CLJ-1366] The empty map literal is read as a different map each time Created: 01/Mar/14  Updated: 21/Jan/16

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: OHTA Shogo Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: memory, reader

Attachments: Text File 0001-make-the-reader-return-the-same-empty-map-when-it-re.patch     Text File 0002-make-the-reader-return-the-same-empty-map-when-it-re.patch    
Patch: Code

 Description   

As reported here (https://groups.google.com/forum/?hl=en#!topic/clojure-dev/n83hlRFsfHg), the empty map literal is read as a different map each time.

user=> (identical? (read-string "{}") (read-string "{}"))
false

Making the reader return the same empty map when it reads an empty map is expected to improve some memory efficiency, and also lead to consistency with the way other collection literals are read in.

user=> (identical? (read-string "()") (read-string "()"))
true
user=> (identical? (read-string "[]") (read-string "[]"))
true
user=> (identical? (read-string "#{}") (read-string "#{}"))
true

Cause: LispReader calls RT.map() with an empty array when it reads an empty map, and RT.map() in turn makes a new map unless its argument given is null.

Approach: make RT.map() return the same empty map when the argument is an empty array as well, not only when null



 Comments   
Comment by OHTA Shogo [ 01/Mar/14 2:59 AM ]

Sorry, the patch 0001-make-the-reader-return-the-same-empty-map-when-it-re.patch didn't work.

The updated patch 0002-make-the-reader-return-the-same-empty-map-when-it-re.patch works, but I'm afraid it'd be beyond the scope of this ticket since it modifies RT.map() behavior a bit.





[CLJ-1242] = on sorted collections with different key types incorrectly throws Created: 31/Jul/13  Updated: 21/Jan/16

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: collections

Attachments: Text File 0001-fix-for-CLJ-1242-tests.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Comparing a sorted-set with numbers to a set with keywords is not symmetric:

user=> (= #{:a} (sorted-set 1))
false
user=> (= (sorted-set 1) #{:a})
ClassCastException java.lang.Long cannot be cast to clojure.lang.Keyword  clojure.lang.Keyword.compareTo (Keyword.java:109)

The latter case should return false instead of throwing.

Cause: APersistentMap.equiv() and APersistentSet.equiv() do not expect this exception be thrown from the containsKey()/contains() check. It would probably be best for PersistentTreeMap and PersistentTreeMap to implement equiv() and handle that possibility appropriately. Should also consider similar changes in equals() if necessary.



 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.

Comment by Alex Miller [ 21/Jan/16 10:33 AM ]

I re-focused this ticket on just the equality aspect. The other request regarding `get` with a value of a different type is consistent with Java behavior and should be considered "as designed" - a separate enhancement ticket could be considered for that one.

user=> (def s (java.util.TreeSet.))
#'user/s
user=> (.add s 1)
true
user=> (.contains s "a")
ClassCastException java.lang.Long cannot be cast to java.lang.String  java.lang.String.compareTo (String.java:108)




[CLJ-1744] Unused destructured local not cleared, causes memory leak Created: 03/Jun/15  Updated: 21/Jan/16

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

Type: Defect Priority: Critical
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 12
Labels: compiler, destructuring, locals-clearing, memory

Attachments: Text File 0001-CLJ-1744-clear-unused-locals.patch     Text File 0001-CLJ-1744-clear-unused-locals-v2.patch    
Patch: Code
Approval: Prescreened

 Description   

Clojure currently doesn't clear unused locals. This is problematic as some form of destructuring can generate unused/unusable locals that the compiler cannot clear and thus can cause head retention:

;; this works
user=> (loop [xs (repeatedly 2 #(byte-array (quot (.maxMemory (Runtime/getRuntime)) 10)))] (when (seq xs) (recur (rest xs))))
nil
;; this doesn't
user=>  (loop [[x & xs] (repeatedly 200 #(byte-array (quot (.maxMemory (Runtime/getRuntime)) 10)))] (when (seq xs) (recur xs)))
OutOfMemoryError Java heap space  clojure.lang.Numbers.byte_array (Numbers.java:1252)

Here's a macroexpansion that exposes this issue:

user=> (macroexpand-all '(loop [[a & b] c] [a b]))
(let* [G__21 c 
       vec__22 G__21
       a (clojure.core/nth vec__22 0 nil)
       b (clojure.core/nthnext vec__22 1)]
 (loop* [G__21 G__21]
   (let* [vec__23 G__21
          a (clojure.core/nth vec__23 0 nil)
          b (clojure.core/nthnext vec__23 1)]
     [a b])

Cause: The first two bindings of a and b will hold onto the head of c since they are never used and not accessible from the loop body they cannot be cleared.

Approach: Track whether local bindings are used. After evaluating the binding expression, if the local binding is not used and can be cleared, then pop the result rather than storing it.

Patch: 0001-CLJ-1744-clear-unused-locals-v2.patch

Screened by: Alex Miller



 Comments   
Comment by Michael Blume [ 03/Jun/15 12:57 PM ]

Nice =)

Comment by James Henderson [ 18/Nov/15 6:12 AM ]

FYI - we've hit this as a memory leak in our production system:

(defn write-response! [{:keys [products merchant-id] :as search-result} writer response-type]
  ;; not using `search-result` throughout this fn - kept in to document intent
  ;; hangs on to `products`, a large lazy-seq, until it's completely consumed, causes memory leak

  ...
  (case response-type
    "application/edn" (print-method products writer)
    ...))
(defn write-response! [{:keys [products merchant-id]} writer response-type]
  ;; fine, releases earlier elements in products as it flies through the response

  ...
  (case response-type
    "application/edn" (print-method products writer)
    ...))

The work-around in our case is easy enough - removing the unused symbol - but, given we assumed including an unused symbol would be a no-op, it did take us a while to find!

Cheers,

James

Comment by Nicola Mometto [ 15/Dec/15 11:47 AM ]

While investigating an unrelated memory leak on core.async (ASYNC-138) I discovered that this bug also affects code inside a `go` macro, since it often emits unreachable bindings

clojure.core.async> (macroexpand '(go (let [test (range 1e10)]
                                        (str test (<! (chan))))))
(let*
 [c__15123__auto__ (clojure.core.async/chan 1) captured-bindings__15124__auto__ (clojure.lang.Var/getThreadBindingFrame)]
 (clojure.core.async.impl.dispatch/run
  (fn*
   []
   (clojure.core/let
    [f__15125__auto__
     (clojure.core/fn
      state-machine__14945__auto__
      ([] (clojure.core.async.impl.ioc-macros/aset-all! (java.util.concurrent.atomic.AtomicReferenceArray. 9) 0 state-machine__14945__auto__ 1 1))
      ([state_17532]
       (clojure.core/let
        [old-frame__14946__auto__
         (clojure.lang.Var/getThreadBindingFrame)
         ret-value__14947__auto__
         (try
          (clojure.lang.Var/resetThreadBindingFrame (clojure.core.async.impl.ioc-macros/aget-object state_17532 3))
          (clojure.core/loop
           []
           (clojure.core/let
            [result__14948__auto__
             (clojure.core/case
              (clojure.core/int (clojure.core.async.impl.ioc-macros/aget-object state_17532 1))
              1
              (clojure.core/let
               [inst_17525 (clojure.core.async.impl.ioc-macros/aget-object state_17532 7) inst_17525 (range 1.0E10) test inst_17525 inst_17526 str test inst_17525 inst_17527 (chan) state_17532 (clojure.core.async.impl.ioc-macros/aset-all! state_17532 7 inst_17525 8 inst_17526)]
               (clojure.core.async.impl.ioc-macros/take! state_17532 2 inst_17527))
              2
              (clojure.core/let
               [inst_17525 (clojure.core.async.impl.ioc-macros/aget-object state_17532 7) inst_17526 (clojure.core.async.impl.ioc-macros/aget-object state_17532 8) inst_17529 (clojure.core.async.impl.ioc-macros/aget-object state_17532 2) inst_17530 (inst_17526 inst_17525 inst_17529)]
               (clojure.core.async.impl.ioc-macros/return-chan state_17532 inst_17530)))]
            (if (clojure.core/identical? result__14948__auto__ :recur) (recur) result__14948__auto__)))
          (catch java.lang.Throwable ex__14949__auto__ (clojure.core.async.impl.ioc-macros/aset-all! state_17532 clojure.core.async.impl.ioc-macros/CURRENT-EXCEPTION ex__14949__auto__) (clojure.core.async.impl.ioc-macros/process-exception state_17532) :recur)
          (finally (clojure.lang.Var/resetThreadBindingFrame old-frame__14946__auto__)))]
        (if (clojure.core/identical? ret-value__14947__auto__ :recur) (recur state_17532) ret-value__14947__auto__))))
     state__15126__auto__
     (clojure.core/-> (f__15125__auto__) (clojure.core.async.impl.ioc-macros/aset-all! clojure.core.async.impl.ioc-macros/USER-START-IDX c__15123__auto__ clojure.core.async.impl.ioc-macros/BINDINGS-IDX captured-bindings__15124__auto__))]
    (clojure.core.async.impl.ioc-macros/run-state-machine-wrapped state__15126__auto__))))
 c__15123__auto__)




[CLJ-1797] Mention cljc when require fails Created: 10/Aug/15  Updated: 20/Jan/16

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

Type: Enhancement Priority: Minor
Reporter: Mike Fikes Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs

Attachments: Text File clj-1797.patch    
Patch: Code
Approval: Prescreened

 Description   

If you attempt to require a namespace for which the code cannot be located, now that cljc files are considered when locating code, it could be mentioned in the list of files considered on the classpath.

$ java -jar clojure-1.7.0.jar 
Clojure 1.7.0
user=> (require 'foo.bar)
FileNotFoundException Could not locate foo/bar__init.class or foo/bar.clj on classpath.  clojure.lang.RT.load (RT.java:449)

Note: FWIW, ClojureScript has similar error messages, and the order listed in the error message is the order tried when loading. If this were followed, I suspect the text of the error message above would end up looking like:

Could not locate foo/bar__init.class, foo/bar.clj, or foo/bar.cljc on classpath.

Patch: clj-1797.patch
Approach: add info about cljcfile not found in exception thrown from load
Screened by: Alex Miller



 Comments   
Comment by Cezary Kosko [ 18/Jan/16 6:21 AM ]

Hey,

I'm a newbie, so I thought I'd do that one (even though it's not vetted yet, but seems it 's bound to be), to get a tad more familiar w/ the sources.

Kind regards,
Cezary

Comment by Alex Miller [ 18/Jan/16 6:53 AM ]

Go for it!





[CLJ-1875] Parameter names in docstring for `into` Created: 06/Jan/16  Updated: 20/Jan/16

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

Type: Enhancement Priority: Minor
Reporter: Marc O'Morain Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

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

 Description   

The docstring for into does not have the correct names for the parameters. The parameter names in the arglist are to, from and xform, but the doctring refers to them as to-coll from-coll, and the docstring does not refer to the optional transducer, xform by name.

Approach: update docstring to reflect param names
Patch: CLJ-1875.patch
Screened by:



 Comments   
Comment by Alex Miller [ 20/Jan/16 3:58 PM ]

The additional phrase at the end doesn't make sense to me. How about instead:

"A transducer, xform, may be supplied as an optional second argument."





[CLJ-701] Primitive return type of loop and try is lost Created: 03/Jan/11  Updated: 20/Jan/16

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

Type: Defect Priority: Major
Reporter: Chouser Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None
Environment:

Clojure commit 9052ca1854b7b6202dba21fe2a45183a4534c501, version 1.3.0-master-SNAPSHOT


Attachments: Text File 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch     Text File 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti-v2.patch     Text File 0002-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch     File hoistedmethod-pass-1.diff     File hoistedmethod-pass-2.diff     File hoistedmethod-pass-3.diff     File hoistedmethod-pass-4.diff     File hoistedmethod-pass-5.diff     File hoistedmethod-pass-6.diff     File hoistedmethod-pass-7.diff    
Patch: Code and Test
Approval: Incomplete

 Description   
(set! *warn-on-reflection* true)
(fn [] (loop [b 0] (recur (loop [a 1] a))))

Generates the following warnings:

recur arg for primitive local: b is not matching primitive, had: Object, needed: long
Auto-boxing loop arg: b

This is interesting for several reasons. For one, if the arg to recur is a let form, there is no warning:

(fn [] (loop [b 0] (recur (let [a 1] a))))

Also, the compiler appears to understand the return type of loop forms just fine:

(use '[clojure.contrib.repl-utils :only [expression-info]])
(expression-info '(loop [a 1] a))
;=> {:class long, :primitive? true}

The problem can of course be worked around using an explicit cast on the loop form:

(fn [] (loop [b 0] (recur (long (loop [a 1] a)))))

Reported by leafw in IRC: http://clojure-log.n01se.net/date/2011-01-03.html#10:31

See Also: CLJ-1422

Patch: 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti-v2.patch



 Comments   
Comment by a_strange_guy [ 03/Jan/11 4:36 PM ]

The problem is that a 'loop form gets converted into an anonymous fn that gets called immediately, when the loop is in a expression context (eg. its return value is needed, but not as the return value of a method/fn).

so

(fn [] (loop [b 0] (recur (loop [a 1] a))))

gets converted into

(fn [] (loop [b 0] (recur ((fn [] (loop [a 1] a))))))

see the code in the compiler:
http://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L5572

this conversion already bites you if you have mutable fields in a deftype and want to 'set! them in a loop

http://dev.clojure.org/jira/browse/CLJ-274

Comment by Christophe Grand [ 23/Nov/12 2:28 AM ]

loops in expression context are lifted into fns because else Hotspot doesn't optimize them.
This causes several problems:

  • type inference doesn't propagate outside of the loop[1]
  • the return value is never a primitive
  • mutable fields are inaccessible
  • surprise allocation of one closure objects each time the loop is entered.

Adressing all those problems isn't easy.
One can compute the type of the loop and emit a type hint but it works only with reference types. To make it works with primitive, primitie fns aren't enough since they return only long/double: you have to add explicit casts.
So solving the first two points can be done in a rather lccal way.
The two other points require more impacting changes, the goal would be to emit a method rather than a fn. So it means at the very least changing ObjExpr and adding a new subclassof ObjMethod.

[1] beware of CLJ-1111 when testing.

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

I don't think this is going to make it into 1.6, so removing the 1.6 tag.

Comment by Kevin Downey [ 21/Jul/14 7:14 PM ]

an immediate solution to this might be to hoist loops out in to distinct non-ifn types generated by the compiler with an invoke method that is typed to return the getJavaClass() type of the expression, that would give us the simplifying benefits of hoisting the code out and free use from the Object semantics of ifn

Comment by Kevin Downey [ 22/Jul/14 8:39 PM ]

I have attached a 3 part patch as hoistedmethod-pass-1.diff

3ed6fed8 adds a new ObjMethod type to represent expressions hoisted out in to their own methods on the enclosing class

9c39cac1 uses HoistedMethod to compile loops not in the return context

901e4505 hoists out try expressions and makes it possible for try to return a primitive expression (this might belong on http://dev.clojure.org/jira/browse/CLJ-1422)

Comment by Kevin Downey [ 22/Jul/14 8:54 PM ]

with hoistedmethod-pass-1.diff the example code generates bytecode like this

user=> (println (no.disassemble/disassemble (fn [] (loop [b 0] (recur (loop [a 1] a))))))
// Compiled from form-init1272682692522767658.clj (version 1.5 : 49.0, super bit)
public final class user$eval1675$fn__1676 extends clojure.lang.AFunction {
  
  // Field descriptor #7 Ljava/lang/Object;
  public static final java.lang.Object const__0;
  
  // Field descriptor #7 Ljava/lang/Object;
  public static final java.lang.Object const__1;
  
  // Method descriptor #10 ()V
  // Stack: 2, Locals: 0
  public static {};
     0  lconst_0
     1  invokestatic java.lang.Long.valueOf(long) : java.lang.Long [16]
     4  putstatic user$eval1675$fn__1676.const__0 : java.lang.Object [18]
     7  lconst_1
     8  invokestatic java.lang.Long.valueOf(long) : java.lang.Long [16]
    11  putstatic user$eval1675$fn__1676.const__1 : java.lang.Object [20]
    14  return
      Line numbers:
        [pc: 0, line: 1]

 // Method descriptor #10 ()V
  // Stack: 1, Locals: 1
  public user$eval1675$fn__1676();
    0  aload_0 [this]
    1  invokespecial clojure.lang.AFunction() [23]
    4  return
      Line numbers:
        [pc: 0, line: 1]
  
  // Method descriptor #25 ()Ljava/lang/Object;
  // Stack: 3, Locals: 3
  public java.lang.Object invoke();
     0  lconst_0
     1  lstore_1 [b]
     2  aload_0 [this]
     3  lload_1 [b]
     4  invokevirtual user$eval1675$fn__1676.__hoisted1677(long) : long [29]
     7  lstore_1 [b]
     8  goto 2
    11  areturn
      Line numbers:
        [pc: 0, line: 1]
      Local variable table:
        [pc: 2, pc: 11] local: b index: 1 type: long
        [pc: 0, pc: 11] local: this index: 0 type: java.lang.Object

 // Method descriptor #27 (J)J
  // Stack: 2, Locals: 5
  public long __hoisted1677(long b);
    0  lconst_1
    1  lstore_3 [a]
    2  lload_3
    3  lreturn
      Line numbers:
        [pc: 0, line: 1]
      Local variable table:
        [pc: 2, pc: 3] local: a index: 3 type: long
        [pc: 0, pc: 3] local: this index: 0 type: java.lang.Object
        [pc: 0, pc: 3] local: b index: 1 type: java.lang.Object

}
nil
user=> 
  

the body of the method __hoisted1677 is the inner loop

for reference the part of the bytecode from the same function compiled with 1.6.0 is pasted here https://gist.github.com/hiredman/f178a690718bde773ba0 the inner loop body is missing because it is implemented as its own IFn class that is instantiated and immediately executed. it closes over a boxed version of the numbers and returns an boxed version

Comment by Kevin Downey [ 23/Jul/14 12:43 AM ]

hoistedmethod-pass-2.diff replaces 901e4505 with f0a405e3 which fixes the implementation of MaybePrimitiveExpr for TryExpr

with hoistedmethod-pass-2.diff the largest clojure project I have quick access to (53kloc) compiles and all the tests pass

Comment by Alex Miller [ 23/Jul/14 12:03 PM ]

Thanks for the work on this!

Comment by Kevin Downey [ 23/Jul/14 2:05 PM ]

I have been working through running the tests for all the contribs projects with hoistedmethod-pass-2.diff, there are some bytecode verification errors compiling data.json and other errors elsewhere, so there is still work to do

Comment by Kevin Downey [ 25/Jul/14 7:08 PM ]

hoistedmethod-pass-3.diff

49782161 * add HoistedMethod to the compiler for hoisting expresssions out well typed methods
e60e6907 * hoist out loops if required
547ba069 * make TryExpr MaybePrimitive and hoist tries out as required

all contribs whose tests pass with master pass with this patch.

the change from hoistedmethod-pass-2.diff in this patch is the addition of some bookkeeping for arguments that take up more than one slot

Comment by Nicola Mometto [ 26/Jul/14 1:37 AM ]

Kevin there's still a bug regarding long/doubles handling:
On commit 49782161, line 101 of the diff, you're emitting gen.pop() if the expression is in STATEMENT position, you need to emit gen.pop2() instead if e.getReturnType is long.class or double.class

Test case:

user=> (fn [] (try 1 (finally)) 2)
VerifyError (class: user$eval1$fn__2, method: invoke signature: ()Ljava/lang/Object;) Attempt to split long or double on the stack  user/eval1 (NO_SOURCE_FILE:1)
Comment by Kevin Downey [ 26/Jul/14 1:46 AM ]

bah, all that work to figure out the thing I couldn't get right and of course I overlooked the thing I knew at the beginning. I want to get rid of some of the code duplication between emit and emitUnboxed for TryExpr, so when I get that done I'll fix the pop too

Comment by Kevin Downey [ 26/Jul/14 12:52 PM ]

hoistedmethod-pass-4.diff logically has the same three commits, but fixes the pop vs pop2 issue and rewrites emit and emitUnboxed for TryExpr to share most of their code

Comment by Kevin Downey [ 26/Jul/14 12:58 PM ]

hoistedmethod-pass-5.diff fixes a stupid mistake in the tests in hoistedmethod-pass-4.diff

Comment by Nicola Mometto [ 21/Aug/15 1:03 PM ]

Kevin Downey FWIW the patch no longer applies

Comment by Michael Blume [ 21/Aug/15 3:06 PM ]

A naive attempt to bring the patch up to date results in

compile-clojure:
     [java] Exception in thread "main" java.lang.ExceptionInInitializerError
     [java] 	at clojure.lang.Compile.<clinit>(Compile.java:29)
     [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod, compiling:(clojure/core.clj:439:11)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7360)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7154)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7341)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7154)
     [java] 	at clojure.lang.Compiler.access$300(Compiler.java:38)
     [java] 	at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:589)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7353)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7154)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7112)
     [java] 	at clojure.lang.Compiler.eval(Compiler.java:7416)
     [java] 	at clojure.lang.Compiler.load(Compiler.java:7859)
     [java] 	at clojure.lang.RT.loadResourceScript(RT.java:372)
     [java] 	at clojure.lang.RT.loadResourceScript(RT.java:363)
     [java] 	at clojure.lang.RT.load(RT.java:453)
     [java] 	at clojure.lang.RT.load(RT.java:419)
     [java] 	at clojure.lang.RT.doInit(RT.java:461)
     [java] 	at clojure.lang.RT.<clinit>(RT.java:331)
     [java] 	... 1 more
     [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod
     [java] 	at clojure.lang.Compiler$FnExpr.parse(Compiler.java:4466)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7351)
     [java] 	... 17 more
Comment by Kevin Downey [ 25/Aug/15 1:00 PM ]

the pass 6 patch builds cleanly on 1d5237f9d7db0bc5f6e929330108d016ac7bf76c(HEAD of master as of this moment) and runs clojure's tests. I have not verified it against other projects as I did with the previous patches (I don't remember how I did that)

Comment by Michael Blume [ 25/Aug/15 1:38 PM ]

I get the same error I shared before applying the new patch to 1d5237f

Comment by Alex Miller [ 25/Aug/15 1:56 PM ]

I get some whitespace warnings with hoistedmethod-pass-6.diff but the patch applies. On a compile I get:

compile-clojure:
     [java] Exception in thread "main" java.lang.ExceptionInInitializerError
     [java] 	at clojure.lang.Compile.<clinit>(Compile.java:29)
     [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod, compiling:(clojure/core.clj:439:11)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7362)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7156)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7343)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7156)
     [java] 	at clojure.lang.Compiler.access$300(Compiler.java:38)
     [java] 	at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:588)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7355)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7156)
     [java] 	at clojure.lang.Compiler.analyze(Compiler.java:7114)
     [java] 	at clojure.lang.Compiler.eval(Compiler.java:7418)
     [java] 	at clojure.lang.Compiler.load(Compiler.java:7861)
     [java] 	at clojure.lang.RT.loadResourceScript(RT.java:372)
     [java] 	at clojure.lang.RT.loadResourceScript(RT.java:363)
     [java] 	at clojure.lang.RT.load(RT.java:453)
     [java] 	at clojure.lang.RT.load(RT.java:419)
     [java] 	at clojure.lang.RT.doInit(RT.java:461)
     [java] 	at clojure.lang.RT.<clinit>(RT.java:331)
     [java] 	... 1 more
     [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod
     [java] 	at clojure.lang.Compiler$FnExpr.parse(Compiler.java:4468)
     [java] 	at clojure.lang.Compiler.analyzeSeq(Compiler.java:7353)
     [java] 	... 17 more
Comment by Nicola Mometto [ 25/Aug/15 2:36 PM ]

Looks like direct linking interacts with the diffs in this patch in non trivial ways.

Comment by Kevin Downey [ 25/Aug/15 3:05 PM ]

I must have screwed up running the tests some how, I definitely get the same error now

Comment by Kevin Downey [ 25/Aug/15 4:00 PM ]

after you get past the cast issuing (just adding some conditional logic there)

it looks like HoistedMethodInvocationExpr needs to be made aware of if it is emitting in an instance method or a static method, and do the right thing with regard to this pointers and argument offsets. this will likely require HoistedMethod growing the ability to be a static method (and maybe preferring static methods when possible).

if you cause HoistedMethod to set usesThis to true on methods that use it, then everything appears hunky-dory (if I ran the tests correctly), but this largely negates the new direct linking stuff, which is not good.

Comment by Kevin Downey [ 27/Aug/15 9:30 PM ]

hoistedmethod-pass-7.diff adds a single commit to hoistedmethod-pass-5.diff

the single commit changes hoisted methods to always be static methods, and adjusts the arguments in the invocation of the hoisted method based on if the containing function is a static/direct function or not.

again I haven't done the extended testing with this patch.

here is an example of what the hoisted methods look like

user> (println (disassemble (fn ^long [^long y] (let [x (try (/ 1 0) (catch Throwable t 0))] x))))                                                                       

// Compiled from form-init3851661302895745152.clj (version 1.5 : 49.0, super bit)                                                                                        
public final class user$eval1872$fn__1873 extends clojure.lang.AFunction implements clojure.lang.IFn$LL {                                                                
                                                                                                                                                                         
  // Field descriptor #9 Lclojure/lang/Var;                                                                                                                              
  public static final clojure.lang.Var const__0;                                                                                                                         
                                                                                                                                                                         
  // Field descriptor #11 Ljava/lang/Object;                                                                                                                             
  public static final java.lang.Object const__1;                                                                                                                         
                                                                                                                                                                         
  // Field descriptor #11 Ljava/lang/Object;                                                                                                                             
  public static final java.lang.Object const__2;                                                                                                                         
                                                                                                                                                                         
  // Method descriptor #14 ()V                                                                                                                                           
  // Stack: 2, Locals: 0                                                                                                                                                 
  public static {};                                                                                                                                                      
     0  ldc <String "clojure.core"> [16]                                                                                                                                 
     2  ldc <String "/"> [18]                                                                                                                                            
     4  invokestatic clojure.lang.RT.var(java.lang.String, java.lang.String) : clojure.lang.Var [24]                                                                     
     7  checkcast clojure.lang.Var [26]                                                                                                                                  
    10  putstatic user$eval1872$fn__1873.const__0 : clojure.lang.Var [28]                                                                                                
    13  lconst_1    
    14  invokestatic java.lang.Long.valueOf(long) : java.lang.Long [34]                                                                                                  
    17  putstatic user$eval1872$fn__1873.const__1 : java.lang.Object [36]                                                                                                
    20  lconst_0                                                                                                                                                         
    21  invokestatic java.lang.Long.valueOf(long) : java.lang.Long [34]                                                                                                  
    24  putstatic user$eval1872$fn__1873.const__2 : java.lang.Object [38]                                                                                                
    27  return                                                                                                                                                           
      Line numbers:                                                                                                                                                      
        [pc: 0, line: 1]                                                                                                                                                 
                                                                                                                                                                         
  // Method descriptor #14 ()V                                                                                                                                           
  // Stack: 1, Locals: 1                                                                                                                                                 
  public user$eval1872$fn__1873();                                                                                                                                       
    0  aload_0 [this]                                                                                                                                                    
    1  invokespecial clojure.lang.AFunction() [41]                                                                                                                       
    4  return                                                                                                                                                            
      Line numbers:                                                                                                                                                      
        [pc: 0, line: 1]                                                                                                                                                 
                                                                                                                                                                         
  // Method descriptor #43 (J)J                                                                                                                                          
  // Stack: 2, Locals: 4  
  public final long invokePrim(long y);                                                                                                                                  
     0  lload_1 [y]                                                                                                                                                      
     1  invokestatic user$eval1872$fn__1873.__hoisted1874(long) : java.lang.Object [47]                                                                                  
     4  astore_3 [x]                                                                                                                                                     
     5  aload_3 [x]                                                                                                                                                      
     6  aconst_null                                                                                                                                                      
     7  astore_3                                                                                                                                                         
     8  checkcast java.lang.Number [50]                                                                                                                                  
    11  invokevirtual java.lang.Number.longValue() : long [54]                                                                                                           
    14  lreturn                                                                                                                                                          
      Line numbers:                                                                                                                                                      
        [pc: 0, line: 1]                                                                                                                                                 
      Local variable table:                                                                                                                                              
        [pc: 5, pc: 8] local: x index: 3 type: java.lang.Object                                                                                                          
        [pc: 0, pc: 14] local: this index: 0 type: java.lang.Object                                                                                                      
        [pc: 0, pc: 14] local: y index: 1 type: long                                                                                                                     
                                                                                                                                                                         
  // Method descriptor #59 (Ljava/lang/Object;)Ljava/lang/Object;                                                                                                        
  // Stack: 5, Locals: 2                                                                                                                                                 
  public java.lang.Object invoke(java.lang.Object arg0);                                                                                                                 
     0  aload_0 [this]                                                                                                                                                   
     1  aload_1 [arg0]                                                                                                                                                   
     2  checkcast java.lang.Number [50]                                                                                                                                  
     5  invokestatic clojure.lang.RT.longCast(java.lang.Object) : long [63]                                                                                              
     8  invokeinterface clojure.lang.IFn$LL.invokePrim(long) : long [65] [nargs: 3]                                                                                      
    13  new java.lang.Long [30]                                                                                                                                          
    16  dup_x2                                                                                                                                                           
    17  dup_x2                                                                                                                                                           
    18  pop                                                                                                                                                              
    19  invokespecial java.lang.Long(long) [68]                                                                                                                          
    22  areturn                                                                                                                                                          
                                                                                                                                                                         
                                                                                                                                                                         
  // Method descriptor #45 (J)Ljava/lang/Object;                                                                                                                         
  // Stack: 4, Locals: 4                                                                                                                                                 
  public static java.lang.Object __hoisted1874(long arg0);                                                                                                               
     0  lconst_1                                                                                                                                                         
     1  lconst_0                                      
                                                                                                                                                        
     2  invokestatic clojure.lang.Numbers.divide(long, long) : java.lang.Number [74]                                                                                     
     5  astore_2                                                                                                                                                         
     6  goto 17                                                                                                                                                          
     9  astore_3 [t]                                                                                                                                                     
    10  getstatic user$eval1872$fn__1873.const__2 : java.lang.Object [38]                                                                                                
    13  astore_2                                                                                                                                                         
    14  goto 17                                                                                                                                                          
    17  aload_2                                                                                                                                                          
    18  areturn                                                                                                                                                          
      Exception Table:                                                                                                                                                   
        [pc: 0, pc: 6] -> 9 when : java.lang.Throwable                                                                                                                   
      Line numbers:                                                                                                                                                      
        [pc: 0, line: 1]                                                                                                                                                 
        [pc: 2, line: 1]                                                                                                                                                 
      Local variable table:                                                                                                                                              
        [pc: 9, pc: 14] local: t index: 3 type: java.lang.Object                                                                                                         
        [pc: 0, pc: 18] local: y index: 1 type: java.lang.Object                                                                                                         
                                                                                                                                                                         
}                           

Comment by Kevin Downey [ 27/Aug/15 9:43 PM ]

there is still an issue with patch 7, (defn f [^long y] (let [x (try (+ 1 0) (catch Throwable t y))] x)) causes a verifier error

Comment by Nicola Mometto [ 28/Aug/15 11:41 AM ]

Patch 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch is patch hoistedmethod-pass-7 with the following changes:

  • fixes the bug mentioned in the last comment by Kevin Downey
  • removes unnecessary whitespace and indentation changes
  • conforms indentation with the surrounding lines
  • squashes the commits into one as preferred by Rich

Authorship is maintained for Kevin Downey

Comment by Kevin Downey [ 28/Aug/15 5:40 PM ]

maybe we need a "Bronsa's Guide to Submitting Patches" to supplement http://dev.clojure.org/display/community/Developing+Patches, I had no idea a single commit was preferred, but that makes sense given the format, although I just noticed the bit on deleting old patches to avoid confusion. Is there a preferred format for patch names too?
If you have recommendations beyond patch formatting into the code itself I am all ears.

Comment by Nicola Mometto [ 28/Aug/15 6:08 PM ]

Kevin, having a single commit per patch is something that I've seen Rich and Alex ask for in a bunch of tickets, as I guess it makes it easier to evaluate the overall diff (even though it sacrifices granularity of description).
No idea for a preferred patch name, I find it hard to imagine it would matter – I just use whatever git format patch outputs.

One thing I personally prefer is to add the ticket name at the beginning of the commit message, it makes it easier to understand changes when using e.g. git blame

Comment by Andy Fingerhut [ 28/Aug/15 6:33 PM ]

Just now I added a suggestion to http://dev.clojure.org/display/community/Developing+Patches that one read their patches before attaching them, and remove any spurious white space changes. Also to consider submitting patches with a single commit, rather than ones broken up into multiple commits, as reviewers tend to prefer those.

Alex Miller recently edited that page with the note about putting the ticket id first in the commit comment.

Only preference on patch file names is the one on that page – that they end with '.patch' or '.diff', because Rich's preferred editor for reading them recognizes those suffixes and displays the file in a patch-specific mode, I would guess.

Comment by Nicola Mometto [ 28/Aug/15 6:38 PM ]

0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti-v2.patch is the same as 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch but it changes some indentation to avoid mixing tabs and spaces

Comment by Michael Blume [ 01/Sep/15 5:27 PM ]

I have a verify error on the latest patch, will attempt to provide a small test case:

Exception in thread "main" java.lang.VerifyError: (class: com/climate/scouting/homestead_test$fn__17125, method: invokeStatic signature: (Ljava/lang/Object;)Ljava/lang/Object Expecting to find integer on stack, compiling:(homestead_test.clj:43:3)

Comment by Michael Blume [ 01/Sep/15 6:29 PM ]

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

I'd rather remove the dependency on compojure-api but I'm having trouble reproducing without it

Comment by Michael Blume [ 01/Sep/15 8:01 PM ]

Ok, less stupid test case:

(let [^boolean foo true]
  (do
    (try foo
      (catch Throwable t))
    nil))
Comment by Michael Blume [ 01/Sep/15 8:30 PM ]

...I wonder how hard it would be to write a generative test which produced small snippets of valid Clojure code, compiled them, and checked for errors. I bet we could've caught this.

Comment by Nicola Mometto [ 02/Sep/15 12:53 AM ]

Michael Blume I'm not sure this should be considered a compiler bug.
Contrary to numeric literals, boolean literals are never emitted unboxed and type hints are not casts so

(let [^boolean a true])
is lying to the compiler telling it a is an unboxed boolean when infact it is a boxed Boolean.

I'd say we wait for what Alex Miller or Rich Hickey think of this before considering Kevin Downey's current patch bugged, I personally (given the current compiler implementation) would consider this an user-code bug, currently ignored, that this patch merely exposes. IOW an instance of currently working code that is not however valid code

(I rest my case that if we had some better spec on what type-hints are supposed to be valid and some better compile-time validation on them, issues like this would not arise)

Comment by Nicola Mometto [ 02/Sep/15 1:13 AM ]

If this is considered a bug, the fix is trivial btw

@@ -5888,7 +5888,7 @@ public static class LocalBinding{
 
 	public boolean hasJavaClass() {
 		if(init != null && init.hasJavaClass()
-		   && Util.isPrimitive(init.getJavaClass())
+		   && (Util.isPrimitive(init.getJavaClass()) || Util.isPrimitive(tag))
 		   && !(init instanceof MaybePrimitiveExpr))
 			return false;
 		return tag != null
Comment by Kevin Downey [ 03/Sep/15 11:22 AM ]

I imagine ^boolean type hints (that don't do anything and are ignored) are going to become very common in clojure code, given everyone's keen interest in code sharing between clojure and clojurescript, and iirc clojurescript actually using ^boolean

Comment by Nicola Mometto [ 12/Sep/15 5:33 AM ]

Last patch doesn't compile anymore since it hits the bug reported in CLJ-1809

Comment by Alex Miller [ 18/Sep/15 8:28 AM ]

Moving to incomplete for now since it seems to be blocked on the other ticket

Comment by Kevin Downey [ 19/Jan/16 4:06 PM ]

0002-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch makes the change that Nicola suggested (with an extra null check). Michael's test case with the ^boolean type hint compiles now

Comment by Michael Blume [ 19/Jan/16 6:01 PM ]

Maybe this is out of scope for this ticket since it's just existing code that you're moving around, but I've always been confused by

//exception should be on stack
gen.visitVarInsn(OBJECT_TYPE.getOpcode(Opcodes.ISTORE), finallyLocal);
finallyExpr.emit(C.STATEMENT, objx, gen);
gen.visitVarInsn(OBJECT_TYPE.getOpcode(Opcodes.ILOAD), finallyLocal);

If we emit finallyExpr as C.STATEMENT, it should have no impact on the stack, and the exception should just be there without us needing to store and load it, right?

Comment by Michael Blume [ 19/Jan/16 6:02 PM ]

Also the latest patch seems to add a patch file to the root directory

Comment by Michael Blume [ 19/Jan/16 7:01 PM ]

New failing code

(defn foo [req]
  (let [^boolean b (get req :is-baz)]
    (do
      (try
        :bar)
      :foo)))
Comment by Kevin Downey [ 20/Jan/16 12:05 AM ]

the latest patch applied to master is causing some test failures for data.xml

Testing clojure.data.xml.test-seq-tree

FAIL in (release-head-top) (test_seq_tree.clj:52)
expected: (= nil (.get input-ref))
  actual: (not (= nil (0 1 2 3 4 5 6 7 8 9)))

FAIL in (release-head-nested-late) (test_seq_tree.clj:60)
expected: (= nil (.get input-ref))
  actual: (not (= nil (1 2 :< 3 4 5 :>))
Comment by Nicola Mometto [ 20/Jan/16 5:54 AM ]

Michael Blume that's still an invalid type hint. Clojure is inconsistent all over the place with enforcing valid type hints, so even though I think we should handle the VerifyError explicitly, I don't think we need to care about making that code work.

WRT ^boolean type hints going to be common because clojurescript uses them – I don't think we should accept invalid code for the sake of interoparability. See CLJ-1883 for an instance of such a wrong type hint being used in Om, the solution was to fix Om, not to make the clojure compiler ignore yet another wrong type hint.





[CLJ-1423] Applying a var to an infinite arglist consumes all available memory Created: 15/May/14  Updated: 18/Jan/16

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

Type: Defect Priority: Major
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: performance

Attachments: Text File apply-var.patch     Text File clj-1423.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

It is possible to apply a function to an infinite argument list: for example, (apply distinct? (repeat 1)) immediately returns false, after realizing just a few elements of the infinite sequence (repeat 1). However, (apply #'distinct? (repeat 1)) attempts to realize all of (repeat 1) into memory at once.

This happens because Var.applyTo delegates to AFn.applyToHelper to decide which arity of Var.invoke to dispatch to; but AFn doesn't expect infinite arglists (mostly those use RestFn). So it uses RT.seqToArray, which doesn't work well in this case.

Instead, Var.applyTo(args) can just dispatch to deref().applyTo(args), and let the function being stored figure out what to do with the arglist.

I've changed Var.applyTo to do this, and added a test (which fails before my patch is applied, and passes afterwards).

Patch: clj-1423.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 18/Jan/16 5:29 PM ]

I also did a quick perf test on this:

(dotimes [x 20] (time (dotimes [y 10000] (apply #'+ (range 100)))))

which showed times ~82 ms per rep before the patch and ~10 ms per rep after the patch (on par with applying the function directly).

I added a new patch that squashes the commits, adds the ticket number to the commit message, attribution was retained.





[CLJ-1755] Calling nth on TransientVector with a default will not check if the transient has been made persistent Created: 16/Jun/15  Updated: 18/Jan/16  Resolved: 18/Jan/16

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

Type: Defect Priority: Major
Reporter: Patrick Gombert Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: transient

Attachments: Text File transient-vector-nth.patch    
Patch: Code
Approval: Triaged

 Description   

Invoking nth with arity two on a TransientVector will ensure that the transient is editable. However, invoking with arity three will return the supplied not-found value if the index is out of range.



 Comments   
Comment by Alex Miller [ 16/Jun/15 9:42 AM ]

Can you add an example to the description and a test to the patch?

Comment by Alex Miller [ 18/Jan/16 5:15 PM ]

An example of this would be something like:

(def t (transient ["a"]))
(persistent! t) ;; no longer editable
(nth t 0 :foo)  ;; IllegalAccessError Transient used after persistent! call
(nth t -1 :foo) ;; returns :foo (EXPECT: same IllegalAccessError)
(nth t 99 :foo) ;; IllegalAccessError as the inner count() call will throw it

Thus, the exposure here is solely on calling nth with a negative index on a no-longer transient collection, which returns the same answer as a persistent collection. This doesn't seem worth pursuing. Declining...





[CLJ-1507] Throw NPE in eval reader Created: 16/Aug/14  Updated: 18/Jan/16

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

Type: Enhancement Priority: Minor
Reporter: dennis zhuang Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: eval-reader
Environment:

Mac OSX 10.9.4
java version "1.7.0_17"
Java(TM) SE Runtime Environment (build 1.7.0_17-b02)
Java HotSpot(TM) 64-Bit Server VM (build 23.7-b01, mixed mode)


Attachments: File fix_npe_eval_reader.diff    
Patch: Code

 Description   
Clojure 1.7.0-master-SNAPSHOT
user=> #=(var a)
NullPointerException   clojure.lang.Symbol.hashCode (Symbol.java:84)
user=> (.printStackTrace *e)
clojure.lang.LispReader$ReaderException: clojure.lang.LispReader$ReaderException: java.lang.NullPointerException
	at clojure.lang.LispReader.read(LispReader.java:218)
	at clojure.core$read.invoke(core.clj:3580)
	at clojure.core$read.invoke(core.clj:3578)
	at clojure.core$read.invoke(core.clj:3576)
	at clojure.core$read.invoke(core.clj:3574)
	at clojure.main$repl_read.invoke(main.clj:139)
	at clojure.main$repl$read_eval_print__6807$fn__6808.invoke(main.clj:237)
	at clojure.main$repl$read_eval_print__6807.invoke(main.clj:237)
	at clojure.main$repl$fn__6816.invoke(main.clj:257)
	at clojure.main$repl.doInvoke(main.clj:257)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at clojure.main$repl_opt.invoke(main.clj:323)
	at clojure.main$main.doInvoke(main.clj:421)
	at clojure.lang.RestFn.invoke(RestFn.java:397)
	at clojure.lang.Var.invoke(Var.java:375)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: clojure.lang.LispReader$ReaderException: java.lang.NullPointerException
	at clojure.lang.LispReader.read(LispReader.java:218)
	at clojure.lang.LispReader$CtorReader.invoke(LispReader.java:1164)
	at clojure.lang.LispReader$DispatchReader.invoke(LispReader.java:609)
	at clojure.lang.LispReader.read(LispReader.java:183)
	... 17 more
Caused by: java.lang.NullPointerException
	at clojure.lang.Symbol.hashCode(Symbol.java:84)
	at java.util.concurrent.ConcurrentHashMap.hash(ConcurrentHashMap.java:332)
	at java.util.concurrent.ConcurrentHashMap.get(ConcurrentHashMap.java:987)
	at clojure.lang.Namespace.findOrCreate(Namespace.java:173)
	at clojure.lang.RT.var(RT.java:341)
	at clojure.lang.LispReader$EvalReader.invoke(LispReader.java:1042)
	at clojure.lang.LispReader$DispatchReader.invoke(LispReader.java:616)
	at clojure.lang.LispReader.read(LispReader.java:183)
	... 20 more

If the var symbol doesn't contains namespace ,it will throw the NPE exception in above code.Instead,i think it should use Compiler.currentNS() when doesn't find the var's namespace.

The patch is attached, after patched:

Clojure 1.7.0-master-SNAPSHOT
user=> #=(var a)
#'user/a





[CLJ-1180] defprotocol doesn't resolve tag classnames Created: 10/Mar/13  Updated: 18/Jan/16

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

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

Attachments: Text File 001-CLJ-1180.patch     Text File clj-1180-2.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

defprotocol doesn't resolve tag classnames, this results in exceptions being thrown when the declared protocol uses as a tag an imported class that is not imported in the namespace that uses it.

user=> (import 'clojure.lang.ISeq)
clojure.lang.ISeq
user=> (defprotocol p (^ISeq f [_]))
p
user=> (ns x)
nil
x=> (defn x [y] (let [z (user/f y)] (inc z)))
CompilerException java.lang.IllegalArgumentException: Unable to resolve classname: ISeq, compiling:(NO_SOURCE_PATH:4:33)

Patch: clj-1180-2.patch

Screened by: Alex Miller

  • My only hesitancy with this patch is making the tagToClass public and reaching into it from core_deftype - this means that call into the Compiler needs to remain open forever as an api.


 Comments   
Comment by Alex Miller [ 03/Sep/13 9:38 AM ]

Similer to CLJ-1232.

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

Patch updates for current master, retains attribution.





[CLJ-1021] (let [i 5] (defmacro m [v] v) m) interprets m as a function, not a macro Created: 02/Jul/12  Updated: 18/Jan/16

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

Type: Defect Priority: Minor
Reporter: Zii Prime Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File 001-propagate-on-macro-meta.diff    
Patch: Code

 Description   
(let [i 5] (defmacro m [& args] args) (def x (m (inc 5) (inc 6) (inc 7))) [x m (meta #'m)])

is

[(8) #<user$eval522$m__523 user$eval522$m__523@11a74355> {:macro true, :ns #<Namespace user>, :name m, :arglists ([& args]), :line 1, :file "NO_SOURCE_PATH"}]

It appears to be interpreting m as a function despite the metadata. This behavior only shows up inside the (let ...).



 Comments   
Comment by Nicola Mometto [ 18/Aug/12 11:43 AM ]

The problem is the same as the one for http://dev.clojure.org/jira/browse/CLJ-918

The patch linked there was mine but i had no CA signed at that time, and no account on jira.

Here is the same patch in the correct format.

Bug #918 should be closed as this is a duplicate

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

I've not looked into details yet, but current patch fails to apply cleanly as of Oct 25, 2013 latest Clojure master.





[CLJ-1358] doc macro does not expand special cases properly Created: 17/Feb/14  Updated: 18/Jan/16

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

Type: Defect Priority: Trivial
Reporter: Chad Taylor Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: repl

Attachments: Text File CLJ-1358.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

The doc macro supports three special cases, mapping & to fn, catch to try, and finally to try. However, the macro does not currently expand these cases - it executes them like a function instead. This is evident if you use the following at a REPL:

user> (macroexpand '(doc try))   ;; ok
((var clojure.repl/print-doc) ((var clojure.repl/special-doc) (quote try)))

user> (macroexpand '(doc catch)) ;; broken
;; -- unexpectedly prints try doc -- ;;
nil

user> (= (with-out-str (doc catch)) (with-out-str (doc try))) ;; broken, expect true
;; -- unexpectedly prints try doc -- ;;
false

Workaround: Call doc with the symbol to which the special case is mapped, fn or try.

Cause: Incorrect quoting when handling special cases in doc macro

Solution: Update special case quoting approach to match the other cases.

Patch: CLJ-1358.patch



 Comments   
Comment by Chad Taylor [ 17/Feb/14 10:41 PM ]

Adding a patch with code and test.





[CLJ-1587] PersistentArrayMap's assoc doesn't respect HASHTABLE_THRESHOLD Created: 12/Nov/14  Updated: 18/Jan/16

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

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: collections, data-structures, maps

Attachments: Text File 0001-PersistentArrayMap-s-assoc-doesn-t-respect-HASHTABLE.patch    
Patch: Code
Approval: Prescreened

 Description   

Currently a map with more than 8 elements will be converted from a PersistentArrayMap to a PersistentHashMap, but if using assoc, it will take 9 elements before the conversion happens:

user=>  (class {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7})
clojure.lang.PersistentArrayMap
user=> (class {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7 8 8})
clojure.lang.PersistentHashMap
user=>  (class (assoc {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7} 8 8))
clojure.lang.PersistentArrayMap
user=>  (class (assoc {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7} 8 8 9 9))
clojure.lang.PersistentHashMap

After patch:

user=> (class {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7})
clojure.lang.PersistentArrayMap
user=> (class {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7 8 8})
clojure.lang.PersistentHashMap
user=> (class (assoc {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7} 8 8))
clojure.lang.PersistentHashMap
user=> (class (assoc {0 0 1 1 2 2 3 3 4 4 5 5 6 6 7 7} 8 8 9 9))
clojure.lang.PersistentHashMap

Patch: 0001-PersistentArrayMap-s-assoc-doesn-t-respect-HASHTABLE.patch

Screened by: Alex Miller






[CLJ-1389] Re-loading a namespace ignores metadata specified for the namespace Created: 20/Mar/14  Updated: 18/Jan/16  Resolved: 18/Jan/16

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

Type: Defect Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Not Reproducible Votes: 1
Labels: metadata, namespace, repl


 Description   

Using the REPL I added some metadata to a namespace and reloaded it.

(ns io.aviso.rook-test5)

to

(ns io.aviso.rook-test5
  "A testing namespace"
  {:inherted   :namespace
   :overridden :namespace})

But requesting the meta data yields nil:

(-> 'io.aviso.rook-test5 find-ns meta)
=> nil

I have tested a few variations, such as putting the metadata on the symbol instead of providing an attribute map. In all cases, the metadata from before the load persists.

Using remove-ns before re-loading the namespace does the right thing ... the metadata shows up as expected.



 Comments   
Comment by Alex Miller [ 18/Jan/16 3:51 PM ]

NR in Clojure 1.8





[CLJ-1317] clojure.zip/seq-zip returns spurious nils during traversal Created: 31/Dec/13  Updated: 18/Jan/16

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

Type: Defect Priority: Minor
Reporter: Michał Marczyk Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: zip

Attachments: Text File 0001-CLJ-1317-fix-seq-zip-to-avoid-spurious-nils.patch    
Patch: Code
Approval: Triaged

 Description   

Problem reported by Lee Spector on the mailing list:

https://groups.google.com/d/msg/clojure/8TL7IGmE7N0/u1xfgTOLDRgJ

Here's a quote from Lee's post describing the problem:

Here's an illustration, stepping through '(() 0) with next and printing the node at each step: 

(loop [z (zip/seq-zip '(() 0))] 
  (if (zip/end? z) 
    :done 
    (do (println (zip/node z)) 
      (recur (zip/next z))))) 

That produces: 

(() 0) 
() 
nil 
0 
:done 

I don't expect the nil to be there. 

The underlying cause is that seq-zip passes identity as the children argument to zipper. Applied to (), this returns (), which is truthy, leading zipper to descend into a non-existent subtree.

One natural solution would be to use seq in place of identity:

(defn seq-zip [root]
  (zipper seq?
          seq  ;; changed
          (fn [node children] (with-meta children (meta node)))
          root))

With this change, no nil is produced in the example above. Patch with this change forthcoming.



 Comments   
Comment by Michał Marczyk [ 31/Dec/13 5:52 PM ]

Note that the docstring of clojure.zip/zipper asks that the children argument return a seq of children. The rest of clojure.zip, however, expects nil to be returned when there are no children, as evidenced by this problem.

One could argue that this behaviour of the rest of clojure.zip should be fixed, but I think it makes sense and is convenient. Perhaps the docstring should be adjusted, though.





[CLJ-1231] fn and letfn don't support hinting the function's return type Created: 17/Jul/13  Updated: 18/Jan/16

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

Type: Enhancement Priority: Minor
Reporter: Tassilo Horn Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: typehints


 Description   

You can add type hints to the argument vector(s) of `defn` to declare the return type of a function like so (with warn-on-reflection being true):

user> (defn foo ^String [s] s)
#'user/foo
user> (.substring (foo "hallo") 1 2)
"a"

But sadly, the same doesn't work with `fn` and `letfn`:

user> (.substring ((fn ^String [s] s) "hallo") 1 2)
Reflection warning, NO_SOURCE_PATH:1:1 - call to substring can't be resolved.
"a"
user> (letfn [(foo ^String [s] s)]
	(.substring (foo "hallo") 1 2))
Reflection warning, NO_SOURCE_PATH:2:7 - call to substring can't be resolved.
"a"

I don't see why this feature is available to `defn` but not to `fn` and `letfn`. I even consider it a kind of defect, because anything else including :pre/:post are also supported by the latter two, so the support for hinting the return type should be there simply because of analogy.






[CLJ-1649] Hash/equality inconsistency for floats & doubles Created: 23/Jan/15  Updated: 18/Jan/16

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

Type: Defect Priority: Minor
Reporter: Michael Gardner Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: numerics

Approval: Triaged

 Description   

This is closely related to CLJ-1036, but there was a suggestion to make a new ticket.

The issue is that for a float f and a double d, we can have (= f d) but not (= (hash f) (hash d)), which breaks a fundamental assumption about hash/equality consistency and leads to weirdness like this (from Immo Heikkinen's email to the Clojure mailing list):

(= (float 0.5) (double 0.5))
=> true
(= #{(float 0.5)} #{(double 0.5)})
=> true
(= {:a (float 0.5)} {:a (double 0.5)})
=> true
(= #{{:a (float 0.5)}} #{{:a (double 0.5)}})
=> false

One way to resolve this would be to tweak the hashing of floats and/or doubles, but that suggestion has apparently been rejected.

An alternative would be to modify = so that it never returns true for float/double comparisons. One should never compare floats with doubles using = anyway, so such a change should have minimal impact beyond restoring hash/equality consistency.






[CLJ-1591] Symbol not being bound in namespace when name clashes with clojure.core Created: 14/Nov/14  Updated: 18/Jan/16

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

Type: Defect Priority: Trivial
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None


 Description   

The following code fails (both in 1.6 and latest 1.7-alpha4):

user=> (ns foo)
nil
foo=>  (def inc inc)
WARNING: inc already refers to: #'clojure.core/inc in namespace: foo, being replaced by: #'foo/inc
#'foo/inc

;; Note inc is unbound at this point, which causes the exception below
foo=> inc
#<Unbound Unbound: #'foo/inc>
foo=> (ns bar)
nil
bar=> (require ['foo :refer ['inc]])
WARNING: inc already refers to: #'clojure.core/inc in namespace: bar, being replaced by: #'foo/inc
nil
bar=> (inc 8)

IllegalStateException Attempting to call unbound fn: #'foo/inc  clojure.lang.Var$Unbound.throwArity (Var.java:43)

Further investigation shows that foo/inc is unbound:

foo/inc
=> #<Unbound Unbound: #'foo/inc>

Further investigation also shows that replacing the (def inc inc) with almost anything else, e.g. (def inc dec), (def inc clojure.core/inc), or (def inc (fn [n] (+ n 1))), causes no exception (but the warnings remain).

I would expect:
a) foo/inc should be bound and have the same value as clojure.core/inc
b) No error when requiring foo/inc
c) bar/inc should be bound to foo/inc



 Comments   
Comment by Nicola Mometto [ 14/Nov/14 10:04 PM ]

The second error should be expected, the right syntax should be (require ['foo :refer ['inc]]) (note the leading quote before inc)

Comment by Mike Anderson [ 14/Nov/14 10:20 PM ]

Thanks for the catch Nicola - I've edited the description. Still get the same error however (just with a slightly different message)

Comment by Alex Miller [ 14/Nov/14 10:22 PM ]

See comment...

Comment by Mike Anderson [ 14/Nov/14 10:24 PM ]

@Alex what comment? Note that the error still occurs even with the right syntax....

Comment by Mike Anderson [ 14/Nov/14 10:26 PM ]

Appears to have been closed prematurely

Comment by Alex Miller [ 14/Nov/14 10:39 PM ]

I can't reproduce with the correct syntax:

Clojure 1.7.0-master-SNAPSHOT
user=> (ns foo)
nil
foo=> (def inc inc)
WARNING: inc already refers to: #'clojure.core/inc in namespace: foo, being replaced by: #'foo/inc
#'foo/inc
foo=> (ns bar)
nil
bar=> (require ['foo :refer ['inc]])
WARNING: inc already refers to: #'clojure.core/inc in namespace: bar, being replaced by: #'foo/inc
nil
Comment by Mike Anderson [ 14/Nov/14 10:55 PM ]

The problem is that the var is still unbound and causes e.g. the following error:

=> (foo/inc 8)
IllegalStateException Attempting to call unbound fn: #'foo/inc clojure.lang.Var$Unbound.throwArity (Var.java:43)

I don't think that should be expected - or am I missing something?

Comment by Alex Miller [ 14/Nov/14 10:57 PM ]

Ah, will take a look. But not right now.

Comment by Andy Fingerhut [ 15/Nov/14 1:09 PM ]

Updated the description with a few more details. The exception goes away if you do (def inc (fn [n] (+ n 1))) instead of (def inc inc), for example. The warnings remain.

Comment by Tom Crayford [ 20/Nov/14 11:07 AM ]

Unsure if this is the same issue (I think it might be?), but I reproduced the exact same error message with AOT compilation involved:

reproduced in this git repository: https://github.com/yeller/compiler_update_not_referenced_bug

clone it, run `lein do clean, uberjar, test`, and that error message will show up every time for me

Comment by Andy Fingerhut [ 20/Nov/14 5:43 PM ]

Mike, I think replacing (def inc inc) in your example with (def inc clojure.core/inc) should be considered as a reasonable workaround for this issue, unless you have some use case where you need to def inc to something that is not in clojure.core (and if so, why?)

The reason (def inc inc) behaves this way is, if not absolutely necessary, at least commonly used in Clojure programs to define recursive functions, e.g. (defn fib [n] (if (<= n 1) 1 (+ (fib (dec n)) (fib (- n 2))))), so that the occurrences of fib in the body are resolved to the fib being defined.

Comment by Alex Miller [ 22/Nov/14 9:05 AM ]

Moving to 1.7 until I can look at this more deeply.

Comment by Mike Anderson [ 23/Nov/14 6:08 PM ]

Andy - yes the workaround is fine for me right now.

I don't think this is an urgent issue but it may be exposing a subtle complexity regarding assumptions about the state of the namespace at different times. Perhaps the semantics should be something like:

  • The def statement itself should be run before the var is interned. e.g. (def inc (inc 5)) should result in (def inc 6)
  • Anything complied / deferred to run after completion of the def statement should use the new var (i.e. the new var should be referenced by fns, lazy sequences etc.)
Comment by Andy Fingerhut [ 23/Nov/14 6:36 PM ]

I'm not sure what your proposal means in a case like this:

(def inc (fn [x] (inc x)))

Is the second inc to be interpreted/resolved before or after the new inc is created? Because it is (fn ...) it should be the after-behavior? What else besides fn should cause the after-behavior, rather than the before-behavior?

Even more fun (not saying that people often write code like this, but the compiler can handle it today):

(def inc (if (> (inc y) 5)
           (fn [x] (inc x))
           (fn [x] (dec x))))

I think the current compiler behavior of 'in the body of a def, the def'd symbol always refers to the new var, not any earlier def'd vars' is fairly straightforward to explain.

Comment by Tom Crayford [ 23/Nov/14 9:15 PM ]

Should I file the AOT issue reproduced in that thing as a new issue?

Comment by Andy Fingerhut [ 24/Nov/14 5:16 PM ]

Tom: Alex Miller or another screener would be best to say whether the AOT issue should be a separate ticket, but my best guess would be "go for it". I tried to look at the link you gave but it seems not to point to anything. Could you double-check that link?

Comment by Tom Crayford [ 24/Nov/14 6:48 PM ]

Andy,

Great. I'll write one up tomorrow sometime. I accidentally left that repo as private, should be visible now.

Comment by Andy Fingerhut [ 24/Nov/14 8:11 PM ]

This comment is really most relevant for ticket CLJ-1604, where it has been copied:

Tom, looked at your project. Thanks for that. It appears not to have anything like (def inc inc) in it. It throws exception during test step of 'lein do clean, uberjar, test' consistently for me, too, but compiles with only warnings and passes tests with 'lein do clean, test'. I have more test results showing in which Clojure versions these results change. To summarize, the changes to Clojure that appear to make the biggest difference in the results are below (these should be added to the new ticket you create – you are welcome to do so):

Clojure 1.6.0, 1.7.0-alpha1, and later changes up through the commit with description "CLJ-1378: Allows FnExpr to override its reported class with a type hint": No errors or warnings for either lein command above.

Next commit with description "Add clojure.core/update, like update-in but takes a single key" that adds clojure.core/update: 'lein do clean, test' is fine, but 'lein do clean, uberjar' throws exception during compilation, probably due to CLJ-1241.

Next commit with description "fix CLJ-1241": 'lein do clean, test' and 'lein do clean, uberjar' give warnings about clojure.core/update, but no errors or exceptions. 'lein do clean, uberjar, test' throws exception during test step that is same as the one I see with Clojure 1.7.0-alpha4. Debug prints of values of clojure.core/update and int-map/update (in data.int-map and in Tom's namespace compiler-update-not-referenced-bug.core) show things look fine when printed inside data.int-map, and in Tom's namespace when not doing the uberjar, but when doing the uberjar, test, int-map/update is unbound in Tom's namespace.

In case it makes a difference, my testing was done with Mac OS X 10.9.5, Leiningen 2.5.0 on Java 1.7.0_45 Java HotSpot(TM) 64-Bit Server VM

Comment by Nicola Mometto [ 25/Nov/14 3:44 PM ]

Tom, I've opened a ticket with a patch fixing the AOT issue: http://dev.clojure.org/jira/browse/CLJ-1604





[CLJ-1074] Read/print round-trip for +/-Infinity and NaN Created: 21/Sep/12  Updated: 18/Jan/16

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

Type: Defect Priority: Minor
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-1029] ns defmacro allows arbitrary execution of clojure.core fns Created: 23/Jul/12  Updated: 18/Jan/16

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

Type: Defect Priority: Minor
Reporter: Craig Brozefsky Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: error-reporting
Environment:

all


Attachments: File ns-patch.diff    
Patch: Code
Approval: Triaged

 Description   

The form:

(ns foo (:print "I AM A ROBOT"))

will print "I AM A ROBOT"

This is because the defmacro takes the name of the first element of the reference, looks it up in the clojure.core namespace and invokes it on the rest of the args.

This is minor, but it does mean that an otherwise declarative form is not executing code.



 Comments   
Comment by Alan Malloy [ 25/Jul/12 4:37 PM ]

One apparent problem with this patch is that you throw an exception for :refer. You should add that, and make sure there aren't any others missing. Also, #{x y z} is better than (set [x y z]), and you should probably use pr-str rather than str, although I can't think of a case where it matters for the objects in question.

Comment by Andy Fingerhut [ 26/Jul/12 6:31 PM ]

A more minor detail of patch formatting – please attach your patch in git format. See the instructions under the section heading "Development" on this web page: http://dev.clojure.org/display/design/JIRA+workflow

Comment by Craig Brozefsky [ 05/Aug/12 9:53 AM ]

git format-patch version of the diff, with the edits suggested by other maintainers.

Comment by Craig Brozefsky [ 05/Aug/12 10:00 AM ]

Alan: please note that :refer was not mentioned in the docstring for ns, or used in any of the unit tests for clojure.

Are you sure that it is an expected argument, or just an arrangement that happens to work under the current ns macro? The docstring for 'refer itself says to use :use in ns macros instead of calling refer.

I added "refer" to the set of accepted references all the same.

Comment by Alex Miller [ 18/Jan/16 3:33 PM ]

This is a case where better error checking would prevent this problem.





[CLJ-1852] Clojure-generated class names length exceed file-system limit Created: 20/Nov/15  Updated: 18/Jan/16

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

Type: Enhancement Priority: Minor
Reporter: Martin Raison Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: compiler
Environment:

tested on CentOS 6



 Description   

Class names generated by the Clojure compiler can be arbitrarily long, exceeding the file system's maximum allowed file name length. For example it happens when you nest functions a bit too deeply:

(defmacro nestfn [n & body]
  (if (> n 0)
    `(fn [] (nestfn ~(- n 1) ~@body))
    body))

(def myf (nestfn 100 "body"))

Compiling this produces a java.io.IOException: File name too long exception.



 Comments   
Comment by Martin Raison [ 20/Nov/15 9:32 PM ]

The Scala community found this issue a while ago, and now the compiler has a max-classfile-name parameter (defaulting to 255). Hashing is used when the limit is exceeded. Maybe we should consider something similar?





[CLJ-1668] ns macro throws NPE if empty reference is specified Created: 02/Mar/15  Updated: 18/Jan/16

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

Type: Enhancement Priority: Minor
Reporter: Philipp Meier Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: errormsgs, ft, macro, namespace

Attachments: Text File clj-1668.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

The following invocations of `ns` will all throw a NPE

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

;; throw

1. Unhandled java.lang.NullPointerException
   (No message)

                      core.clj: 1518  clojure.core/name
                      core.clj: 5330  clojure.core/ns/process-reference
                      core.clj: 2559  clojure.core/map/fn
                  LazySeq.java:   40  clojure.lang.LazySeq/sval
                  LazySeq.java:   49  clojure.lang.LazySeq/seq
                       RT.java:  484  clojure.lang.RT/seq
                      core.clj:  133  clojure.core/seq
                      core.clj:  694  clojure.core/concat/cat/fn
                  LazySeq.java:   40  clojure.lang.LazySeq/sval
                  LazySeq.java:   49  clojure.lang.LazySeq/seq
                     Cons.java:   39  clojure.lang.Cons/next
                       RT.java: 1654  clojure.lang.RT/boundedLength
                      AFn.java:  148  clojure.lang.AFn/applyToHelper
                      Var.java:  700  clojure.lang.Var/applyTo

I'd expect an exception that is describing the cause of the error, not an "symptom".

Proposed: Check for nil reference in ns and throw.

Patch: clj-1668.patch






[CLJ-1808] map-invert should use transients and reduce-kv instead of reduce Created: 30/Aug/15  Updated: 18/Jan/16

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

Type: Enhancement Priority: Minor
Reporter: Nikita Prokopov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: ft

Attachments: Text File clj-1808-map-invert-should-use-reduce-kv-and-transient.patch    
Patch: Code
Approval: Prescreened

 Description   

Two performance enhancements on clojure.set/map-invert:

1) Use reduce-kv to avoid realizing mapentry's from input map
2) Use transients to create the output map

Patch: clj-1808-map-invert-should-use-reduce-kv-and-transient.patch



 Comments   
Comment by Alex Miller [ 04/Sep/15 10:42 AM ]

Would be nice to see a quick perf test that compared before/after times.





[CLJ-1873] Docstrings for require and *data-readers* do not mention cljc files Created: 01/Jan/16  Updated: 18/Jan/16

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

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, ft

Attachments: Text File clj-1873-v1.patch    
Patch: Code
Approval: Prescreened

 Description   

The behavior of require and *data-readers* was modified in Clojure 1.7.0 to look for files ending with .cljc as well as files ending with .clj, but their doc strings do not mention this change.

Patch: clj-1873-v1.patch

Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 01/Jan/16 4:30 AM ]

Patch clj-1873-v1.patch dated Jan 1 2016 adds mentions of .cljc files to the doc strings of require and *data-readers*





[CLJ-1654] Reuse seq in some Created: 04/Feb/15  Updated: 18/Jan/16

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

Type: Enhancement Priority: Trivial
Reporter: Gijs Stuurman Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: ft, performance

Attachments: Text File 0000-reuse-seq-in-some.patch     Text File clj-1654.patch    
Patch: Code
Approval: Prescreened

 Description   

By using when-let at most two seq constructions can be avoided per invocation of some.

Patch: clj-1654.patch

Screened by: Alex Miller



 Comments   
Comment by Ghadi Shayban [ 04/Feb/15 12:11 PM ]

This is similar to the tweak to dorun/doall in CLJ-1515. It is a good benefit when a collection doesn't cache its seq

Comment by Alex Miller [ 12/Jan/16 9:40 AM ]

Updated patch to include ticket number in commit message, no other changes, attribution retained.





[CLJ-1759] macroexpand throws runtime exception on symbol bound to a class Created: 17/Jun/15  Updated: 18/Jan/16

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

Type: Defect Priority: Trivial
Reporter: W. David Jarvis Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, ft
Environment:

OSX 10.10.3, Leiningen 2.5.1, Java 1.8.0_45 64-bit.


Attachments: Text File clj-1759.patch    
Patch: Code
Approval: Prescreened

 Description   

The use of macroexpand on short class name symbols triggers a RuntimeException.

user=> (import 'java.net.URI)
java.net.URI
user=> (macroexpand '(java.net.URI "http://google.com")) ;; fine
(java.net.URI "http://google.com")
user=> (macroexpand '(URI "http://google.com")) ;; huh?
java.lang.RuntimeException: Expecting var, but URI is mapped to class java.net.URI
user=> (pst *e)
RuntimeException Expecting var, but URI is mapped to class java.net.URI
	clojure.lang.Util.runtimeException (Util.java:221)
	clojure.lang.Compiler.lookupVar (Compiler.java:7092)
	clojure.lang.Compiler.isMacro (Compiler.java:6571)
	clojure.lang.Compiler.macroexpand1 (Compiler.java:6626)
	clojure.core/macroexpand-1 (core.clj:3870)
	clojure.core/macroexpand (core.clj:3879)

Neither of these should throw an error during macroexpansion (basically should be same after expansion. Both should throw the same error when evaluated (ClassCast trying to invoke a Class as an IFn).

Approach: Throw the runtime error in lookupVar only if internNew is true. In that case we unexpectedly found something other than a var and should still report. Otherwise, just let lookupVar flow through to return a null (no var found.



 Comments   
Comment by Alex Miller [ 18/Jun/15 6:19 AM ]

The compiler is trying to determine if the thing in function position is a var that is a macro that requires expansion in Compiler.isMacro().

In the case of (java.net.URI "http://google.com"), lookupVar determines that java.net.URI is an unmapped symbol and does nothing, meaning no expansion is necessary (this of course will fail at evaluation time with "ClassCastException java.lang.Class cannot be cast to clojure.lang.IFn").

In the case of (URI "http://google.com"), lookupVar finds a symbol mapped to something that's not a var and throws the RuntimeException that is seen.

I would expect that neither of these should throw an error during macroexpansion (basically the same thing they start as) and that both should throw the same error when evaluated. Attaching a patch that will only throw the error if internNew - in that case you unexpectedly found something other than a var and you should still report (otherwise, just return null - lookupVar didn't find a var).

The internNew case comes up with:

(import java.net.URI) 
(def URI "abc") ;; java.lang.RuntimeException: Expecting var, but URI is mapped to class java.net.URI

with the patch:

user=> (macroexpand '(java.net.URI "http://google.com")) 
(java.net.URI "http://google.com") 
user=> (macroexpand '(URI "http://google.com")) 
(URI "http://google.com") 
user=> (java.net.URI "http://google.com") 
ClassCastException java.lang.Class cannot be cast to clojure.lang.IFn user/eval9 (NO_SOURCE_FILE:6) 
user=> (URI "http://google.com") 
ClassCastException java.lang.Class cannot be cast to clojure.lang.IFn user/eval11 (NO_SOURCE_FILE:7)




[CLJ-1870] Reloading a defmulti nukes metadata on the var Created: 22/Dec/15  Updated: 18/Jan/16

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

Type: Defect Priority: Critical
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: ft, metadata, multimethods

Attachments: Text File 0001-CLJ-1870-don-t-destroy-defmulti-metadata-on-reload.patch    
Patch: Code
Approval: Prescreened

 Description   

Reloading a defmulti expression destroys any existing (or new) metadata on that multimethod's var:

user=> (defmulti foo "docstring" :op)
#'user/foo
user=> (-> #'foo meta :doc)
"docstring"
user=> (defmulti foo "docstring" :op)
nil
user=> (-> #'foo meta :doc)
nil

This is highly problematic for tools.analyzer, since it relies on such metadata to convey information to the pass scheduler about pass dependencies.

This means that any code that uses core.async cannot be reloaded using `require :reload-all`, since it will cause tools.analyzer to reload and the passes to scheduled in a random order. See ASYNC-154 for one example.

Cause: defmulti has defonce semantics and the first def does not re-apply meta.

Approach: Re-apply meta before first def.

After patch:

user=> (defmulti foo "docstring" :op)
#'user/foo
user=> (-> #'foo meta :doc)
"docstring"
user=> (defmulti foo "docstring" :op)
nil
user=> (-> #'foo meta :doc)
"docstring"

Patch: 0001-CLJ-1870-don-t-destroy-defmulti-metadata-on-reload.patch

Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 23/Dec/15 10:16 AM ]

Related: CLJ-900 CLJ-1446

Comment by Alex Miller [ 23/Dec/15 10:42 AM ]

This patch doesn't compile for me: RuntimeException: Unable to resolve symbol: mm in this context, compiling:(clojure/core.clj:1679:17)

Also, please build the patch with more context - use -U10 with git format-patch to expand it.

Comment by Nicola Mometto [ 23/Dec/15 11:06 AM ]

Updated patch fixing the typo & using -U10

Comment by Alex Miller [ 23/Dec/15 11:17 AM ]

Now:

java.lang.RuntimeException: Too many arguments to def, compiling:(clojure/core.clj:3561:1)

Comment by Nicola Mometto [ 23/Dec/15 11:22 AM ]

whoops. Sorry for this, here's the updated (and working) patch





[CLJ-1364] Primitive VecSeq does not implement equals or hashing methods Created: 19/Feb/14  Updated: 18/Jan/16

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: collections, ft

Attachments: Text File clj-1364.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

VecSeq (as produced by (seq (vector-of :int 1 2 3))) does not implements equals, hashCode, or hasheq and does not play with any other Clojure collections or sequences appropriately in this regard.

user=> (def rs (range 3))
user=> (def vs (seq (vector-of :int 0 1 2)))
user=> rs
(0 1 2)
user=> vs
(0 1 2)
user=> (.equals rs vs)
true
user=> (.equals vs rs)    ;; expect: true
false
user=> (.equiv rs vs)
true
user=> (.equiv vs rs)
true
user=> (.hashCode rs)
29824
user=> (.hashCode vs)     ;; expect to match (.hashCode rs)
2081327893
user=> (System/identityHashCode vs)  ;; show that we're just getting Object hashCode
2081327893
user=> (.hasheq rs)
29824
user=> (.hasheq vs)       ;; expect same as (.hasheq rs) but not implemented at all
IllegalArgumentException No matching field found: hasheq for class clojure.core.VecSeq  clojure.lang.Reflector.getInstanceField (Reflector.java:271)

Approach: Implement Object.hashCode(), Object.equals(), and IHashEq.hasheq() in the primitive vector seq implementation. All of these leverage the prim vec seq itself rather than the underlying prim vec as it was quite a big simpler. The hasheq() impl calls Murmur3/hashOrdered, which takes an Iterable, so Iterable was also implemented using an iterator over the seq.

Some existing tests were expanded to include coverage of the primitive vec seq.

Patch: clj-1364.patch






[CLJ-1730] Improve `refer` performance Created: 13/May/15  Updated: 18/Jan/16

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: ft, performance

Attachments: Text File refer-perf.patch    
Patch: Code
Approval: Prescreened

 Description   

refer underlies require, use, and refer-clojure use cases and is not particularly efficient at its primary job of copying symbol/var mapping from one namespace to another.

Approach: Some improvements that can be made:

  • Go directly to the namespace mappings and avoid creating filtered intermediate maps (ns-publics)
  • Use transients to build map of references to refer
  • Instead of cas'ing each new reference individually, build map of all changes, then cas
  • For (:require :only ...) case - instead of walking all referred vars and looking for matches, walk only the included vars and look up each one

There are undoubtedly more dramatic changes (like immutable namespaces) in how all this works that could further improve performance but I tried to make the scope small-ish for this change.

While individual refer timings are greatly reduced (~50% reduction for (refer clojure.core), ~90% reduction for :only use), refer is only a small component of broader require load times so the improvements in practice are modest.

Performance:

expr in a new repl 1.7.0-beta3 1.7.0-beta3+patch
(in-ns 'foo) (clojure.core/refer 'clojure.core) 2.65 ms 0.994 ms
(in-ns 'bar) (clojure.core/refer 'clojure.core :only '[inc dec]) 1.04 ms 0.113 ms
(use 'criterium.core) 0.877 ms 0.762 ms
(require '[clojure.core.async :refer (>!! <!! chan close!)]) 3408 ms 3302 ms

Patch: refer-perf.patch






[CLJ-1705] vector-of throws NullPointerException if given unrecognized type Created: 14/Apr/15  Updated: 18/Jan/16

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

Type: Defect Priority: Minor
Reporter: John Croisant Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: checkargs, errormsgs, ft
Environment:

MacOS X Version 10.9.5

java version "1.8.0_11"
Java(TM) SE Runtime Environment (build 1.8.0_11-b12)
Java HotSpot(TM) 64-Bit Server VM (build 25.11-b03, mixed mode)


Attachments: Text File clj-1705-2.patch     Text File clj-1705-3.patch     Text File CLJ-1705.patch    
Patch: Code and Test
Approval: Prescreened

 Description   

Summary:

If the user passes an unrecognized type keyword to vector-of, it will throw a NullPointerException with no message. This gives no indication to the user of what the problem is, which can frustrate the user and make debugging harder than it needs to be.

Example:

user=> (vector-of :integer 1 2 3) ; user meant (vector-of :int 1 2 3)

NullPointerException   clojure.core/vector-of (gvec.clj:472)

Approach: Throw more informative error message than NPE with more info.

After:

user=> (vector-of :integer 1 2 3)
IllegalArgumentException Unrecognized type :integer  clojure.core/vector-of (gvec.clj:498)

Patch: clj-1705-3.patch

Screened by: Alex Miller



 Comments   
Comment by Johan Mena [ 05/May/15 12:12 AM ]

Here’s my proposed solution: https://github.com/jhn/clojure/commit/0522fffd7893e8c79969f5c6ac91a333484c8e23 — It works as expected and the tests pass.

One more thing I'm not sure about is how to create a patch for this. The ticket mentions "Release 1.4, Release 1.6” as affected versions, but I found this to still be present in 1.7 and in 1.5 too. Do I checkout only these two tags (1.4, 1.6) from the git repo and create a patch for each one separately? Or what's the usual way to go about it? I read through the Issue Tracking section of the wiki but it's still not very clear.

Thanks.

Johan

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

in general, just work from master. We don't generally back port to older versions.

You should make a patch following the instructions at http://dev.clojure.org/display/community/Developing+Patches

Comment by Andy Fingerhut [ 05/May/15 10:20 AM ]

Johan, are you listed as "johan (jhn)" on the contributor list here? http://clojure.org/contributing

If so, it would be good to update that listing to something like "Johan Mena (jhn)" for the time when someone needs to check that you have signed a Clojure CA, before your patch is committed.

Comment by Alex Miller [ 05/May/15 10:35 AM ]

I believe that's correct - I have updated the contributing page.

Comment by Johan Mena [ 05/May/15 1:57 PM ]

The patch I attached yesterday did start from master, so I think it's good to go for review.

Thanks Alex & Andy!

Comment by Alex Miller [ 06/May/15 9:56 AM ]

Ok, I looked at the patch. Functionally, seems ok other than that the find+destructuing doesn't seem to have a use over get.

However, I also looked at performance for the happy path using criterium and the following test:

(quick-bench (vector-of :int 1 2))

Before the patch: 27.418350 ns
After the patch: 79.883695 ns

The reason for this is that the prior code created a single map and constructed the array managers in there once, whereas the patch causes the array manager to be created each time through the code.

Re-extracting the map and replacing the find with get, I was back to: 34.320916 ns

I think that's too much of a hit for this change so I looked at a couple variants:

  • def'ing each am separately and using a `case` expression - 36.566750 ns
  • using `or` instead of if-let - 29.680252 ns

I think that last one is pretty close, but we're paying a hit just to invoke the new function, so my final stab was turning that into a macro - 28.411626 ns, which seems tolerable to me. Because I used a macro, I also had to move the type hints in vector-of to avoid reflection.

Since I had everything sitting here, I went ahead and made a new patch. I feel bad about replacing yours, but not sure what else makes sense to do.

Comment by Alex Miller [ 06/May/15 10:02 AM ]

Ok, I split up the patch into test and code commits so you have credit for some of the changes! Since it's got my stuff in here, this will have to wait till it gets added to 1.8 to get screened by someone else.

Comment by Johan Mena [ 06/May/15 11:04 AM ]

Haha, you shouldn't have bothered, but thanks!

And thanks for the detailed explanation! Actually 'get' was my first approach too but the (get map key not-found) form kept throwing the exception in the `not-found` part even in the happy case. I asked around and found out the else part needs to be evaluated in this case too. Someone suggested using a macro but I'm not very familiar with that just yet, so I went with if-let, which seemed readable. Just out of curiosity, when you tried the get approach, did you use (get map key) and check for nil to throw the exception?

I ran my code through the irc channel and got positive feedback, so completely forgot about performance. Will keep it in mind next time.





[CLJ-1826] drop-last docstring refers to 'coll' args refer to 's' Created: 13/Oct/15  Updated: 18/Jan/16

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

Type: Enhancement Priority: Trivial
Reporter: Marc O'Morain Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, ft

Attachments: Text File 0001-CLJ-1826-drop-last-docstring-refers-to-coll-args-ref.patch    
Patch: Code
Approval: Prescreened

 Description   

The doc-string for drop-last refers to coll, but the arguments are named n and s, rather than coll.

user=> (doc drop-last)
-------------------------
clojure.core/drop-last
([s] [n s])
  Return a lazy sequence of all but the last n (default 1) items in coll
nil
user=> (source drop-last)
(defn drop-last
  "Return a lazy sequence of all but the last n (default 1) items in coll"
  {:added "1.0"
   :static true}
  ([s] (drop-last 1 s))
  ([n s] (map (fn [x _] x) s (drop n s))))
nil


 Comments   
Comment by Yen-Chin, Lee [ 20/Oct/15 9:39 AM ]

Patch based on current master. (f76b343d)

Comment by Alex Miller [ 20/Oct/15 9:51 AM ]

Thanks, looks good. This won't make it in 1.8 but we will look at it in the next release.





[CLJ-1837] Improve wording of index-of and last-index-of doc strings Created: 29/Oct/15  Updated: 18/Jan/16

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

Type: Enhancement Priority: Trivial
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, ft

Attachments: Text File clj-1837-v1.patch    
Patch: Code
Approval: Prescreened

 Description   

Feel free to decline this if it is too trivial. I was reviewing doc strings for new functions in Clojure 1.8.0, and those for clojure.string/index-of and clojure.string/last-index-of seem like they could be worded in a way that is much less subject to confusion.

Current doc string for clojure.string/index-of:

Return index of value (string or char) in s, optionally searching
  forward from from-index or nil if not found.

Issue: the lack of punctuation in the phrase "optionally searching forward from from-index or nil if not found" makes it appear that the "or" connects the first and last part of that phrase.

Suggested clarification:

Return index of value (string or char) in s, optionally searching
  forward from from-index.  Returns nil if value not found.

Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 01/Jan/16 5:01 AM ]

Patch clj-1837-v1.patch dated Jan 1 2016 modifies the doc strings of index-of and last-index-of as suggested.

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

I retracted my last comment, I think it's fine as is.





[CLJ-1885] data/diff does not return a tuple when comparing different maps Created: 16/Jan/16  Updated: 16/Jan/16

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

Type: Defect Priority: Minor
Reporter: Eric Dvorsak Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

all


Attachments: Text File CLJ-1885.patch     Text File CLJ-1885-tests.patch    
Approval: Triaged

 Description   

Problem: clojure.data/diff inconsistently returns a lazy seq when comparing different maps, but a vector otherwise.

user> (data/diff {:a 1 :b 2} {:a 1})
({:b 2} nil {:a 1})

This is inconsistent with doc and normal behavior :

user> (data/diff {:a 1 :b 2} {:a 1 :b 2})
[nil nil {:a 1, :b 2}]
user> (data/diff #{1 2 3} #{1 2 3})
[nil nil #{1 3 2}]
user> (data/diff #{1 2 3} #{1 2})
[#{3} nil #{1 2}]

The docstring states: "Recursively compares a and b, returning a tuple of [things-only-in-a things-only-in-b things-in-both]", implying that it should always return a vector.



 Comments   
Comment by Eric Dvorsak [ 16/Jan/16 10:02 AM ]

Fixing it just requires to vectorize diff-associative output like this :

(defn- diff-associative
  "Diff associative things a and b, comparing only keys in ks."
  [a b ks]
  (vec (reduce
   (fn [diff1 diff2]
     (doall (map merge diff1 diff2)))
   [nil nil nil]
   (map
    (partial diff-associative-key a b)
    ks))))
Comment by Alex Miller [ 16/Jan/16 10:10 AM ]

There are other potential ways to address this, such as by using transducers instead. Not sure if that's worth doing, but seems reasonable to consider while we're making changes.

Comment by Eric Dvorsak [ 16/Jan/16 10:15 AM ]

Maybe this could be done as an improvement and proposed in an other ticket.

Vec is already used to vectorize the lists in diff-sequential. I would suggest to just fix the bug and add the test cases that should have screen it.

Comment by Eric Dvorsak [ 16/Jan/16 10:20 AM ]

There is a test case that should already fail :

[{:a #{2}} {:a #{4}} {:a #{3}}] {:a #{2 3}} {:a #{3 4}}

I get

({:a #{2}} {:a #{4}} {:a #{3}})
Comment by Alex Miller [ 16/Jan/16 10:33 AM ]

The test may need to be made more strict, checking not just for sequential equality but also for a returned vector.

Just curious - was this issue causing a problem in your code or did you just notice it and find it surprising?

Comment by Eric Dvorsak [ 16/Jan/16 11:05 AM ]

Simple patch that just does for maps what is done for lists : Creates a new vector with the vec function.

Comment by Eric Dvorsak [ 16/Jan/16 11:08 AM ]

@Alex Miller : I noticed a bug in my program behavior and traced it down to a (get diff 2) instead of (nth diff 2), but I realized that it was only buggy in some cases so I looked further and found out if was coming from diff.

Comment by Eric Dvorsak [ 16/Jan/16 11:27 AM ]

More strict tests checking for a returned vector.





[CLJ-1452] clojure.core/*rand* for seedable randomness Created: 20/Jun/14  Updated: 14/Jan/16

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

Comment by Ghadi Shayban [ 14/Jan/16 10:15 AM ]

Just noting, ThreadLocalRandom is >= JDK 7.





[CLJ-1884] Add support for two parameters to rand and rand-int Created: 14/Jan/16  Updated: 14/Jan/16

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

Type: Enhancement Priority: Minor
Reporter: Juan José Conti Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File rand_with_two_params1.patch    
Patch: Code and Test

 Description   

I'd like to propose a change for rand and rand-int. I'd like to add the possibility to call them with two parameters having the result is in that range.

I think it would be a useful change as now, when you want to get a random number in a range you have to google how to do it because you don't remember it and end up with something that is not obvious what it's doing:

(+ a (rand (- b a))

The change is simple (I've done it locally, build and tested it). Patch attached.

https://groups.google.com/forum/#!topic/clojure-dev/hl2XtXNGb8w






[CLJ-1883] references to a symbol ending with ? that came from a foreign namespace confuses the compiler inside a :cljc block Created: 13/Jan/16  Updated: 13/Jan/16  Resolved: 13/Jan/16

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

Type: Defect Priority: Major
Reporter: Geraldo Lopes de Souza Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None

Attachments: File bug.cljc     File bug.tar.bz2    

 Description   

I've found a situation where I'm requiring a om.tempid on a .cljc namespace. The require is
[om.tempid :as tempid :refer [tempid?]]
The reference of tempid? on line 24 of bug.cljc generates
java.lang.IllegalArgumentException: Unable to resolve classname: clojure.core$boolean@7521bffd, compiling/home/geraldo/bug/src/bug/bug.cljc:14:5)

To circumvent I've created a local alias of the tempid? function (line 26). It seems that the ? at the end of the name is causing the trouble because if I reference om.tempid/tempid it does not trigger. Note that the local alias was ended with ? to show that only when is a foreign ns that the problem is present.

Regards and forgive my english.

Geraldo Lopes de Souza



 Comments   
Comment by Alex Miller [ 13/Jan/16 8:20 AM ]

tempid? is:

(defn ^boolean tempid? [x]
  (instance? TempId x))

I suspect the ? has nothing to do with it and it's the ^boolean type hint that's the issue.

Comment by Alex Miller [ 13/Jan/16 8:27 AM ]

This is a bug in Om in using a type hint in non-conditional code that is cljs-specific. David Nolen is fixing in Om.

Comment by Alex Miller [ 13/Jan/16 8:27 AM ]

https://github.com/omcljs/om/commit/cc39f37561455a54153aaef7e5ca36782839aa38

Comment by Geraldo Lopes de Souza [ 13/Jan/16 4:38 PM ]

Alex,

I saw the other issue referencing the ^boolean but I didn't relate with this.

Thank you,

Geraldo Lopes de Souza





[CLJ-1860] 0.0 and -0.0 compare equal but have different hash values Created: 01/Dec/15  Updated: 12/Jan/16

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

Type: Defect Priority: Major
Reporter: Patrick O'Brien Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: math

Approval: Triaged

 Description   

0.0 and -0.0 compare as equal but have different hash values:

user=> (= 0.0 -0.0)
true
user=> (hash -0.0)
-2147483648
user=> (hash 0.0)
0

This causes problems as the equality/hashing assumption is violated.

user=> #{[1 2 0.0] [1 2 -0.0]}
#{[1 2 -0.0] [1 2 0.0]}

user=> (hash-map 0.0 1 -0.0 2)
{0.0 2}

user=> (hash-map [0.0] 1 [-0.0] 2)
{[0.0] 1, [-0.0] 2}

user=> (array-map [0.0] 1 [-0.0] 2)
{[0.0] 2}

user=> (hash-set [0.0] [-0.0])
#{[0.0] [-0.0]}

Cause: Apparently there is some lurking Java weirdness here. In Java, primitive positive and negative doubles are == but the boxed form is NOT .equals(). See also: http://docs.oracle.com/javase/7/docs/api/java/lang/Double.html#equals%28java.lang.Object%29

Double equality is checked with == in Clojure, which will report true. Hashing falls through to .hashCode(), which returns different values (but is consistent with the .equals() result on the boxed form).

Approach: There are two approaches - make 0.0 != -0.0, or make their hashes the same. Java takes the former approach in the boxed Double so that seems like what we should do.

Patch:






[CLJ-1678] Update failing tests for IBM JDK 1.7 and 1.8 Created: 19/Mar/15  Updated: 12/Jan/16

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

Type: Enhancement Priority: Trivial
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: test
Environment:

IBM JDK 1.7 and 1.8


Approval: Triaged

 Description   

For Sun/Oracle JDKs, and IBM JDK 1.6, we have this:

user=> (.hashCode 9223372039002259457N)
1

For IBM JDKs 1.7 and 1.8, it changed to this (I do not know why):

user=> (.hashCode 9223372039002259457N)
33

This causes a few example-based tests in Clojure to fail when run on those IBM JDK versions. There does not appear to be any bug in Clojure here. Those tests were written with particular constant values that are different, but have equal .hashCode values, to test Clojure's code generated that selects between branches in a case. In particular, these tests in control.clj fail:

;; line 386 in Clojure 1.6.0 and 1.7.0-master-SNAPSHOT as of Mar 19 2015:
    (is (== (.hashCode 1) (.hashCode 9223372039002259457N)))

;; and later on line 423 in the same file:
  (testing "test warn for hash collision"
    (should-print-err-message
     #"Performance warning, .*:\d+ - hash collision of some case test constants; if selected, those entries will be tested sequentially..*\r?\n"
     (case 1 1 :long 9223372039002259457N :big 2)))

There are other tests in the same file with the same constant 9223372039002259457N that do not fail with IBM JDKs 1.7 and 1.8, but they do not test hash collisions as they were intended to.

Some possibilities for what could be changed:

1. Pick a different pair of number other than 1 and 9223372039002259457N when running tests on IBM JDKs 1.7 and 1.8, so that the hash values do collide. For example, 33 and 9223372039002259457N.

2. skip these tests completely when running on IBM JDKs 1.7 and 1.8.



 Comments   
Comment by Alex Miller [ 20/Mar/15 4:03 AM ]

I think my preference would be to skip these tests for the ibm jdk.





[CLJ-1381] Improve support for extending protocols to primitive arrays Created: 13/Mar/14  Updated: 12/Jan/16  Resolved: 12/Jan/16

Status: Closed
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: Duplicate Votes: 5
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
Comment by Alex Miller [ 12/Jan/16 3:16 PM ]

Dupe of CLJ-1790





[CLJ-1643] Generative test for sequence implementations Created: 15/Jan/15  Updated: 12/Jan/16

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: generative-test, test

Attachments: Text File clj-1643-gen-seq-test-v1.patch    
Patch: Code and Test
Approval: Triaged

 Description   

This is an attempt to write a minimal-foreknowledge failing test for CLJ-1633. By minimal-foreknowledge, I mean a test that fails in the presence of the bug, but which one could imagine writing without intimate knowledge of the details of the bug. I suspect that looking for tests like this is a good way to find gaps in test coverage, and produce tests that will uncover novel regressions later on.

Approach: Generate a single list of operations that could be performed on a sequence, changing that sequence. Make two copies of that operation list, and insert what should be identity-preserving operations into each. Run the two lists of operations and verify that the final results are the same.

With CLJ-1633 unfixed, we get this output:

[java] Testing clojure.test-clojure.sequences
     [java]
     [java] FAIL in (seq-gentest) (sequences.clj:135)
     [java] {:acts1 (->> nil (cons :foo) (cons :foo) into-array next (apply list)),
     [java]  :acts2 (->> nil (cons :foo) (cons :foo) next),
     [java]  :result1 (:foo :foo),
     [java]  :result2 (:foo),
     [java]  :pass false}
     [java]
     [java] expected: (:result res)
     [java]   actual: false





[CLJ-1630] Destructuring allows multiple &-params Created: 31/Dec/14  Updated: 12/Jan/16

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

Type: Defect Priority: Minor
Reporter: Michael Blume Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: destructuring, errormsgs

Attachments: Text File CLJ-1630-v2.patch     Text File no-multiple-rest-params-v1.patch    
Patch: Code and Test
Approval: Triaged

 Description   

(let [[foo & bar & baz] []]) compiles and probably shouldn't.



 Comments   
Comment by Alex Miller [ 01/Jan/15 10:17 AM ]

I see:

user=> (defn foo [bar & baz & qux])

CompilerException java.lang.RuntimeException: Invalid parameter list, compiling:(/private/var/folders/7r/_1fj0f517rgcxwx79mn79mfc0000gn/T/form-init3743582784321941885.clj:1:1)

?

Comment by Michael Blume [ 01/Jan/15 12:27 PM ]

Sorry, I was working on memory rather than actually typing the thing I put in the description into a REPL, which was dumb.

user=> (let [[foo & bar & baz] []])
nil





[CLJ-1472] The locking macro fails bytecode verification on ART runtime Created: 23/Jul/14  Updated: 12/Jan/16

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

Type: Defect Priority: Minor
Reporter: Adam Clements Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: android
Environment:

Android ART runtime


Attachments: Text File 0001-CLJ-1472-Locking-macro-without-explicit-monitor-ente.patch     Text File clj-1472-2.patch     Text File clj-1472.patch    
Patch: Code
Approval: Vetted

 Description   

Android ART runs compile time verification on bytecode and fails on any usage of the locking macro. The error looks like that seen in CLJ-1829 (in that case clojure.core.server/stop-server calls the locking macro):

10-16 14:49:26.801 2008-2008/? E/AndroidRuntime: java.lang.VerifyError: Rejecting class clojure.core.server$stop_server because it failed compile-time verification (declaration of 'clojure.core.server$stop_server' appears in /data/app/com.clojure_on_android-1/base.apk)

Cause: From discussion on an Android issue (https://code.google.com/p/android/issues/detail?id=80823), it seems this is due to more strictly enforcing the "structural locking" provisions in the JVM spec (https://docs.oracle.com/javase/specs/jvms/se7/html/jvms-2.html#jvms-2.11.10).

It appears that the mixture of monitor-enter, monitor-exit, and the try/finally block in `locking` create paths that ART is flagging as not having balanced monitorenter/monitorexit bytecode. Particularly, monitorenter and monitorexit themselves can throw (on a null locking object). The Java bytecode does some tricky exception table handling to cover these cases which (afaict) is not possible to do without modifying the Clojure compiler.

Approach: One possible approach is to have locking invoke the passed body in the context of a synchronized block in Java. This avoids the issue of the tricky bytecode by handing it over to Java but has unknown performance impacts.

Patch: clj-1472-2.patch

See also: Examination of the bytecode as compared to a java synchronized block shows up a number of differences:
https://gist.github.com/AdamClements/2ae6c4919964b71eb470



 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.

Comment by Ghadi Shayban [ 21/Dec/15 12:19 PM ]

According to Marcus Lagergren JRockit and Hotspot both account for lock releases being in a different method... Perhaps Android has a different (wrong) interpretation?

Comment by Alex Miller [ 21/Dec/15 3:33 PM ]

I added a new clj-1472.patch that fixes a few minor details I didn't like about the prior patch. However, it is still essentially the same change so I have retained the original author attribution.

Comment by Ghadi Shayban [ 21/Dec/15 4:14 PM ]

alex did you upload the right patch?

Comment by Alex Miller [ 21/Dec/15 4:58 PM ]

Ah, no messed that up. Will fix.

Comment by Nicola Mometto [ 22/Dec/15 3:54 AM ]

Alex, it's probably worth making that a `^:once fn*`

Comment by Ghadi Shayban [ 22/Dec/15 1:05 PM ]

From Android:

Android doesn't run Java bytecode, it runs Dex bytecode. The dexdump output of what your class translates to is interesting.

Neither is the JVMS interesting. Android isn't a Java Virtual Machine. We follow the JLS, but not the JVMS (how could we, when we don't run Java bytecode). As such, all appeals against it are irrelevant. We try to be compatible to the spirit of the JVMS wrt/ Dex bytecode, but if your source isn't Java, there are no guarantees.

Now, the verifiers were (and probably still are) broken, even against our (pretty bad) spec, and regrettably we aren't very consistent. In Marshmallow, for example, a lot of the code we can't correctly verify wrt/ structured locking is rejected as a VerifyError, which is not in the spirit of the JVMS. In the next release, this will be relaxed, however, and postponed to an actual check while running the code.

Regrettably there isn't anything we can do about old releases, you'll have to work around any issues. I'll try to take a look at your class when I find the time.

Sounds like making a workaround in Clojure is the least of all evils.

Comment by Alex Miller [ 22/Dec/15 1:46 PM ]

Added -2 patch with ^:once.

Comment by Alex Miller [ 12/Jan/16 10:30 AM ]

Our current belief is that Android is at fault here, so backlogging this for now.





[CLJ-1509] Some clojure namespaces not AOT-compiled and included in the clojure jar Created: 20/Aug/14  Updated: 12/Jan/16

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

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

Patch: Code

 Description   

There is a list of namespaces to AOT in build.xml and several namespaces are missing from that list, thus no .class files for those namespaces are created or included in the standard clojure jar file as part of the build.

Missing namespaces include:

  • clojure.core.reducers
  • clojure.instant
  • clojure.parallel
  • clojure.uuid

Proposal: Attached patch sorts the ns list alphabetically (for easier maintenance) and adds clojure.instant and clojure.uuid to the compiled namespaces. clojure.parallel is deprecated and requires the JSR-166 jar so was not included (perhaps it's a separate ticket to remove this). clojure.core.reducers uses a compile-time check to choose the fork/join packages to use so cannot be compiled early.

Patch: clj-1509.diff

Screened by:



 Comments   
Comment by Alex Miller [ 20/Aug/14 1:06 PM ]

Looking at this a bit further, clojure.core.reducers uses the compile-if macro to determine what version of fork/join is available so AOT-compiling this namespace would fix that decision at build time rather than runtime, so it cannot be included.





[CLJ-1818] cl-format does not respect aesthetic ~A with a keyword Created: 26/Sep/15  Updated: 12/Jan/16

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

Type: Defect Priority: Trivial
Reporter: Jong-won Choi Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: print

Approval: Triaged

 Description   

In Common Lisp, (format nil "~a" :A) returns "A". However, in Clojure, it returns with the colon:

(clojure.pprint/cl-format false "~a" :A)
=> ":A"


 Comments   
Comment by Jong-won Choi [ 28/Sep/15 6:26 AM ]

Found another problem of cl-format:

(clojure.pprint/cl-format false "SELECT * from RateSchedules ~@[WHERE ~{~A=?~^ ~}~]" '())
=> "SELECT * from RateSchedules WHERE" ;; instead of "SELECT * from RateSchedules"

I guess the problem is () or [] has to be treated as falsey but not.

Comment by Alex Miller [ 28/Sep/15 9:58 AM ]

:a is a keyword and I would expect it's ascii format to be :a. I'm not sure what case sensitivity has to do with it.

Comment by Andy Fingerhut [ 28/Sep/15 10:08 AM ]

Alex, case is a side issue. Common Lisp's (format nil "~a" :A) returns "A", not ":A". It is the presence of the colon in the output that is the issue, not the case of the string.

Comment by Jong-won Choi [ 28/Sep/15 4:41 PM ]

For a record, what Alex described is for ~S - standard. See http://www.lispworks.com/documentation/lw50/CLHS/Body/22_cd.htm





[CLJ-1733] print-dup form unreadable for sorted sets and maps Created: 19/May/15  Updated: 12/Jan/16

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

Type: Defect Priority: Minor
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-1872] empty? is broken for transient collections Created: 26/Dec/15  Updated: 12/Jan/16

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

Type: Defect Priority: Critical
Reporter: Leonid Bogdanov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections

Approval: Triaged

 Description   

Couldn't find whether it was brought up earlier, but it seems that empty? predicate is broken for transient collections

user=> (empty? (transient []))
IllegalArgumentException Don't know how to create ISeq from: clojure.lang.PersistentVector$TransientVector  clojure.lang.RT.seqFrom (RT.java:528)

user=> (empty? (transient ()))
ClassCastException clojure.lang.PersistentList$EmptyList cannot be cast to clojure.lang.IEditableCollection  clojure.core/transient (core.clj:3209)

user=> (empty? (transient {}))
IllegalArgumentException Don't know how to create ISeq from: clojure.lang.PersistentArrayMap$TransientArrayMap  clojure.lang.RT.seqFrom (RT.java:528)

user=> (empty? (transient #{}))
IllegalArgumentException Don't know how to create ISeq from: clojure.lang.PersistentHashSet$TransientHashSet  clojure.lang.RT.seqFrom (RT.java:528)

The workaround is to use (zero? (count (transient ...))) check instead.



 Comments   
Comment by Alex Miller [ 26/Dec/15 9:58 PM ]

Probably similar to CLJ-700.





[CLJ-1372] Inconsistent hash with java collections Created: 09/Mar/14  Updated: 12/Jan/16

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

Type: Defect Priority: Minor
Reporter: Jozef Wagner Assignee: Unassigned
Resolution: Unresolved Votes: 9
Labels: collections, interop
Environment:

1.6.0 master


Attachments: Text File 0001-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M-alternative.patch     Text File 0001-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M.patch     Text File 0001-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M-substring.patch     Text File 0005-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M.patch     Text File 0006-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M.patch     Text File 0007-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M.patch     File clj-1372-2.diff     File clj-1372.diff    
Patch: Code and Test
Approval: Vetted

 Description   

c.c/hash always use hashCode for java collections, which is incompatible when comparing with Clojure collections, which use Murmur3.

user=> (== (hash (java.util.ArrayList. [1 2 3])) (hash [1 2 3]))
false
user=> (= (java.util.ArrayList. [1 2 3]) [1 2 3])
true

One way to fix it is to add a special case in Util/hasheq for java.util.Collections, as it is now for Strings.

Link to a discussion of this topic in the Clojure group: https://groups.google.com/forum/#!topic/clojure/dQhdwZsyIEw



 Comments   
Comment by Jozef Wagner [ 09/Mar/14 8:02 AM ]

Same problem for maps, so hasheq should have a special case for java.util.Map too.

Comment by Jozef Wagner [ 09/Mar/14 9:21 AM ]

Added patch with fix for j.u. Map, Set and List.

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

Add patch clj-1372-2.diff that is identical to Jozef Wagner's clj-1372.diff, except it also adds some new tests that fail without his changes, and pass with them.

Comment by Alex Miller [ 10/Mar/14 9:31 AM ]

I think the contract on equiv/hasheq is more narrowly scoped than this and only applies if both collections are IPersistentCollection. In other words, I don't think this is wanted or required.

Note that the Java .equals/.hashCode contract is maintained here - these collections will compare as .equals() and do have the same .hashCode().

Comment by Jozef Wagner [ 10/Mar/14 9:38 AM ]

Without the patch the following statement is not valid: "If two objects are equal with c.c/=, than their hash returned by c.c/hash is the same number". We can say that this is valid only iff both objects are 'clojure' objects, but this goes against clojures interop principles (interop is easy, fast, no surprises).

Comment by Jozef Wagner [ 10/Mar/14 9:54 AM ]

Manifestation of this bug

user=> (assoc (hash-map [1 2 3] :foo) (java.util.ArrayList. [1 2 3]) :bar)
{[1 2 3] :bar, [1 2 3] :foo}
user=> (get (hash-map [1 2 3] :foo) (java.util.ArrayList. [1 2 3]))
nil
Comment by Alex Miller [ 10/Mar/14 10:58 AM ]

I agree that would be a nice thing to say without qualification.

There is a real cost to adding more branches in hasheq - adding those collection checks affects every hasheq. Running a full Clojure build, I see the following set of classes with >100 occurences where this happens (note that exactly 0 of these are the Java collections - this case doesn't exist in the Clojure build itself):

clojure.lang.Var 107001502
java.lang.Class 2651389
java.lang.Character 2076322 
java.util.UUID 435235 
java.util.Date 430956
clojure.lang.Compiler$LocalBinding 116830
java.lang.Boolean 112361
java.util.regex.Pattern 325

We'd be adding 4 more instanceof checks in the path of every one of those hasheqs. This would also likely blow any JVM inlining.

Rich says "all bets should be off for hasheq/equiv of non-values" where Java collections obviously fall into the class of "non-values".

Comment by Andy Fingerhut [ 10/Mar/14 11:04 AM ]

Would a doc patch be considered? Say one that modified the doc of clojure.core/hash to include a phrase indicating that it is only promised to be consistent with clojure.core/= for immutable values? It could even perhaps mention that Floats are out, too: see CLJ-1036

Comment by Alex Miller [ 10/Mar/14 12:00 PM ]

I think it would be preferred to do any detailed docs about hash at http://clojure.org/data_structures rather than in the docstring. Although the docstring on hash probably could use an update and a pointer to the web site after the latest changes.

Comment by Jozef Wagner [ 10/Mar/14 12:14 PM ]

Neverthless it is a breaking change from 1.5, and it should be mentioned in changelog. What still bugs me is that c.c/= is supported in such cases but the c.c/hash is not. If supporting c.c/hash is expensive, isn't it better to drop support for c.c/= in such cases? It will eliminate surprises such as:

user=> (apply distinct? (hash-set [1 2 3] (java.util.Collections/unmodifiableList [1 2 3])))
false
Comment by Alex Miller [ 10/Mar/14 2:05 PM ]

I'm not sure it's a "breaking" change if something not considered to be guaranteed changes. But I take your point.

I don't think it's feasible to drop = support for Clojure and Java collections - that seems important and useful. And if it were free to do so, I would like to be able to say without qualification that if equiv=true, then hasheq is the same.

It's unclear to me that the examples listed on this ticket are actually real problems people are likely to encounter. The main users of hasheq are hash map and hash set. So to manifest, you would need to be putting a mixture of Clojure and Java collections into one of those, in particular a mixture of collections that compare as equal.

Still thinking about it.

Comment by Jozef Wagner [ 10/Mar/14 3:27 PM ]

Sorry for spamming but there may be another option, to not fallback into hashCode in hasheq, but to instead throw in cases where hasheq is requested for non-values. This will lead to a cleaner separation of hash types. Of course it will prevent putting non-values into hash-set.

Comment by Alex Miller [ 10/Mar/14 3:34 PM ]

There is no simple check for "valueness" though?

Comment by Andy Fingerhut [ 10/Mar/14 3:37 PM ]

An idea, for what it might be worth: Add one test for instance of java.util.Collection in Util.hasheq method instead of 3 separate tests for Set, List, and Map. It doesn't cover Map.Entry.

Comment by Alex Miller [ 10/Mar/14 3:38 PM ]

Map doesn't extend Collection either.

Comment by Stuart Halloway [ 11/Mar/14 10:44 AM ]

I think this needs more consideration and should not hold up 1.6.

Comment by Andy Fingerhut [ 20/Mar/14 2:01 PM ]

Both patches clj-1372.diff and clj-1372-2.diff fail to apply cleanly as of latest Clojure master on Mar 20 2014. They did apply cleanly before the Mar 19 2014 commit, I believe, and the only issue appears to be a changed line of diff context. Given the discussion about whether such a change is desired, it sounds like more thought is needed before deciding what change should be made, if any.

Comment by Mike Anderson [ 11/May/14 2:31 PM ]

This is a pretty bad defect. It absolutely needs to be fixed. It's not really about whether using a mix of Clojure and Java collections is a likely use case or not (it probably isn't...), it's about providing consistent guarantees that people can rely upon.

For example, now I'm really unsure about whether some of the library functions I have that use sets or maps are broken or not. I'd be particularly worried about anything that implements object caches / memoisation / interning based on hashed values. Such code may now have some really nasty subtle defects.

Since they are library functions, I can't guarantee what kind of objects are passed in so the code has to work with all possible inputs (either that or I need to write a clear docstring and throw an exception if the input is not supported).

Comment by Michał Marczyk [ 12/May/14 11:29 PM ]

This patch (0001-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M.patch) makes hasheq consistent with = for java.util.{List,Map,Map.Entry,Set}. Additionally it extends the special treatment of String (return hasheq of hashCode) to all types not otherwise handled (see below for a comment on this).

It is also available here:

https://github.com/michalmarczyk/clojure/tree/alien-hasheq-2

An earlier version is available here:

https://github.com/michalmarczyk/clojure/tree/alien-hasheq

If I understand correctly, what needs to be benchmarked is primarily the "dispatch time" for clojure.lang.Util/hasheq given a Clojure type. So, I ran a Criterium benchmark repeatedly hashing the same persistent hash map, on the theory that this will measure just the dispatch time on IHashEq instances. I then ran a separate benchmark hashing a PHM, a string and a long and adding up the results with unchecked-add. Hopefully this is a good start; I've no doubt additional benchmarks would be useful.

The results are somewhat surprising to me: hasheq on PHM is actually slightly faster in this benchmark on my build than on 1.6.0; the "add three hasheqs" benchmark is slightly faster on 1.6.0.

;;; 1.6.0

;;; NB. j.u.HM benchmark irrelevant
user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] #_(assert (= (hash phm) (hash juhm))) (c/bench (clojure.lang.Util/hasheq phm)) (c/bench (clojure.lang.Util/hasheq juhm)))
WARNING: Final GC required 1.24405836928592 % of runtime
Evaluation count : 5549560980 in 60 samples of 92492683 calls.
             Execution time mean : 9.229881 ns
    Execution time std-deviation : 0.156716 ns
   Execution time lower quantile : 8.985994 ns ( 2.5%)
   Execution time upper quantile : 9.574039 ns (97.5%)
                   Overhead used : 1.741068 ns

Found 2 outliers in 60 samples (3.3333 %)
	low-severe	 2 (3.3333 %)
 Variance from outliers : 6.2652 % Variance is slightly inflated by outliers
Evaluation count : 35647680 in 60 samples of 594128 calls.
             Execution time mean : 1.695145 µs
    Execution time std-deviation : 20.186554 ns
   Execution time lower quantile : 1.670049 µs ( 2.5%)
   Execution time upper quantile : 1.740329 µs (97.5%)
                   Overhead used : 1.741068 ns

Found 2 outliers in 60 samples (3.3333 %)
	low-severe	 2 (3.3333 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil

user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] #_(assert (= (hash phm) (hash juhm))) (c/bench (unchecked-add (clojure.lang.Util/hasheq phm) (unchecked-add (clojure.lang.Util/hasheq "foo") (clojure.lang.Util/hasheq 123)))))
WARNING: Final GC required 1.028614538339401 % of runtime
Evaluation count : 1029948300 in 60 samples of 17165805 calls.
             Execution time mean : 56.797488 ns
    Execution time std-deviation : 0.732221 ns
   Execution time lower quantile : 55.856731 ns ( 2.5%)
   Execution time upper quantile : 58.469940 ns (97.5%)
                   Overhead used : 1.836671 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil

;;; patch applied

user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] (assert (= (hash phm) (hash juhm))) (c/bench (clojure.lang.Util/hasheq phm)) (c/bench (clojure.lang.Util/hasheq juhm)))
Evaluation count : 5537698680 in 60 samples of 92294978 calls.
             Execution time mean : 8.973200 ns
    Execution time std-deviation : 0.157079 ns
   Execution time lower quantile : 8.733544 ns ( 2.5%)
   Execution time upper quantile : 9.289350 ns (97.5%)
                   Overhead used : 1.744772 ns
Evaluation count : 2481600 in 60 samples of 41360 calls.
             Execution time mean : 24.287800 µs
    Execution time std-deviation : 288.124326 ns
   Execution time lower quantile : 23.856445 µs ( 2.5%)
   Execution time upper quantile : 24.774097 µs (97.5%)
                   Overhead used : 1.744772 ns
nil

user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] #_(assert (= (hash phm) (hash juhm))) (c/bench (unchecked-add (clojure.lang.Util/hasheq phm) (unchecked-add (clojure.lang.Util/hasheq "foo") (clojure.lang.Util/hasheq 123)))))
WARNING: Final GC required 1.298136122909759 % of runtime
Evaluation count : 954751500 in 60 samples of 15912525 calls.
             Execution time mean : 61.681794 ns
    Execution time std-deviation : 0.712110 ns
   Execution time lower quantile : 60.622003 ns ( 2.5%)
   Execution time upper quantile : 62.904801 ns (97.5%)
                   Overhead used : 1.744772 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil

As a side note, the earlier version of the patch available on the other branch doesn't have a separate branch for String. This made hasheq faster for objects implementing IHashEq, but slowed down the "three hashes" benchmark roughly by a factor of 2.

Comment by Alex Miller [ 12/May/14 11:39 PM ]

Just for clarity, please refer to patches attached here by name so as time goes on we don't have to correlate attachment time with comment time.

I'm not particularly worried about the cost of things that implement IHashEq as they should be unaffected other than potential inlining issues. I am curious about the cost of hasheq for objects that fall through to the end of the cases and pay the cost for all of the checks. The list farther up in the comments is a good place to start - things like Class, Character, and Var (which could possibly be addressed in Var).

Comment by Michał Marczyk [ 12/May/14 11:47 PM ]

Good point, I've edited the above comment to include the patch name.

Thanks for the benchmarking suggestions – I'll post some new results in ~6 minutes.

Comment by Michał Marczyk [ 13/May/14 12:18 AM ]

First, for completeness, here's a new patch (0001-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M-alternative.patch) which doesn't do the extra murmuring for types not otherwise handled. It's slower for the single PHM case; see below for details. Also, here's the branch on GitHub:

https://github.com/michalmarczyk/clojure/tree/alien-hasheq-3

As for the new results, the perf hit is quite large, I'm afraid:

;;; with patch (murmur hashCode for default version)
user=> (let [class-instance java.lang.String character-instance \a var-instance #'hash] (c/bench (clojure.lang.Util/hasheq class-instance)) (c/bench (clojure.lang.Util/hasheq character-instance)) (c/bench (clojure.lang.Util/hasheq var-instance)))
WARNING: Final GC required 1.409118084170768 % of runtime
Evaluation count : 655363680 in 60 samples of 10922728 calls.
             Execution time mean : 96.459888 ns
    Execution time std-deviation : 1.019817 ns
   Execution time lower quantile : 95.079086 ns ( 2.5%)
   Execution time upper quantile : 98.684168 ns (97.5%)
                   Overhead used : 1.708347 ns
Evaluation count : 675919140 in 60 samples of 11265319 calls.
             Execution time mean : 88.965959 ns
    Execution time std-deviation : 0.825226 ns
   Execution time lower quantile : 87.817159 ns ( 2.5%)
   Execution time upper quantile : 90.755688 ns (97.5%)
                   Overhead used : 1.708347 ns
Evaluation count : 574987680 in 60 samples of 9583128 calls.
             Execution time mean : 103.881498 ns
    Execution time std-deviation : 1.103615 ns
   Execution time lower quantile : 102.257474 ns ( 2.5%)
   Execution time upper quantile : 106.071144 ns (97.5%)
                   Overhead used : 1.708347 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil

;;; 1.6.0
user=> (let [class-instance java.lang.String character-instance \a var-instance #'hash] (c/bench (clojure.lang.Util/hasheq class-instance)) (c/bench (clojure.lang.Util/hasheq character-instance)) (c/bench (clojure.lang.Util/hasheq var-instance)))
WARNING: Final GC required 1.3353133083866688 % of runtime
Evaluation count : 1829305260 in 60 samples of 30488421 calls.
             Execution time mean : 34.205701 ns
    Execution time std-deviation : 0.379106 ns
   Execution time lower quantile : 33.680636 ns ( 2.5%)
   Execution time upper quantile : 34.990138 ns (97.5%)
                   Overhead used : 1.718257 ns

Found 2 outliers in 60 samples (3.3333 %)
	low-severe	 1 (1.6667 %)
	low-mild	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
Evaluation count : 1858100340 in 60 samples of 30968339 calls.
             Execution time mean : 30.401309 ns
    Execution time std-deviation : 0.213878 ns
   Execution time lower quantile : 30.095976 ns ( 2.5%)
   Execution time upper quantile : 30.871497 ns (97.5%)
                   Overhead used : 1.718257 ns
Evaluation count : 1592932200 in 60 samples of 26548870 calls.
             Execution time mean : 36.292934 ns
    Execution time std-deviation : 0.333512 ns
   Execution time lower quantile : 35.795063 ns ( 2.5%)
   Execution time upper quantile : 36.918183 ns (97.5%)
                   Overhead used : 1.718257 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil

One PHM and Class/Character/Var results with the new patch (no extra murmur step in the default case):

user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] #_(assert (= (hash phm) (hash juhm))) (c/bench (unchecked-add (clojure.lang.Util/hasheq phm) (unchecked-add (clojure.lang.Util/hasheq "foo") (clojure.lang.Util/hasheq 123)))))
WARNING: Final GC required 1.258952964663877 % of runtime
Evaluation count : 1007768460 in 60 samples of 16796141 calls.
             Execution time mean : 58.195608 ns
    Execution time std-deviation : 0.482804 ns
   Execution time lower quantile : 57.655857 ns ( 2.5%)
   Execution time upper quantile : 59.154655 ns (97.5%)
                   Overhead used : 1.567532 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil
user=> (let [class-instance java.lang.String character-instance \a var-instance #'hash] (c/bench (clojure.lang.Util/hasheq class-instance)) (c/bench (clojure.lang.Util/hasheq character-instance)) (c/bench (clojure.lang.Util/hasheq var-instance)))
Evaluation count : 647944080 in 60 samples of 10799068 calls.
             Execution time mean : 91.275863 ns
    Execution time std-deviation : 0.659943 ns
   Execution time lower quantile : 90.330980 ns ( 2.5%)
   Execution time upper quantile : 92.711120 ns (97.5%)
                   Overhead used : 1.567532 ns
Evaluation count : 699506160 in 60 samples of 11658436 calls.
             Execution time mean : 84.564131 ns
    Execution time std-deviation : 0.517071 ns
   Execution time lower quantile : 83.765607 ns ( 2.5%)
   Execution time upper quantile : 85.569206 ns (97.5%)
                   Overhead used : 1.567532 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
Evaluation count : 594919980 in 60 samples of 9915333 calls.
             Execution time mean : 100.336792 ns
    Execution time std-deviation : 0.811312 ns
   Execution time lower quantile : 99.313490 ns ( 2.5%)
   Execution time upper quantile : 102.167675 ns (97.5%)
                   Overhead used : 1.567532 ns

Found 3 outliers in 60 samples (5.0000 %)
	low-severe	 3 (5.0000 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil
Comment by Michał Marczyk [ 13/May/14 1:05 AM ]

Here's a new patch (0001-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M-substring.patch) that takes the outrageous approach of replacing the Iterable/Map/Entry test with a .startsWith("java.util.") on the class name. (I experimented with .getClass().getPackage(), but the performance of that was terrible.) The branch is here:

https://github.com/michalmarczyk/clojure/tree/alien-hasheq-4

Hash perf on the "fall-through" cases with this patch seems to be very good:

user=> (let [class-instance java.lang.String character-instance \a var-instance #'hash] (c/bench (clojure.lang.Util/hasheq class-instance)) (c/bench (clojure.lang.Util/hasheq character-instance)) (c/bench (clojure.lang.Util/hasheq var-instance)))
WARNING: Final GC required 1.31690036780011 % of runtime
Evaluation count : 1661453640 in 60 samples of 27690894 calls.
             Execution time mean : 35.099750 ns
    Execution time std-deviation : 0.422800 ns
   Execution time lower quantile : 34.454839 ns ( 2.5%)
   Execution time upper quantile : 35.953584 ns (97.5%)
                   Overhead used : 1.556642 ns
Evaluation count : 1630167600 in 60 samples of 27169460 calls.
             Execution time mean : 35.487409 ns
    Execution time std-deviation : 0.309872 ns
   Execution time lower quantile : 35.083030 ns ( 2.5%)
   Execution time upper quantile : 36.190015 ns (97.5%)
                   Overhead used : 1.556642 ns

Found 4 outliers in 60 samples (6.6667 %)
	low-severe	 3 (5.0000 %)
	low-mild	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
Evaluation count : 1440434700 in 60 samples of 24007245 calls.
             Execution time mean : 40.894457 ns
    Execution time std-deviation : 0.529510 ns
   Execution time lower quantile : 40.055991 ns ( 2.5%)
   Execution time upper quantile : 41.990985 ns (97.5%)
                   Overhead used : 1.556642 ns
nil
Comment by Michał Marczyk [ 13/May/14 1:28 AM ]

The new patch (...-substring.patch) returns hashCode for java.util.** classes other than List, Map, Map.Entry and Set, of course, so no behaviour change there.

Here are the benchmarks for repeated PHM lookups (slightly slower than 1.6.0 apparently, though within 1 ns) and the "add three hasheqs" benchmark (66 ns with patch vs. 57 ns without):

user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] (assert (= (hash phm) (hash juhm))) (c/bench (clojure.lang.Util/hasheq phm)) (c/bench (clojure.lang.Util/hasheq juhm)))
Evaluation count : 5183841240 in 60 samples of 86397354 calls.
             Execution time mean : 10.076893 ns
    Execution time std-deviation : 0.182592 ns
   Execution time lower quantile : 9.838456 ns ( 2.5%)
   Execution time upper quantile : 10.481086 ns (97.5%)
                   Overhead used : 1.565749 ns
Evaluation count : 3090420 in 60 samples of 51507 calls.
             Execution time mean : 19.596627 µs
    Execution time std-deviation : 224.380257 ns
   Execution time lower quantile : 19.288347 µs ( 2.5%)
   Execution time upper quantile : 20.085620 µs (97.5%)
                   Overhead used : 1.565749 ns
nil

user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] #_(assert (= (hash phm) (hash juhm))) (c/bench (unchecked-add (clojure.lang.Util/hasheq phm) (unchecked-add (clojure.lang.Util/hasheq "foo") (clojure.lang.Util/hasheq 123)))))
WARNING: Final GC required 1.418253438197936 % of runtime
Evaluation count : 879210900 in 60 samples of 14653515 calls.
             Execution time mean : 66.939309 ns
    Execution time std-deviation : 0.747984 ns
   Execution time lower quantile : 65.667310 ns ( 2.5%)
   Execution time upper quantile : 68.155046 ns (97.5%)
                   Overhead used : 1.724002 ns
nil

It is important to note that I have obtained the no-patch result for the "three hasheqs" benchmarks on a fresh JVM when benchmarking 1.6.0, so that's also how I repeated the benchmark with the patch applied. Hashing many different types changes the results noticeably – presumably HotSpot backs off from some optimizations after seeing several different types passed in to hasheq?

Comment by Michał Marczyk [ 13/May/14 8:04 AM ]

Here's a new patch (0005-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M.patch) that introduces a new isAlien static method that checks for instanceof Map/Map.Entry/Iterable and uses this method to test for "alien collection".

Initial benchmarking results are promising:

;;; "fall-through" benchmark
user=> (let [class-instance java.lang.String character-instance \a var-instance #'hash] (c/bench (clojure.lang.Util/hasheq class-instance)) (c/bench (clojure.lang.Util/hasheq character-instance)) (c/bench (clojure.lang.Util/hasheq var-instance)))
WARNING: Final GC required 1.258979068087473 % of runtime
Evaluation count : 1598432100 in 60 samples of 26640535 calls.
             Execution time mean : 36.358882 ns
    Execution time std-deviation : 0.566925 ns
   Execution time lower quantile : 35.718889 ns ( 2.5%)
   Execution time upper quantile : 37.414722 ns (97.5%)
                   Overhead used : 1.823120 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
Evaluation count : 1626362460 in 60 samples of 27106041 calls.
             Execution time mean : 35.426993 ns
    Execution time std-deviation : 0.294517 ns
   Execution time lower quantile : 35.047064 ns ( 2.5%)
   Execution time upper quantile : 36.058667 ns (97.5%)
                   Overhead used : 1.823120 ns

Found 1 outliers in 60 samples (1.6667 %)
	low-severe	 1 (1.6667 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
Evaluation count : 1461423180 in 60 samples of 24357053 calls.
             Execution time mean : 39.541873 ns
    Execution time std-deviation : 0.423707 ns
   Execution time lower quantile : 38.943560 ns ( 2.5%)
   Execution time upper quantile : 40.499433 ns (97.5%)
                   Overhead used : 1.823120 ns

Found 2 outliers in 60 samples (3.3333 %)
	low-severe	 2 (3.3333 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil

;;; "three hasheqs" benchmark
user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] #_(assert (= (hash phm) (hash juhm))) (c/bench (unchecked-add (clojure.lang.Util/hasheq phm) (unchecked-add (clojure.lang.Util/hasheq "foo") (clojure.lang.Util/hasheq 123)))))
WARNING: Final GC required 1.5536755331464491 % of runtime
Evaluation count : 820376460 in 60 samples of 13672941 calls.
             Execution time mean : 71.999365 ns
    Execution time std-deviation : 0.746588 ns
   Execution time lower quantile : 70.869739 ns ( 2.5%)
   Execution time upper quantile : 73.565908 ns (97.5%)
                   Overhead used : 1.738155 ns

Found 2 outliers in 60 samples (3.3333 %)
	low-severe	 2 (3.3333 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil
Comment by Michał Marczyk [ 13/May/14 8:28 AM ]

Ah, I left out the repeated phm hasheq lookup + hasheq of a java.util.HashMap instance pair of benchmarks from the above – here it is for completeness (no surprises though):

user=> (let [phm (apply hash-map (interleave (range 128) (range 128))) juhm (java.util.HashMap. phm)] (assert (= (hash phm) (hash juhm))) (c/bench (clojure.lang.Util/hasheq phm)) (c/bench (clojure.lang.Util/hasheq juhm)))
WARNING: Final GC required 1.260853406580491 % of runtime
Evaluation count : 5369135760 in 60 samples of 89485596 calls.
             Execution time mean : 10.380464 ns
    Execution time std-deviation : 3.407284 ns
   Execution time lower quantile : 9.510624 ns ( 2.5%)
   Execution time upper quantile : 11.461485 ns (97.5%)
                   Overhead used : 1.566301 ns

Found 5 outliers in 60 samples (8.3333 %)
	low-severe	 3 (5.0000 %)
	low-mild	 2 (3.3333 %)
 Variance from outliers : 96.4408 % Variance is severely inflated by outliers
Evaluation count : 3078180 in 60 samples of 51303 calls.
             Execution time mean : 19.717981 µs
    Execution time std-deviation : 209.896848 ns
   Execution time lower quantile : 19.401811 µs ( 2.5%)
   Execution time upper quantile : 20.180163 µs (97.5%)
                   Overhead used : 1.566301 ns

Found 2 outliers in 60 samples (3.3333 %)
	low-severe	 2 (3.3333 %)
 Variance from outliers : 1.6389 % Variance is slightly inflated by outliers
nil
Comment by Alex Miller [ 13/May/14 9:17 AM ]

Please don't submit any patches that change hashcode for anything other than making Java collections match Clojure collections - any other change is out of scope of this ticket.

In general, I would prefer just the execution time mean report for the moment rather than everything - the full criterium output makes these comments much harder to read and compare.

Comment by Alex Miller [ 13/May/14 9:33 AM ]

Could I get a summary of approaches, and a timing of 1.6.0 vs each patch for a consistent set of tests - say time of hash for Long, PHM, juHM, Class, and the "three hasheqs" test?

Comment by Rich Hickey [ 13/May/14 9:47 AM ]

"Hashing many different types changes the results noticeably – presumably HotSpot backs off from some optimizations after seeing several different types passed in to hasheq?"

Right - if your benchmarks do not treat this site as megamorphic you will get all sorts of distorted results.

Comment by Michał Marczyk [ 14/May/14 3:15 AM ]

Ok, I have what I think is an improved microbenchmark for this: xor of hasheqs for a long, a double, a string, a class, a character and a PHM (single instance, so it'll be a hash lookup). The results are not very encouraging.

Single form including the require to make it convenient to run; also bundled is a j.u.HashMap (128 entries) hasheq benchmark:

(do
  (require '[criterium.core :as c])
  (let [l    41235125123
        d    123.456
        s    "asdf;lkjh"
        k    BigInteger
        c    \S
        phm  (apply hash-map (interleave (range 128) (range 128)))
        juhm (java.util.HashMap. phm)
        f    (fn f []
               (-> (clojure.lang.Util/hasheq l)
                   (bit-xor (clojure.lang.Util/hasheq d))
                   (bit-xor (clojure.lang.Util/hasheq s))
                   (bit-xor (clojure.lang.Util/hasheq k))
                   (bit-xor (clojure.lang.Util/hasheq c))
                   (bit-xor (clojure.lang.Util/hasheq phm))))]
    (c/bench (f))
    (c/bench (hash juhm))))

Mean execution time as reported by Criterium:

version xor (ns) j.u.HM (µs)
unpatched 1.6.0 148.128748 1.701640
0005 patch 272.039667 21.201178
original patch 268.670316 21.169436
-alternative patch 271.747043 20.755397

The substring patch is broken (see below), so I skipped it. The patch I'm describing as the "original" one is attached as 0001-CLJ-1372-consistent-hasheq-for-java.util.-List-Map-M.patch.

Decisions common to all the patches:

1. One extra if statement in hasheq just above the default return with a three-way instanceof check.

2. The types tested for are j.u.Iterable, j.u.Map.Entry and j.u.Map.

3. Murmur3.hashOrdered takes Iterable, so that's why it's on the list. Map does not extend Iterable, so it's listed separately. Map.Entry is on the list, because ultimately the way to hash maps is to iterate over and hash their entries.

4. The actual hashing of the "alien" / host types is done by a separate static method – clojure.lang.Util.doalienhasheq – on the theory that this will permit hasheq to be inlined more aggressively and limit the worst of the perf hit to alien collections.

5. doalienhasheq checks for Map, Map.Entry, Set and List; entries are converted to lists for hashing, maps are hashed through entry sets and lists and sets are passed directly to Murmur3.

6. There is also a default case for other Iterable types – we must return hashCode or the result of composing some other function with hashCode for these, since we use equals to test them for equivalence.

The 0005 patch has hasheq call a separate private static method to perform the three-way type check, whereas the others put the check directly in the actual if test. The -alternative patch and the 0005 patch return hashCode in the default case, whereas the original patch composes Murmur3.hashInt with hashCode.

The substring patch only works for java.util.** classes and so doesn't solve the problem (it wouldn't correctly hash Guava collections, for example).

All of the patches change c.l.Util.hasheq and add one or two new static methods to clojure.lang.Util that act as helpers for hasheq. None of them changes anything else. Murmuring hashCode was a performance experiment that appeared to have a slight positive impact on some of the "fast cases" (in fact it's still the best performer among the current three patches in the microbenchmark presented above, although the margin of victory is of course extremely tiny). Thus I think all the current patches are in fact limited in scope to changes directly relevant to the ticket; the -alternative patch and the 0005 patch certainly are.

Comment by Michał Marczyk [ 14/May/14 3:29 AM ]

For completeness, branching on Map, Set etc. directly in hasheq, as with Jozef's original patch, results in the following timings in the microbenchmark introduced in my previous comment:

xor 315.866626 ns
juhm 18.520133 µs
Comment by Michał Marczyk [ 14/May/14 4:01 AM ]

New patch (0006) that leaves out the Map.Entry check; instead, two methods are introduced in the Murmur3 class to handle j.u.maps.

Java map entries aren't really integrated into Clojure – you can't use them like vectors, can't call seq on them etc. – so I don't think they need to match Clojure map entries in hasheq as long as j.u.maps do.

Timings:

xor 233.341689 ns
juhm 9.104637 µs
Comment by Michał Marczyk [ 14/May/14 4:17 AM ]

Checking for Map/Iterable in-line doesn't seem to affect xor benchmark results very much, but makes juhm hashing quicker. This is rather surprising to me. In any case, here's a new patch (0007) and the timings:

xor 233.062337 ns
juhm 8.629149 µs
Comment by Alex Miller [ 14/May/14 7:17 AM ]

What are equivalent timings without the patch?

Comment by Michał Marczyk [ 14/May/14 7:43 AM ]

They're listed in the table in the comment introducing the benchmark – 148.128748 ns for xor, 1.701640 µs for juhm.

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

What if we override hasheq for different types instead of using instanceof?

Comment by Michał Marczyk [ 14/May/14 12:50 PM ]

Overloaded methods are resolved statically, so there's no avoiding testing for type in the Object overload.

A more specific overload could be used to speed up hashing for its parameter type given a type hint or for literals, since the compiler would emit calls to that overload given appropriate compile-time information. There wouldn't be any speed-up in "implicit" hashing during hash map / set ops, however.

Comment by Daniel Compton [ 12/Nov/14 9:19 PM ]

This hit me when upgrading Factual/skuld from 1.5.1 to 1.6. clojure.data.fressian serialises c.l.PersistentHashSet sets into java.util.HashSet. This breaks equality checking in https://github.com/Factual/skuld/blob/b720feb142e6d274e85be208dc1d6d8634801719/test/skuld/net_test.clj#L8-L29 as we are comparing a set of maps where the original set contains a PersistentSet and the serialised and deserialised one contains a HashSet.

Comment by Daniel Compton [ 12/Nov/14 11:54 PM ]

This has come up again for me, details are in http://dev.clojure.org/jira/browse/DFRS-7

Comment by Max Penet [ 16/Mar/15 11:22 AM ]

This bit me again today as well (wasted a lot of time before figuring this one out). Any chance we'll have a patch for 1.7?

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

afaik, we are still in search of an approach with tolerable performance impacts before this can be considered.

Comment by Alex Miller [ 12/Jan/16 10:01 AM ]

While we are not opposed to addressing this ticket, we are not interested in addressing it at the expense of performance in comparisons that we care about more, and none of the current proposals meets that bar. For now, I am moving this to Backlog but would pull it out if a solution presents itself.





[CLJ-1167] repl value of *file* is "NO_SOURCE_PATH", of *source-path* is "NO_SOURCE_FILE" Created: 19/Feb/13  Updated: 12/Jan/16

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


 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-1263] Allow static compilation of function invocations Created: 14/Sep/13  Updated: 12/Jan/16  Resolved: 11/Jan/16

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

Type: Enhancement Priority: Major
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Completed Votes: 4
Labels: compiler


 Description   

This proposal is to allow metadata on functions to prevent a fully dynamic var deref to be used whenever the function is called.

When the function is invoked, JVM "invokevirtual" instruction will be used, which is faster than the current implementation (var deref + IFn cast + invokinterface) and has less restrictions (no need to predefine interfaces to match the function parameters). The JVM is generally able to compile such invokevirtual instructions into extremely efficient code - effectively as fast as pure Java.

This is intended to pave the way to better support for statically compiled, high performa