<< Back to previous view

[CLJ-1510] line-seq and read-line don't return nil on Ctrl-D in lein repl Created: 21/Aug/14  Updated: 21/Aug/14

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

Type: Defect Priority: Major
Reporter: Geoff Little Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reader
Environment:

OSX, lein repl



 Description   

Executing in lein repl either

(doseq [line (line-seq (java.io.BufferedReader. *in*)) :while line]
(println line))

Or

(doseq [line (read-line) :while line]
(println line))

One would expect these to return on a user's enter of Ctrl-D, however they never return.



 Comments   
Comment by Andy Fingerhut [ 21/Aug/14 11:51 PM ]

If instead of 'lein repl' you run:

java -cp clojure.jar clojure.main

at a shell prompt, you get a REPL where the first doseq expression you give does return when you enter Ctrl-D.

The second doseq expression returns a string from (read-line), and then the doseq iterates over that as a sequence of characters, and it returns without needing Ctrl-D after reading a single line in both 'lein repl' and the plain Clojure REPL with the command given above.

I suggest that the behavior with your first doseq expression is an issue to be taken up with the Leiningen developers. Several ways of contacting them are given at http://leiningen.org/#community I would recommend IRC or the email list first, before creating a Github issue.





[CLJ-823] Piping seque into seque can deadlock Created: 03/Aug/11  Updated: 21/Aug/14

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

Type: Defect Priority: Minor
Reporter: Greg Chapman Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None
Environment:

Windows 7; JVM 1.6; Clojure 1.3 beta 1


Attachments: Text File clj-823-v1.patch    
Patch: Code and Test
Approval: Vetted

 Description   

I'm not sure if this is a supported scenario, but the following deadlocks in Clojure 1.3:

(let [xs (seque (range 150000))
ys (seque (filter odd? xs))]
(apply + ys))

As I understand it, the problem is that ys' fill takes place on an agent thread, so when it calls xs' drain, the (send-off agt fill) does not immediately trigger xs' fill, but is instead put on the nested list to be performed when ys' agent returns. Unfortunately, ys' fill will eventually block trying to take from xs, and so it never returns and the pending send-offs are never sent. Wrapping the send-off in drain to:

(future (send-off agt fill))

is a simple (probably not optimal) way to fix the deadlock.



 Comments   
Comment by Peter Monks [ 07/Jan/13 3:43 PM ]

Reproduced on 1.4.0 and 1.5.0-RC1 as well, albeit with this example:

(seque 3 (seque 3 (range 10)))

Comment by Stuart Halloway [ 30/Mar/13 9:16 AM ]

release-pending-sends?

Comment by Andy Fingerhut [ 21/Aug/14 6:13 PM ]

clj-823-v1.patch uses (release-pending-sends) in seque's drain function in an attempt to avoid the deadlock when a seque is fed into another seque, as suggested by Stuart Halloway. It adds Peter Monks's small quick test case demonstrating the deadlock, which fails (i.e. hangs until killed) without the change and passes with it.





[CLJ-1191] Improve apropos to show some indication of namespace of symbols found Created: 04/Apr/13  Updated: 20/Aug/14

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

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: repl

Attachments: Text File clj-1191-patch-v1.txt     Text File clj-1191-patch-v2.txt     Text File clj-1191-v3.patch    
Patch: Code and Test
Approval: Screened

 Description   

apropos does find all symbols in all namespaces that match the argument, but the return value gives no clue as to which namespace the found symbols are in. It can even return multiple occurrences of the same symbol, which only gives a clue that the symbol exists in more than one namespace, but not which ones. For example:

user=> (apropos "replace")
(postwalk-replace prewalk-replace replace re-quote-replacement replace replace-first)

user=> (apropos 'macro)
(macroexpand-all macroexpand macroexpand-1 defmacro)

It would be nice if the returned symbols could indicate the namespace.

With the screened patch clj-1191-v3.patch applied the output for the examples above becomes:

user=> (apropos "replace")
(clojure.core/replace clojure.string/re-quote-replacement clojure.string/replace clojure.string/replace-first clojure.walk/postwalk-replace clojure.walk/prewalk-replace)

user=> (apropos 'macro)
(clojure.core/defmacro clojure.core/macroexpand clojure.core/macroexpand-1 clojure.walk/macroexpand-all)

Patch: clj-1191-v3.patch

Screened by: Alex Miller



 Comments   
Comment by Andy Fingerhut [ 04/Apr/13 8:25 PM ]

Path clj-1191-patch-v1.txt enhances apropos to put a namespace/ qualifier before every symbol found that is not in the current namespace ns. It also finds the shortest namespace alias if there is more than one. Examples of output with patch:

user=> (apropos "replace")
(replace clojure.string/re-quote-replacement clojure.string/replace clojure.string/replace-first clojure.walk/postwalk-replace clojure.walk/prewalk-replace)

user=> (require '[clojure.string :as str])
nil
user=> (apropos "replace")
(replace clojure.walk/postwalk-replace clojure.walk/prewalk-replace str/re-quote-replacement str/replace str/replace-first)

user=> (in-ns 'clojure.string)
#<Namespace clojure.string>
clojure.string=> (clojure.repl/apropos "replace")
(re-quote-replacement replace replace-by replace-first replace-first-by replace-first-char replace-first-str clojure.core/replace clojure.walk/postwalk-replace clojure.walk/prewalk-replace)

Comment by Colin Jones [ 05/Apr/13 1:34 PM ]

+1

apropos as it already stands is quite helpful for already-referred vars, but not for vars that are only in other nses.

This update includes the information someone would need to further investigate the output.

Comment by Alex Miller [ 20/Aug/14 11:22 AM ]

If you have "use"d many namespaces (which is not uncommon at the repl), this updated apropos still doesn't help you understand where a particular function is coming from (as the ns will be omitted). It's cool that this patch is "unresolving" and finding the shortest-alias etc but I think it's actually doing too much. In my opinion, simply providing the full namespace for all matches would actually be more useful (and easier).

Comment by Andy Fingerhut [ 20/Aug/14 12:27 PM ]

Patch clj-1191-patch-v2.txt dated Aug 20 2014 modifies apropos so that every symbol returned has a full namespace qualifier, even if it is in clojure.core. Before this patch:

user=> (apropos "replace")
(prewalk-replace postwalk-replace replace replace-first re-quote-replacement replace)

user=> (apropos 'macro)
(macroexpand-all macroexpand macroexpand-1 defmacro)

After this patch:

user=> (apropos "replace")
(clojure.core/replace clojure.string/re-quote-replacement clojure.string/replace clojure.string/replace-first clojure.walk/postwalk-replace clojure.walk/prewalk-replace)

user=> (apropos 'macro)
(clojure.core/defmacro clojure.core/macroexpand clojure.core/macroexpand-1 clojure.walk/macroexpand-all)
Comment by Alex Miller [ 20/Aug/14 1:34 PM ]

Some comments on the code itself:

1) I don't think we should do anything special for ns - there are plenty of ways to search your current ns. I think it unnecessarily adds a lot of complexity without enough value.
2) Rather than finding vars and work back to syms, I think this should instead retain the ns context as it walks the ns-publics keys so that you can easily reassemble a fully-qualified symbol name.
3) Why do you need the set at the end? Seems like symbols should already be unique at this point?

Comment by Andy Fingerhut [ 20/Aug/14 5:02 PM ]

Patch clj-1191-patch-v3.txt attempts to address Alex Miller's comments on the v2 patch.

Perhaps the diff will get down to a 1-line change

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

Patch clj-1191-v3.patch is identical to clj-1191-patch-v3.txt mentioned in the previous comment, but conforms to the requested .patch or .diff file name ending.





[CLJ-1284] Clojure functions and reified objects should expose a public static field to identify their proper Clojure name Created: 24/Oct/13  Updated: 20/Aug/14

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

Type: Enhancement Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs

Attachments: Text File CLJ-1284-store-demunged-names.patch    

 Description   

There are several examples of frameworks that attempt to de-mangle a Java class name into a Clojure symbol (including namespace); this is useful for writing out an improved, Clojure-specific stack trace when reporting exceptions.

Existing libraries are based on regular expression matching and guesswork, and can occasionally give incorrect results, such as when a namespace or function name actually contains an underscore.

It would be helpful for authors of such frameworks if Clojure would expose a static final field on such classes with the proper name that should appear in the stack trace; libraries would then be able to use reflection to access the proper name of the field, without the current guesswork.

I would suggest CLOJURE_SOURCE_NAME as a reasonable name for such a field.

Other Clojure class constructs beyond functions, such as reified types and protocol implementations, would also benefit, though it is less obvious what exact string value would properly and unambiguously identify what purpose the class plays.



 Comments   
Comment by Alex Miller [ 24/Oct/13 8:31 PM ]

FYI, there is a patch on the way in for 1.6 that contains a new demunge function in Compiler. However, the munged name is not always reversible so having the original around is a good idea.

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

The patch Alex is referring to is attached to CLJ-1083.

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

Howard, there seems to be some overlap in the intent between this ticket and CLJ-1278. I guess either of them could be done without the other, but wanted to check.

Comment by Daniel Solano Gómez [ 20/Aug/14 2:17 PM ]

Here's an initial stab at adding this feature.

Some notes:

  • This will tag emitted classes from deftype and fn
  • This will handle fn}}s that are enclosed, but the output will be slightly different from the standard {{demunge function: only the initial $ is transformed to a /.
  • Unfortunately, because the defn for type/record constructor occurs in a let form, the generated symbol doesn't match what it should be.
  • There is no exposed API to get the demunged symbol from the class. Perhaps demunge should check if the given name corresponds to a class with this field?

I welcome any input on how this should really work. In particular, any ideas on how to best deal with {{defn}}s that are not top-level forms.





[CLJ-1509] Some clojure namespaces not AOT-compiled and included in the clojure jar Created: 20/Aug/14  Updated: 20/Aug/14

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
Approval: Triaged

 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-1378] Hints don't work with #() form of function Created: 11/Mar/14  Updated: 20/Aug/14

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

Type: Enhancement Priority: Major
Reporter: Roy Varghese Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: interop, typehints

Attachments: File clj-1378.diff     File clj-1378-v2.diff    
Patch: Code and Test
Approval: Screened

 Description   

Example showing how a local fn can be hinted but an anonymous function cannot:

;; OK
user> (let [ex (java.util.concurrent.Executors/newFixedThreadPool 1)
            f (fn [])]
        (.submit ex ^Runnable f))
nil
;; ERROR - this should work the same as the previous
user> (let [ex (java.util.concurrent.Executors/newFixedThreadPool 1)]
        (.submit ex #()))
CompilerException java.lang.IllegalArgumentException: More than one matching method found: submit, compiling:(/private/var/folders/7r/_1fj0f517rgcxwx79mn79mfc0000gn/T/form-init7901279404687292754.clj:3:9)

Cause: Functions have metadata, but Compiler does not look in them for type hints. Var expressions and local bindings use :tag metadata to override return of getJavaClass(). Compiler parses #() into a FnExpr, which always return AFunction as its class.

Proposed: Change FnExpr.getJavaClass() to return tag as type if it is available.

Patch: clj-1378-v2.diff

Screened by: Alex Miller



 Comments   
Comment by Jozef Wagner [ 12/Mar/14 4:03 AM ]

Functions do have metadata, but Compiler does not look in them for type hints.

user=> (with-meta #() {:foo :bar})
#<clojure.lang.AFunction$1@779325ee>

When compiler is determining which native method to use, it matches method signature with classes of given args. There is a getJavaClass() method in Compiler.java which returns a class for given expression. Vars expressions and local bindings use :tag metadata to override this class, but most other expressions don't. Compiler parses #() into a FnExpr, which always return AFunction as its class.

Most of time this approach is OK, as AFunction implements Runnable and Callable so there is no need for type hint. However, in this particular case, there are overrides for both Runnable and Callable, and as AFunction can be either of them, the expression is ambiguous.

Comment by Jozef Wagner [ 12/Mar/14 4:17 AM ]

Patch added, following expression will now run without error

(.submit (java.util.concurrent.Executors/newCachedThreadPool) ^Runnable #())
Comment by Alex Miller [ 12/Mar/14 9:34 AM ]

Could you add a test to the patch?

Comment by Jozef Wagner [ 12/Mar/14 2:53 PM ]

Attached patch clj-1378-v2.diff which contains both fix and test.





[CLJ-1498] Remove birth-thread check from transients Created: 08/Aug/14  Updated: 20/Aug/14

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

Type: Enhancement Priority: Major
Reporter: Rich Hickey Assignee: Alex Miller
Resolution: Unresolved Votes: 0
Labels: collections, transient

Attachments: File clj-1498-2.diff     File clj-1498-3.diff     File clj-1498.diff    
Patch: Code
Approval: Screened

 Description   

Transients protect themselves from use by any thread other than the one that creates them. This is good for safety, however it eliminates certain valid usages of transients. For example, usage in a go-block might occur in subsequent invocations across multiple OS threads (but only one logical thread of control).

Current simple test:

user> (def v (transient []))
#'user/v
user> (persistent! @(future (conj! v 1)))
IllegalAccessError Transient used by non-owner thread  clojure.lang.PersistentVector$TransientVector.ensureEditable (PersistentVector.java:464)

Proposal: Remove the owner check from transient collections. (Leave the edit after persistent check as is.) The test above should succeed.

After:

user=> (def v (transient []))
#'user/v
user=> (persistent! @(future (conj! v 1)))
[1]

The clj-1498-3.diff version of the patch also replaces the AtomicReference<Thread> with AtomicBoolean as we can now track just ownership, not who owns it.

Doc update: Various pieces of documentation will need to be updated with this change, namely http://clojure.org/transients

Patch: clj-1498-3.diff

Alternative: Another idea would be to make this check optional with some kind of option on the transient call (transient coll :check-owner true). Not sure whether what the default would be for that.



 Comments   
Comment by Jozef Wagner [ 09/Aug/14 7:08 AM ]

I suggest to add a functionality to pass ownership of a transient to the different thread, or to release the ownership by passing nil.

user=> (def v (pass! (transient []) nil))
#'user/v
user=> (persistent! @(future (conj! v 1)))
[1]

pass! has to be called by current owner thread, or by any thread if the transient is currently released.

Comment by Alex Miller [ 13/Aug/14 1:42 PM ]

New patch that replaces AtomicReference<Thread> with AtomicBoolean.

Comment by Stuart Halloway [ 19/Aug/14 11:05 AM ]

Alex, can you please expand the example test you provided to a generative test that covers the following combinations:

  1. different collection sizes (above and below the ArrayMap size boundary)
  2. different shapes (vector vs. map)
  3. successful use across threads (positive use case this ticket enables)

data_structures.clj has helpers for generating transient interactions that you can build on.

Comment by Alex Miller [ 20/Aug/14 8:59 AM ]

Enhanced existing generative tests to test random actions against sets, vectors, and both PHM and PAM. Added additional actions to do transient modification actions in other threads as well as originating thread.





[CLJ-1508] Supplied-p parameter in clojure Created: 18/Aug/14  Updated: 18/Aug/14

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: destructuring
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 supplied_p.diff    
Patch: Code and Test

 Description   

As see in https://groups.google.com/forum/?hl=en#!topic/clojure/jWc51JOkvsA

I think we can add a ? option for destructure ,then we can write a test like :

(deftest supplied-p-in-destructuring
  (let [{:keys [a b c d] :p? {a a-p? b b-p? c c-p? d d-p?} :or {a 1}} {:b 2 :c 3 }]
    (is (= a 1))
    (is (false? a-p?))
    (is (= 2 b))
    (is (true? b-p?))
    (is (= 3 c))
    (is (true? c-p?))
    (is (nil? d))
    (is (false? d-p?))))

Even if the a var has a default value 1 by :or option,but the a-p? is still false.
Just like the supplied-p-parameter in Commons LISP.

The patch is attached with code and test.



 Comments   
Comment by Steve Miner [ 18/Aug/14 8:24 AM ]

As mentioned on the mailing list, you could use {:as arg} destructuring to get same information. Here's a slightly modified example that works in the current Clojure:

(deftest supplied-p-in-destructuring
  ;; (let [{:keys [a b c d] :p? {a a-p? b b-p? c c-p? d d-p?} :or {a 1}} {:b 2 :c 3 }]
  (let [{:keys [a b c d] :or {a 1} :as argmap} {:b 2 :c 3 }
        supplied? (partial contains? argmap)
        a-p? (supplied? :a)
        b-p? (supplied? :b)
        c-p? (supplied? :c)
        d-p? (supplied? :d)]
    (is (= a 1))
    (is (false? a-p?))
    (is (= 2 b))
    (is (true? b-p?))
    (is (= 3 c))
    (is (true? c-p?))
    (is (nil? d))
    (is (false? d-p?))))




[CLJ-1507] Throw NPE in eval reader Created: 16/Aug/14  Updated: 16/Aug/14

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

Type: Defect 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-1506] A little improvement when reading syntax quote form Created: 16/Aug/14  Updated: 16/Aug/14

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: syntax-quote
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 fast_syntax_quote_reader.diff    

 Description   

When reading syntax quote on keyword,string or number etc,it returns the form as result directly. Read it in:
https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/LispReader.java#L844-847

else if(form instanceof Keyword
       || form instanceof Number
       || form instanceof Character
       || form instanceof String)
   ret = form;

But missing check if it is a nil,regular pattern or boolean constants.
After patched:

else if(form == null
       || form instanceof Keyword
       || form instanceof Number
       || form instanceof Character
       || form instanceof Pattern
       || form instanceof Boolean
       || form instanceof String)
    ret = form;

It's a little patch, i am not sure if it is worth a try.






[CLJ-1502] Clojure Inspector navigation error Created: 12/Aug/14  Updated: 15/Aug/14

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

Type: Defect Priority: Minor
Reporter: Dan Campbell Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: bug, inspector, navigation
Environment:

Windows 7 and 8, Java 7, Clojure repl


Attachments: Text File clj-1502-v1.patch    
Patch: Code

 Description   

With Clojure 1.6.0 on some platforms (details below), if you create an object such as

(def nst (vec '((3 7 22) 99 (123 18 225 437))))

and then you inspect the tree representing the object

(inspect-tree nst)

Most of the navigation with the keyboard proceeds fine. However, when you point to an individual value - e.g. the 99 or the 437 - and press the right arrow key, there is an error

Exception in thread "AWT-EventQueue-0" java.lang.UnsupportedOperationException: count not supported on this type: Long
	at clojure.lang.RT.countFrom(RT.java:556)
	at clojure.lang.RT.count(RT.java:530)
	at clojure.inspector$fn__6907.invoke(inspector.clj:40)
	at clojure.lang.MultiFn.invoke(MultiFn.java:227)
	at clojure.inspector$tree_model$fn__6929.invoke(inspector.clj:63)
	at clojure.inspector.proxy$java.lang.Object$TreeModel$775afa87.getChildCount(Unknown Source)
	at javax.swing.plaf.basic.BasicTreeUI$Actions.traverse(BasicTreeUI.java:4395)
	at javax.swing.plaf.basic.BasicTreeUI$Actions.actionPerformed(BasicTreeUI.java:4052)
	at javax.swing.SwingUtilities.notifyAction(SwingUtilities.java:1662)
	at javax.swing.JComponent.processKeyBinding(JComponent.java:2878)
	at javax.swing.JComponent.processKeyBindings(JComponent.java:2925)
	at javax.swing.JComponent.processKeyEvent(JComponent.java:2841)
	at java.awt.Component.processEvent(Component.java:6282)
	at java.awt.Container.processEvent(Container.java:2229)
	at java.awt.Component.dispatchEventImpl(Component.java:4861)
	at java.awt.Container.dispatchEventImpl(Container.java:2287)
	at java.awt.Component.dispatchEvent(Component.java:4687)
	at java.awt.KeyboardFocusManager.redispatchEvent(KeyboardFocusManager.java:1895)
	at java.awt.DefaultKeyboardFocusManager.dispatchKeyEvent(DefaultKeyboardFocusManager.java:762)
	at java.awt.DefaultKeyboardFocusManager.preDispatchKeyEvent(DefaultKeyboardFocusManager.java:1027)
	at java.awt.DefaultKeyboardFocusManager.typeAheadAssertions(DefaultKeyboardFocusManager.java:899)
	at java.awt.DefaultKeyboardFocusManager.dispatchEvent(DefaultKeyboardFocusManager.java:727)
	at java.awt.Component.dispatchEventImpl(Component.java:4731)
	at java.awt.Container.dispatchEventImpl(Container.java:2287)
	at java.awt.Window.dispatchEventImpl(Window.java:2719)
	at java.awt.Component.dispatchEvent(Component.java:4687)
	at java.awt.EventQueue.dispatchEventImpl(EventQueue.java:735)
	at java.awt.EventQueue.access$200(EventQueue.java:103)
	at java.awt.EventQueue$3.run(EventQueue.java:694)
	at java.awt.EventQueue$3.run(EventQueue.java:692)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:76)
	at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:87)
	at java.awt.EventQueue$4.run(EventQueue.java:708)
	at java.awt.EventQueue$4.run(EventQueue.java:706)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.security.ProtectionDomain$1.doIntersectionPrivilege(ProtectionDomain.java:76)
	at java.awt.EventQueue.dispatchEvent(EventQueue.java:705)
	at java.awt.EventDispatchThread.pumpOneEventForFilters(EventDispatchThread.java:242)
	at java.awt.EventDispatchThread.pumpEventsForFilter(EventDispatchThread.java:161)
	at java.awt.EventDispatchThread.pumpEventsForHierarchy(EventDispatchThread.java:150)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:146)
	at java.awt.EventDispatchThread.pumpEvents(EventDispatchThread.java:138)
	at java.awt.EventDispatchThread.run(EventDispatchThread.java:91)

Environments where this has been reproduced:
+ Windows 7 Enterprise, SP1, Oracle JDK 1.7.0_51, Clojure 1.6.0
+ Ubuntu Linux 14.04.1, Oracle JDK 1.7.0_65, Clojure 1.6.0

Environments where the same sequence of events does not cause an exception:
+ Mac OS X 10.8.5, Oracle JDK 1.7.0_51, Clojure 1.6.0



 Comments   
Comment by Andy Fingerhut [ 13/Aug/14 6:08 PM ]

Patch clj-1502-v1.patch avoids the exception in the situation reported. Tested manually on OS X, Linux, and Windows 7 versions mentioned in the patch comment. I suspect it is not worth the effort to write an automated test for this.

Comment by Dan Campbell [ 15/Aug/14 6:40 PM ]

Thanks, Andy

  • DC




[CLJ-1504] Add :inline to most core predicates Created: 15/Aug/14  Updated: 15/Aug/14

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

Type: Enhancement Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-add-inline-to-some-core-predicates.patch    
Patch: Code

 Description   

This will allow instance? predicates calls to be emitted using the instanceof JVM bytecode and will also allow tools like core.typed or tools.analyzer.jvm to infer the type of a var/local on a per branch basis without having to special-case all the core predicates.



 Comments   
Comment by Jozef Wagner [ 15/Aug/14 1:32 PM ]

Related ticket CLJ-1227 and related quote from Alex:

definline is considered to be an experimental feature and Rich would like to discourage its use as the hope is to remove it in the future. The desired replacement is something like common lisp compiler macros that could allow the compiler to detect special situations and optimize the result but leave behind a function invocation for the case where no special behavior is available.

Comment by Nicola Mometto [ 15/Aug/14 1:42 PM ]

This patch uses "manual" :inline metadata on functions, it's used by many other core functions (like +,- et), not definline so Rich's comment doesn't apply.





[CLJ-1503] allow for `{~@foo} and `#{~(gensym) ~(gensym)} Created: 14/Aug/14  Updated: 15/Aug/14

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

Type: Enhancement Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-allow-for-foo-and-gensym-gensym.patch    
Patch: Code

 Description   

Currently both `{@foo} and `#{(gensym) ~(gensym)} throw an exception at read time even though they could actually return valid run-time code.
This patch introduces the SyntaxQuotedMap and SyntaxQuotedSet classes that are used internally in the reader to represent syntax quoted maps and sets, that may skip the duplicate key and length checks.

The SyntaxQuotedMap extends PersistentArrayMap as a workaround for the lack of defrecords in java, since a SyntaxQuotedMap needs to be an IPersistentMap in case it's used as metadata.






[CLJ-1499] Replace seq-based iterators with direct iterators for all non-seq collections that use SeqIterator Created: 08/Aug/14  Updated: 13/Aug/14

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

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

Approval: Vetted

 Description   

What the title says



 Comments   
Comment by Alex Miller [ 13/Aug/14 1:57 PM ]

The list of non-seqs that uses SeqIterator are:

  • records (in core_deftype.clj)
  • APersistentSet - fallback, maybe is ok?
  • PersistentHashMap
  • PersistentQueue
  • PersistentStructMap

Seqs (that do not need to be changed) are:

  • ASeq
  • LazySeq.java

LazyTransformer$MultiStepper - not sure





[CLJ-1501] LazySeq switches to equiv when using equals Created: 11/Aug/14  Updated: 11/Aug/14

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

Type: Defect Priority: Major
Reporter: Jozef Wagner Assignee: Jozef Wagner
Resolution: Unresolved Votes: 0
Labels: collections, interop, seq

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

 Description   

When comparing lazy seqs with java equality operator .equals, the implementation switches to the Clojures .equiv comparison. This switch is not present in any other Seq or ordered collection type.

user> (.equals '(3) '(3N))
false
user> (.equals [3] [3N])
false
user> (.equals (seq [3]) (seq [3N]))
false
user> (.equals (lazy-seq [3]) (lazy-seq [3N]))
true


 Comments   
Comment by Jozef Wagner [ 11/Aug/14 9:32 AM ]

Patch clj-1501.diff with tests added





[CLJ-1493] Fast keyword intern Created: 06/Aug/14  Updated: 11/Aug/14

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: keywords, performance
Environment:

Mac OS X 10.9.4 / 2.6 GHz Intel Core i5 / 8 GB 1600 MHz DDR3
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 fast_keyword_intern.diff    
Patch: Code
Approval: Triaged

 Description   

Keyword's intern(Symbol) method uses recursive invocation to get a valid keyword instance.I think it can be rewrite into a 'for loop'
to reduce method invocation cost.
So i developed this patch, and make some simple benchmark.Run the following command line three times after 'ant jar':

java -Xms64m -Xmx64m -cp test:clojure.jar clojure.main -e "(time (dotimes [n 10000000] (keyword (str n))))"

Before patched:

"Elapsed time: 27343.827 msecs"
"Elapsed time: 26172.653 msecs"
"Elapsed time: 25673.764 msecs"

After patched:

"Elapsed time: 24884.142 msecs"
"Elapsed time: 23933.423 msecs"
"Elapsed time: 25382.783 msecs"

It looks the patch make keyword's intern a little more fast.

The patch is attached and test.

Thanks.

P.S. I've signed the contributor agreement, and my email is killme2008@gmail.com .



 Comments   
Comment by Alex Miller [ 07/Aug/14 9:01 AM ]

Looks intriguing (and would be a nice change imo). I ran this on a json parsing benchmark I used for the keyword changes and saw ~3% improvement.

Comment by dennis zhuang [ 07/Aug/14 9:54 PM ]

Updated the patch, remove the 'k == null' clause in for loop,it's not necessary.

Comment by Andy Fingerhut [ 11/Aug/14 1:29 AM ]

Dennis, while JIRA can handle multiple patches with the same name, it can be confusing for people discussing the patches, and for some scripts I have to evaluate them. Please consider giving the patches different names (e.g. with version numbers in them), or removing older ones if they are obsolete.

Comment by dennis zhuang [ 11/Aug/14 9:19 AM ]

Hi,andy

Thank you for reminding me.I deleted the old patch.





[CLJ-1002] chunk-* functions not documented Created: 27/May/12  Updated: 11/Aug/14

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

Type: Enhancement Priority: Minor
Reporter: Jim Blomo Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Attachments: Text File document_chunk_fns.patch     Text File document_chunk_fns_v2.patch    
Patch: Code

 Description   

None of the chunk related functions defined in core.clj have documentation. While their implementations are straightforward, it means the functions do not show up in http://clojure.org/api. Are these not considered part of the API? If so, should they be private? Otherwise, I think they should have public documentation.

For searchability, the function are:

chunk-append, chunk, chunk-first, chunk-rest, chunk-next, chunk-cons, chunked-seq?



 Comments   
Comment by Ben Moss [ 01/Jun/14 3:38 PM ]

Did my best to explain these functions as I understand them.

Comment by Andy Fingerhut [ 01/Jul/14 10:02 AM ]

Ben, the correct order for adding doc strings to a defn is like this:

(defn ^:static ^clojure.lang.ChunkBuffer chunk-buffer
  "Returns a fixed length buffer of the given capacity."
  ^clojure.lang.ChunkBuffer [capacity]
  (clojure.lang.ChunkBuffer. capacity))

with the doc string after the symbol that names the function, but before the argument vector.

The Clojure compiler gives no errors or warnings if you do it in the order, that is true. However, it also does not attach the provided string as documentation metadata, and thus (doc fn-name) will not print the doc string.

Could you update the patch to put the proposed doc strings in the correct place?

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

FYI, there are several functions in clojure.core that are not private (because they are useful to implementors outside core) but also not documented so they don't show up in public api (because they are not considered part of the api). It's possible that these functions fall in that category and are intentionally undocumented, however I am unsure.

Comment by Ben Moss [ 06/Aug/14 2:34 PM ]

Sorry for the rookie mistake, Andy. As for whether these are intended to be documented at all, I don't really know.

Comment by Andy Fingerhut [ 06/Aug/14 4:10 PM ]

Ben, that is a rookie and experienced-Clojure-developer mistake, as I have learned from running Eastwood on many Clojure projects written by experienced Clojure developers. There were (and still are I think) some occurrences of it in Clojure itself.

Comment by Andy Fingerhut [ 11/Aug/14 7:39 AM ]

Ben, your patch document_chunk_fns_v2.patch dated Aug 6 2014 applies cleanly to latest master, but causes Clojure's tests to fail. This is because there is a test that ensures that every function in clojure.core with a doc string also has :added metadata. This can be corrected if you create :added metadata for the functions where you add doc strings. Search for :added in core.clj for examples. See "How to run all Clojure tests" on this wiki page: http://dev.clojure.org/display/community/Developing+Patches





[CLJ-1496] Added a new arity to 'ex-info' that only accepts a message. Created: 08/Aug/14  Updated: 11/Aug/14

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: ex-info, exceptions
Environment:

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)

Mac OSX 10.9.4


Attachments: File ex_info_arity.diff    
Patch: Code

 Description   

We often use 'ex-info' to throw a custom exception.But ex-info at least accepts two arguments: a string message and a data map.
In most cases,but we don't need to throw a exception that taken a data map.
So i think we can add a new arity to ex-info:

(ex-info "the exception message")

That created a ExceptionInfo instance carries empty data.

I am not sure it's useful for other people,but it's really useful for our developers.

The patch is attached.






[CLJ-1490] Exception on protocol implementation after protocol reloaded could be improved Created: 04/Aug/14  Updated: 11/Aug/14

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

Type: Enhancement Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: errormsgs, protocols

Attachments: Text File CLJ-1490.1.patch    
Patch: Code and Test

 Description   

In a situation where you define a protocol, and then define a class that extends that protocol (e.g., reify, defrecord, deftype) and then later, re-define the protocol (typically, by reloading the namespace that defines the protocol), then the existing instances are no longer valid.

However, the exception that gets generated can be confusing:

     java.lang.IllegalArgumentException: No implementation of method: :injections of protocol: #'fan.microservice/MicroService found for class: fan.auth.AuthService
                                           clojure.core/-cache-protocol-fn                  core_deftype.clj:  544
                                           fan.microservice/eval23300/fn/G                  microservice.clj:   12
                                                       clojure.core/map/fn                          core.clj: 2559
                                                 clojure.lang.LazySeq.sval                      LazySeq.java:   40
                                                  clojure.lang.LazySeq.seq                      LazySeq.java:   49
                                                    clojure.lang.Cons.next                         Cons.java:   39
                                             clojure.lang.RT.boundedLength                           RT.java: 1654
                                               clojure.lang.RestFn.applyTo                       RestFn.java:  130
                                                        clojure.core/apply                          core.clj:  626
                 fan.microservice.StandardContainer/construct-ring-handler                  microservice.clj:   51

The confusing part is that (in the above example) AuthService does extend MicroService, just not the correct version of it.

The exception message should be extended to identify that this is "possibly because the protocol was reloaded since the class was defined."

A patch will be ready shortly.



 Comments   
Comment by Howard Lewis Ship [ 04/Aug/14 12:15 PM ]

Patch with tests





[CLJ-1488] Implement Named over Vars Created: 01/Aug/14  Updated: 11/Aug/14

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

Type: Enhancement Priority: Trivial
Reporter: Reid McKenzie Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-Implement-clojure.lang.Named-over-Vars.patch    
Patch: Code
Approval: Triaged

 Description   

Vars, while a general reference structure, are used to implement bindings and have special reader and printer notation reflecting this reality. Unlike Keywords and Symbols which share the "namespace/name" notation of Vars, Vars do not implement the clojure.lang.Named interface while they print as if they were Named.

The attached patch implements Named over Vars.

Example:

user=> (name :clojure.core/conj)
"conj"
user=> (namespace :clojure.core/conj)
"clojure.core"
user=> (name 'clojure.core/conj)
"conj"
user=> (namespace 'clojure.core/conj)
"clojure.core"
user=> (name #'clojure.core/conj)
"conj"
user=> (namespace #'clojure.core/conj)
"clojure.core"
user=> (with-local-vars [x 1] (name x))
"--unnamed--"
user=> (with-local-vars [x 1] (namespace x))
nil
user=> (with-local-vars [x 1] (println x))
#<Var: --unnamed-->

This is useful for applications such as the CinC project where Vars are often taken directly as values in which context they would ideally be interchangeable with the Symbols the bound values of which they represent.



 Comments   
Comment by Nicola Mometto [ 02/Aug/14 11:42 AM ]

With this patch calling `name` on a unnamed Var will cause a NPE, I don't think this is desiderable.

Comment by Reid McKenzie [ 02/Aug/14 1:39 PM ]

I agree, however this behavior seems to be standard in Core.

Clojure 1.6.0
user=> (name nil)
NullPointerException clojure.core/name (core.clj:1518)
user=> (namespace nil)
NullPointerException clojure.core/namespace (core.clj:1526)

I'm also not convinced that the "name" or "namespace" of an unbound var is meaningful, in which case a NPE is probably acceptable.

Comment by Nicola Mometto [ 02/Aug/14 1:45 PM ]

I was not talking about unbound Vars, but about anonymous Vars, I'm assuming you miswrote.

I'd agree with you that throwing an exception could be a reasonable behaviour, except I can test for nil before calling name on it while there's no way to test whether a var is named or not, except trying to access directly the .name field which is excatly what this ticket is for.

Comment by Nicola Mometto [ 02/Aug/14 2:27 PM ]

Me and Reid have been talking about this issue over IRC, here's what's come up:

  • Vars can be either unnamed (as are Vars returned by with-local-vars) or contain both a namespace and a name part( that's the case for interned Vars)
  • there's currently no way to test for the "internedness" of a Var, so accessing either the .name or the .namespace field of the Var testing for nil is the only way to do it currently

given the above, the current patch seems unsatisfactory, here some proposed solutions:

  • make Var Named, make namespace return nil for an unnamed Var and name return "--unnamed--"
  • keep Var not implementing Named, add a "var-symbol" function returning either a namespaced symbol matching the ns+name of the Var or nil for an unnamed Var

Personally, I'd rather have the second solution implemented as I don't feel Var should be Named given that they can be unnamed and that strikes me as a contradicion

Comment by Reid McKenzie [ 02/Aug/14 3:16 PM ]

Added patches explicitly handling the unnamed var cases.

Comment by Reid McKenzie [ 02/Aug/14 3:33 PM ]

Squashed all patches into a single diff and updated attachments.





[CLJ-1442] Tag gensym sourced symbols with metadata Created: 09/Jun/14  Updated: 11/Aug/14

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

Type: Enhancement Priority: Trivial
Reporter: Reid McKenzie Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None

Attachments: Text File 0003-Annotate-generated-symbols-with-metadata.patch    
Patch: Code

 Description   

For static analysis tools derived from TANAL it is frequently useful to determine whether a symbol is user defined or the result of code generation. As tools analyzer depends on the Clojure core for evaluation and symbol generation a user wishing to annotate generated symbols must currently provide a binding replacing clojure.core/gensym with a snippet equivalent to the following patch. Such overloading is not appropriate for TANAL, TE* or user code as it is a redefinition of clojure.core behavior which should be standard rather than subjected to users with crowbars.



 Comments   
Comment by Gary Trakhman [ 09/Jun/14 2:11 PM ]

This could eventually help with filtering out def'd symbols like 't131045 coming from reify in CLJS. I've been seeing this behavior with core.async namespaces in an autodoc-cljs proof-of-concept, which could eventually target tools.analyzer.

Comment by Alex Miller [ 09/Jun/14 2:57 PM ]

Re the patch, why not call the Symbol constructor that takes meta instead of with-meta? For performance, it might also be useful to use the same constant map as well.

Comment by Reid McKenzie [ 09/Jun/14 3:10 PM ]

Because the compiler will emit the meta map as a static field the patch as-is will share the same map instance between all annotated symbols. Calling the metadata constructor is reasonable, I'll update the patch.

Comment by Reid McKenzie [ 09/Jun/14 3:28 PM ]

So the metadata constructor of Symbol is private, see https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Symbol.java#L100. Without changing this directly constructing symbols with metadata is not possible from the core. If you're worried about escaping the var indirection cost of adding metadata via with-meta inlining with-meta is an option, however then we're building two symbols for no good reason. Exposing the currently private metadata constructor is probably the right fix, abet its own ticket.

Comment by Andy Fingerhut [ 01/Jul/14 6:41 PM ]

From the comments above it appears that this is not planned to be a final version of this patch, but FYI some automated scripts I have found that patch 0001-Annotate-generated-symbols-with-metadata.patch dated Jun 9 2014 applies cleanly to the latest Clojure master as of Jul 1 2014, but Clojure fails to build.

Comment by Reid McKenzie [ 02/Jul/14 1:37 AM ]

Thanks Andy, I'll rework and test it in the morning

Comment by Reid McKenzie [ 07/Jul/14 9:49 AM ]

Because of the work that clojure.lang.Symbol/intern does, exposing and using the metadata constructor directly makes no sense. The updated patch directly invokes clojure.lang.Symbol/withMeta rather than indirecting through clojure.core/with-meta and taking the performance hit of calling through a Var. Builds cleanly on my system.

Comment by Andy Fingerhut [ 01/Aug/14 8:49 PM ]

Reid, although JIRA can handle multiple attachments with the same name, it can be a bit confusing for people, and for some scripts I have for determining which patches apply and test cleanly. Would you mind renaming one of your patches?

Comment by Reid McKenzie [ 01/Aug/14 10:55 PM ]

3rd and final cut at this patch.





[CLJ-1059] PersistentQueue doesn't implement java.util.List, causing nontransitive equality Created: 03/Sep/12  Updated: 11/Aug/14

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

Type: Defect Priority: Major
Reporter: Philip Potter Assignee: Philip Potter
Resolution: Unresolved Votes: 1
Labels: queue

Attachments: File 001-clj-1059-make-persistentqueue-implement-list.diff     File 002-clj-1059-asequential-rebased-to-cached-hasheq.diff    
Patch: Code and Test

 Description   

PersistentQueue implements Sequential but doesn't implement java.util.List. Lists form an equality partition, as do Sequentials. This means that you can end up with nontransitive equality:

(def q (conj clojure.lang.PersistentQueue/EMPTY 1 2 3))
;=> #user/q
(def al (doto (java.util.ArrayList.) (.add 1) (.add 2) (.add 3)))
;=> #user/al
(def v [1 2 3])
;=> #user/v
(= al v)
;=> true
(= v q)
;=> true
(not= al q)
;=> true

This happens because PersistentQueue is a Sequential but not a List, ArrayList is a List but not a Sequential, and PersistentVector is both.



 Comments   
Comment by Philip Potter [ 15/Sep/12 3:41 AM ]

Whoops, according to http://dev.clojure.org/display/design/JIRA+workflow I should have emailed clojure-dev before filing this ticket. Here is the discussion:

https://groups.google.com/d/topic/clojure-dev/ME3-Ke-RbNk/discussion

Comment by Philip Potter [ 15/Sep/12 2:37 PM ]

Attached 001-make-PersistentQueue-implement-List.diff, 15/Sep/12

Note that this patch has a minor conflict with the one I added to CLJ-1070, because both add an extra interface to PersistentQueue - List in this case, IHashEq in CLJ-1070.

Comment by Chouser [ 18/Sep/12 1:04 AM ]

Philip, patch looks pretty good – thanks for doing this. A couple notes:

This is only my opinion, but I prefer imports be listed without wildcards, even if it means an extra couple lines at the top of a .java file.

I noticed the "List stuff" code is a copy of what's in ASeq and EmptyList. I suppose this is copied because EmptyList and PersistentQueue extend Obj and therefore can't extend ASeq. Is this the only reason? It seems a shame to duplicate these method definitions, but I don't know of a better solution, do you?

It would also be nice if the test check a couple of the List methods you've implemented.

Comment by Chouser [ 18/Sep/12 1:08 AM ]

oh, also "git am" refused to apply the patch, but I'm not sure why. "patch -p 1" worked perfectly.

Comment by Philip Potter [ 18/Sep/12 1:19 AM ]

did you use the --keep-cr option to git am?

I struggled to know whether I should be adding CRs or not to line endings, because the files I was editing weren't consistent in their usage. If you open them in emacs, half the lines have ^M at the end.

Comment by Philip Potter [ 18/Sep/12 1:21 AM ]

Will submit another patch, with the import changed. I'll have a think about the list implementation and see what ideas I can come up with.

Comment by Philip Potter [ 18/Sep/12 3:17 PM ]

Attached 002-make-PersistentQueue-implement-Asequential.diff

This patch is an alternative to 001-make-PersistentQueue-implement-List.diff

So I took on board what you said about ASeq, but it didn't feel right making PersistentQueue directly implement ISeq, somehow.

So I split ASeq into two parts – ASequential, which implements j.u.{Collection,List} and manages List-equality and hashcodes; and ASeq, which... doesn't seem to be doing much anymore, to be honest.

As a bonus, this patch fixes CLJ-1070 too, so I went and added the tests from that ticket in to demonstrate this fact. It also tidies up PersistentQueue by removing all equals/hashcCode stuff and all Collection stuff.

(It turns out that because ASeq was already implementing Obj, the fact that PersistentQueue was implementing Obj was no barrier to using it.)

Would appreciate comments on this approach, and how it differs from the previous patch here and the patch on CLJ-1070.

Comment by Philip Potter [ 18/Sep/12 3:44 PM ]

Looking at EmptyList's implementation of List, it is a duplicate of the others, but it shouldn't be. I think its implementation of indexOf is the biggest culprit - it should just be 'return -1;' but it has a great big for loop! But this is beyond the scope of this ticket, so I won't patch that here.

Comment by Andy Fingerhut [ 20/Oct/12 12:29 PM ]

Philip, now that the patch for CLJ-1070 has been applied, these patches no longer apply cleanly. Would you be willing to update them? If so, please remove the obsolete patches.

Comment by Philip Potter [ 22/Oct/12 5:10 AM ]

Andy, thanks so much for your efforts to make people aware of these things. I will indeed submit new patches, hopefully later this week.

Comment by Philip Potter [ 03/Nov/12 12:23 PM ]

Replaced existing patches with new ones which apply cleanly to master.

There are two patches:

001-clj-1059-make-persistentqueue-implement-list.diff

This fixes equality by making PersistentQueue implement List directly. I also took the opportunity to remove the wildcard import and to add tests for the List methods, as compared with the previous version of the patch.

002-clj-1059-asequential.diff

This fixes equality by creating a new abstract class ASequential, and making PersistentQueue extend this.

My preferred solution is still the ASequential patch, but I'm leaving both here for comparison.

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

Vetting.

Comment by Andy Fingerhut [ 11/Dec/12 12:50 PM ]

Philip, this time I think it was patches that were committed for CLJ-1000 that make your patch 002-clj-1059-asequential.diff not apply cleanly. I often fix up stale patches where the change is straightforward and mechanical, but in this case you are moving some methods that CLJ-1000's patch changed the implementation of, so it would be best if someone figured out a way to update this patch in a way that doesn't clobber the CLJ-1000 changes.

Comment by Philip Potter [ 11/Dec/12 1:57 PM ]

Thanks Andy. Submitted a new patch, 002-clj-1059-asequential-rebased-to-cached-hasheq.diff, which supersedes 002-clj-1059-asequential.diff.

The patch 001-clj-1059-make-persistentqueue-implement-list.diff still applies cleanly, and is still an alternative to 002-clj-1059-asequential-rebased-to-cached-hasheq.diff.

Comment by Andy Fingerhut [ 30/Jan/14 4:50 PM ]

With the commits to Clojure master made in the week leading up to Jan 30 2014, particularly changes to hasheq, patch 002-clj-1059-asequential-rebased-to-cached-hasheq.diff no longer applies cleanly.

Patch 001-clj-1059-make-persistentqueue-implement-list.diff still does.

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

This issue was run into again and a duplicate ticket CLJ-1374 created – later closed as a duplicate of this one. Just wanted to record that this issue is being hit by others besides those originally reporting it.

Comment by Andy Fingerhut [ 11/Aug/14 1:37 AM ]

One or more commits made to Clojure master between Aug 1 2014 and Aug 10 2014 conflict with the patch 001-clj-1059-make-persistentqueue-implement-list.diff, and it no longer applies cleanly.





[CLJ-1152] PermGen leak in multimethods and protocol fns when evaled Created: 30/Jan/13  Updated: 08/Aug/14

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

Type: Defect Priority: Critical
Reporter: Chouser Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: memory, protocols

Attachments: File multifn_weak_method_cache.diff     File naive-lru-for-multimethods-and-protocols.diff     File naive-lru-method-cache-for-multimethods.diff    
Patch: Code
Approval: Vetted

 Description   

There is a PermGen memory leak that we have tracked down to protocol methods and multimethods called inside an eval, because of the caches these methods use. The problem only arises when the value being cached is an instance of a class (such as a function or reify) that was defined inside the eval. Thus extending IFn or dispatching a multimethod on an IFn are likely triggers.

My fellow LonoClouder, Jeff Dik describes how to reproduce and work around the problem:

The easiest way that I have found to test this is to set "-XX:MaxPermSize" to a reasonable value so you don't have to wait too long for the PermGen spaaaaace to fill up, and to use "-XX:+TraceClassLoading" and "-XX:+TraceClassUnloading" to see the classes being loaded and unloaded.

leiningen project.clj
(defproject permgen-scratch "0.1.0-SNAPSHOT"
  :dependencies [[org.clojure/clojure "1.5.0-RC1"]]
  :jvm-opts ["-XX:MaxPermSize=32M"
             "-XX:+TraceClassLoading"
             "-XX:+TraceClassUnloading"])

You can use lein swank 45678 and connect with slime in emacs via M-x slime-connect.

To monitor the PermGen usage, you can find the Java process to watch with "jps -lmvV" and then run "jstat -gcold <PROCESS_ID> 1s". According to the jstat docs, the first column (PC) is the "Current permanent spaaaaace capacity (KB)" and the second column (PU) is the "Permanent spaaaaace utilization (KB)". VisualVM is also a nice tool for monitoring this.

Multimethod leak

Evaluating the following code will run a loop that eval's (take* (fn foo [])).

multimethod leak
(defmulti take* (fn [a] (type a)))

(defmethod take* clojure.lang.Fn
  [a]
  '())

(def stop (atom false))
(def sleep-duration (atom 1000))

(defn run-loop []
  (when-not @stop
    (eval '(take* (fn foo [])))
    (Thread/sleep @sleep-duration)
    (recur)))

(future (run-loop))

(reset! sleep-duration 0)

In the lein swank session, you will see many lines like below listing the classes being created and loaded.

[Loaded user$eval15802$foo__15803 from __JVM_DefineClass__]
[Loaded user$eval15802 from __JVM_DefineClass__]

These lines will stop once the PermGen spaaaaace fills up.

In the jstat monitoring, you'll see the amount of used PermGen spaaaaace (PU) increase to the max and stay there.

-    PC       PU        OC          OU       YGC    FGC    FGCT     GCT
 31616.0  31552.7    365952.0         0.0      4     0    0.000    0.129
 32000.0  31914.0    365952.0         0.0      4     0    0.000    0.129
 32768.0  32635.5    365952.0         0.0      4     0    0.000    0.129
 32768.0  32767.6    365952.0      1872.0      5     1    0.000    0.177
 32768.0  32108.2    291008.0     23681.8      6     2    0.827    1.006
 32768.0  32470.4    291008.0     23681.8      6     2    0.827    1.006
 32768.0  32767.2    698880.0     24013.8      8     4    1.073    1.258
 32768.0  32767.2    698880.0     24013.8      8     4    1.073    1.258
 32768.0  32767.2    698880.0     24013.8      8     4    1.073    1.258

A workaround is to run prefer-method before the PermGen spaaaaace is all used up, e.g.

(prefer-method take* clojure.lang.Fn java.lang.Object)

Then, when the used PermGen spaaaaace is close to the max, in the lein swank session, you will see the classes created by the eval'ing being unloaded.

[Unloading class user$eval5950$foo__5951]
[Unloading class user$eval3814]
[Unloading class user$eval2902$foo__2903]
[Unloading class user$eval13414]

In the jstat monitoring, there will be a long pause when used PermGen spaaaaace stays close to the max, and then it will drop down, and start increasing again when more eval'ing occurs.

-    PC       PU        OC          OU       YGC    FGC    FGCT     GCT
 32768.0  32767.9    159680.0     24573.4      6     2    0.167    0.391
 32768.0  32767.9    159680.0     24573.4      6     2    0.167    0.391
 32768.0  17891.3    283776.0     17243.9      6     2   50.589   50.813
 32768.0  18254.2    283776.0     17243.9      6     2   50.589   50.813

The defmulti defines a cache that uses the dispatch values as keys. Each eval call in the loop defines a new foo class which is then added to the cache when take* is called, preventing the class from ever being GCed.

The prefer-method workaround works because it calls clojure.lang.MultiFn.preferMethod, which calls the private MultiFn.resetCache method, which completely empties the cache.

Protocol leak

The leak with protocol methods similarly involves a cache. You see essentially the same behavior as the multimethod leak if you run the following code using protocols.

protocol leak
(defprotocol ITake (take* [a]))

(extend-type clojure.lang.Fn
  ITake
  (take* [this] '()))

(def stop (atom false))
(def sleep-duration (atom 1000))

(defn run-loop []
  (when-not @stop
    (eval '(take* (fn foo [])))
    (Thread/sleep @sleep-duration)
    (recur)))

(future (run-loop))

(reset! sleep-duration 0)

Again, the cache is in the take* method itself, using each new foo class as a key.

A workaround is to run -reset-methods on the protocol before the PermGen spaaaaace is all used up, e.g.

(-reset-methods ITake)

This works because -reset-methods replaces the cache with an empty MethodImplCache.



 Comments   
Comment by Chouser [ 30/Jan/13 9:10 AM ]

I think the most obvious solution would be to constrain the size of the cache. Adding an item to the cache is already not the fastest path, so a bit more work could be done to prevent the cache from growing indefinitely large.

That does raise the question of what criteria to use. Keep the first n entries? Keep the n most recently used (which would require bookkeeping in the fast cache-hit path)? Keep the n most recently added?

Comment by Jamie Stephens [ 18/Oct/13 9:35 AM ]

At a minimum, perhaps a switch to disable the caches – with obvious performance impact caveats.

Seems like expensive LRU logic is probably the way to go, but maybe don't have it kick in fully until some threshold is crossed.

Comment by Alex Miller [ 18/Oct/13 4:28 PM ]

A report seeing this in production from mailing list:
https://groups.google.com/forum/#!topic/clojure/_n3HipchjCc

Comment by Adrian Medina [ 10/Dec/13 11:43 AM ]

So this is why we've been running into PermGen space exceptions! This is a fairly critical bug for us - I'm making extensive use of multimethods in our codebase and this exception will creep in at runtime randomly.

Comment by Kevin Downey [ 17/Apr/14 9:52 PM ]

it might be better to split this in to two issues, because at a very abstract level the two issues are the "same", but concretely they are distinct (protocols don't really share code paths with multimethods), keeping them together in one issue seems like a recipe for a large hard to read patch

Comment by Kevin Downey [ 26/Jul/14 5:49 PM ]

naive-lru-method-cache-for-multimethods.diff replaces the methodCache in multimethods with a very naive lru cache built on PersistentHashMap and PersistentQueue

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

naive-lru-for-multimethods-and-protocols.diff creates a new class clojure.lang.LRUCache that provides an lru cache built using PHashMap and PQueue behind an IPMap interface.

changes MultiFn to use an LRUCache for its method cache.

changes expand-method-impl-cache to use an LRUCache for MethodImplCache's map case

Comment by Kevin Downey [ 30/Jul/14 3:10 PM ]

I suspect my patch naive-lru-for-multimethods-and-protocols.diff is just wrong, unless MethodImplCache really is being used as a cache we can't just toss out entries when it gets full.

looking at the deftype code again, it does look like MethidImplCache is being used as a cache, so maybe the patch is fine

if I am sure of anything it is that I am unsure so hopefully someone who is sure can chime in

Comment by Nicola Mometto [ 31/Jul/14 11:02 AM ]

I haven't looked at your patch, but I can confirm that the MethodImplCache in the protocol function is just being used as a cache

Comment by dennis zhuang [ 08/Aug/14 6:21 AM ]

I developed a new patch that convert the methodCache in MultiFn to use WeakReference for dispatch value,and clear the cache if necessary.

I've test it with the code in ticket,and it looks fine.The classes will be unloaded when perm gen is almost all used up.





[CLJ-1495] Defining a record with defrecord twice breaks record equality Created: 07/Aug/14  Updated: 07/Aug/14

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

Type: Defect Priority: Minor
Reporter: Matt Halverson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: defrecord


 Description   

If I spin up a fresh repl and type the following four lines, I consistently get this unexpected behavior. I discovered it because it was breaking a unit test.

user> (defrecord Foo [bar])
user.Foo
user> (= (->Foo 42) #user.Foo{:bar 42}) ;;expect this to evaluate to true
true
user> (defrecord Foo [bar])
user.Foo
user> (= (->Foo 42) #user.Foo{:bar 42}) ;;expect this to evaluate to true also -- but it doesn't!
false
user>

This may be related to http://dev.clojure.org/jira/browse/CLJ-1457.

You may also find the following interesting (posted by a fellow irc chatter, reproducible on my machine):

user=> (defrecord Foo [a])
user.Foo
user=> #user.Foo[1]
#user.Foo{:a 1}
user=> (defrecord Foo [b])
user.Foo
user=> (Foo. 1)
#user.Foo{:a 1}





[CLJ-1322] doseq with several bindings causes "ClassFormatError: Invalid Method Code length" Created: 10/Jan/14  Updated: 07/Aug/14

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

Type: Defect Priority: Major
Reporter: Miikka Koskinen Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None
Environment:

Clojure 1.5.1, java 1.7.0_25, OpenJDK Runtime Environment (IcedTea 2.3.10) (7u25-2.3.10-1ubuntu0.12.04.2)


Attachments: Text File doseq-bench.txt     Text File doseq.patch     File script.clj    
Patch: Code
Approval: Screened

 Description   

Important Perf Note the new impl is faster for collections that are custom-reducible but not chunked, and is also faster for large numbers of bindings. The original implementation is hand tuned for chunked collections, and wins for larger chunked coll/smaller binding count scenarios, presumably due to the fn call/return tracking overhead of reduce. Details are in the comments.
Screened By Stu
Patch doseq.patch

user=> (def a1 (range 10))
#'user/a1
user=> (doseq [x1 a1 x2 a1 x3 a1 x4 a1 x5 a1 x6 a1 x7 a1 x8 a1] (do))
CompilerException java.lang.ClassFormatError: Invalid method Code length 69883 in class file user$eval1032, compiling:(NO_SOURCE_PATH:2:1)

While this example is silly, it's a problem we've hit a couple of times. It's pretty surprising when you have just a couple of lines of code and suddenly you get the code length error.



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

reproduces with jdk 1.8.0 and clojure 1.6

Comment by Nicola Mometto [ 22/Apr/14 5:35 PM ]

A potential fix for this is to make doseq generate intermediate fns like `for` does instead of expanding all the code directly.

Comment by Ghadi Shayban [ 25/Jun/14 8:39 PM ]

Existing doseq handles chunked-traversal internally, deciding the
mechanics of traversal for a seq. In addition to possibly conflating
concerns, this is causing a code explosion blowup when more bindings are
added, approx 240 bytes of bytecode per binding (without modifiers).

This approach redefs doseq later in core.clj, after protocol-based
reduce (and other modern conveniences like destructuring.)

It supports the existing :let, :while, and :when modifiers.

New is a stronger assertion that modifiers cannot come before binding
expressions. (Same semantics as let, i.e. left to right)

valid: [x coll :when (foo x)]
invalid: [:when (foo x) x coll]

This implementation does not suffer from the code explosion problem.
About 25 bytes of bytecode + 1 fn per binding.

Implementing this without destructuring was not a party, luckily reduce
is defined later in core.

Comment by Andy Fingerhut [ 26/Jun/14 12:25 AM ]

For anyone reviewing this patch, note that there are already many tests for correct functionality of doseq in file test/clojure/test_clojure/for.clj. It may not be immediately obvious, but every test for 'for' defined with deftest-both is a test for 'for' and also for 'doseq'.

Regarding the current implementation of doseq: it in't simply that it is too many bytes per binding, it is that the code size doubles with each additional binding. See these results, which measures size of the macroexpanded form rather than byte code size, but those two things should be fairly linearly related to each other here:

(defn formsize [form]
  (count (with-out-str (print (macroexpand form)))))

user=> (formsize '(doseq [x (range 10)] (print x)))
652
user=> (formsize '(doseq [x (range 10) y (range 10)] (print x y)))
1960
user=> (formsize '(doseq [x (range 10) y (range 10) z (range 10)] (print x y z)))
4584
user=> (formsize '(doseq [x (range 10) y (range 10) z (range 10) w (range 10)] (print x y z w)))
9947
user=> (formsize '(doseq [x (range 10) y (range 10) z (range 10) w (range 10) p (range 10)] (print x y z w p)))
20997

Here are results for the same expressions after Ghadi's patch doseq.patch dated June 25 2014:

user=> (formsize '(doseq [x (range 10)] (print x)))
93
user=> (formsize '(doseq [x (range 10) y (range 10)] (print x y)))
170
user=> (formsize '(doseq [x (range 10) y (range 10) z (range 10)] (print x y z)))
247
user=> (formsize '(doseq [x (range 10) y (range 10) z (range 10) w (range 10)] (print x y z w)))
324
user=> (formsize '(doseq [x (range 10) y (range 10) z (range 10) w (range 10) p (range 10)] (print x y z w p)))
401

It would be good to see some performance results with and without this patch, too.

Comment by Stuart Halloway [ 28/Jun/14 2:21 PM ]

In the tests below, the new impl is called "doseq2", vs. the original impl "doseq"

(def hund (into [] (range 100)))
(def ten (into [] (range 10)))
(def arr (int-array 100))
(def s "superduper")

;; big seq, few bindings: doseq2 LOSES
(dotimes [_ 5]
  (time (doseq [a (range 100000000)])))
;; 1.2 sec

(dotimes [_ 5]
  (time (doseq2 [a (range 100000000)])))
;; 1.8 sec

;; small unchunked reducible, few bindings: doseq2 wins
(dotimes [_ 5]
  (time (doseq [a s b s c s])))
;; 0.5 sec

(dotimes [_ 5]
  (time (doseq2 [a s b s c s])))
;; 0.2 sec

(dotimes [_ 5]
  (time (doseq [a arr b arr c arr])))
;; 40 msec

(dotimes [_ 5]
  (time (doseq2 [a arr b arr c arr])))
;; 8 msec

;; small chunked reducible, few bindings: doseq2 LOSES
(dotimes [_ 5]
  (time (doseq [a hund b hund c hund])))
;; 2 msec

(dotimes [_ 5]
  (time (doseq2 [a hund b hund c hund])))
;; 8 msec

;; more bindings: doseq2 wins bigger and bigger
(dotimes [_ 5]
  (time (doseq [a ten b ten c ten d ten ])))
;; 2 msec

(dotimes [_ 5]
  (time (doseq2 [a ten b ten c ten d ten ])))
;; 0.4 msec

(dotimes [_ 5]
  (time (doseq [a ten b ten c ten d ten e ten])))
;; 18 msec

(dotimes [_ 5]
  (time (doseq2 [a ten b ten c ten d ten e ten])))
;; 1 msec
Comment by Ghadi Shayban [ 28/Jun/14 6:23 PM ]

Hmm, I cannot reproduce your results.

I'm not sure whether you are testing with lein, on what platform, what jvm opts.

Can we test using this little harness instead directly against clojure.jar? I've attached a the harness and two runs of results (one w/ default heap, the other 3GB w/ G1GC)

I added a medium and small (range) too.

Anecdotally, I see doseq2 outperform in all cases except the small range. Using criterium shows a wider performance gap favoring doseq2.

I pasted the results side by side for easier viewing.

core/doseq                          doseq2
"Elapsed time: 1610.865146 msecs"   "Elapsed time: 2315.427573 msecs"
"Elapsed time: 2561.079069 msecs"   "Elapsed time: 2232.479584 msecs"
"Elapsed time: 2446.674237 msecs"   "Elapsed time: 2234.556301 msecs"
"Elapsed time: 2443.129809 msecs"   "Elapsed time: 2224.302855 msecs"
"Elapsed time: 2456.406103 msecs"   "Elapsed time: 2210.383112 msecs"

;; med range, few bindings:
core/doseq                          doseq2
"Elapsed time: 28.383197 msecs"     "Elapsed time: 31.676448 msecs"
"Elapsed time: 13.908323 msecs"     "Elapsed time: 11.136818 msecs"
"Elapsed time: 18.956345 msecs"     "Elapsed time: 11.137122 msecs"
"Elapsed time: 12.367901 msecs"     "Elapsed time: 11.049121 msecs"
"Elapsed time: 13.449006 msecs"     "Elapsed time: 11.141385 msecs"

;; small range, few bindings:
core/doseq                          doseq2
"Elapsed time: 0.386334 msecs"      "Elapsed time: 0.372388 msecs"
"Elapsed time: 0.10521 msecs"       "Elapsed time: 0.203328 msecs"
"Elapsed time: 0.083378 msecs"      "Elapsed time: 0.179116 msecs"
"Elapsed time: 0.097281 msecs"      "Elapsed time: 0.150563 msecs"
"Elapsed time: 0.095649 msecs"      "Elapsed time: 0.167609 msecs"

;; small unchunked reducible, few bindings:
core/doseq                          doseq2
"Elapsed time: 2.351466 msecs"      "Elapsed time: 2.749858 msecs"
"Elapsed time: 0.755616 msecs"      "Elapsed time: 0.80578 msecs"
"Elapsed time: 0.664072 msecs"      "Elapsed time: 0.661074 msecs"
"Elapsed time: 0.549186 msecs"      "Elapsed time: 0.712239 msecs"
"Elapsed time: 0.551442 msecs"      "Elapsed time: 0.518207 msecs"

core/doseq                          doseq2
"Elapsed time: 95.237101 msecs"     "Elapsed time: 55.3067 msecs"
"Elapsed time: 41.030972 msecs"     "Elapsed time: 30.817747 msecs"
"Elapsed time: 42.107288 msecs"     "Elapsed time: 19.535747 msecs"
"Elapsed time: 41.088291 msecs"     "Elapsed time: 4.099174 msecs"
"Elapsed time: 41.03616 msecs"      "Elapsed time: 4.084832 msecs"

;; small chunked reducible, few bindings:
core/doseq                          doseq2
"Elapsed time: 31.793603 msecs"     "Elapsed time: 40.082492 msecs"
"Elapsed time: 17.302798 msecs"     "Elapsed time: 28.286991 msecs"
"Elapsed time: 17.212189 msecs"     "Elapsed time: 14.897374 msecs"
"Elapsed time: 17.266534 msecs"     "Elapsed time: 10.248547 msecs"
"Elapsed time: 17.227381 msecs"     "Elapsed time: 10.022326 msecs"

;; more bindings:
core/doseq                          doseq2
"Elapsed time: 4.418727 msecs"      "Elapsed time: 2.685198 msecs"
"Elapsed time: 2.421063 msecs"      "Elapsed time: 2.384134 msecs"
"Elapsed time: 2.210393 msecs"      "Elapsed time: 2.341696 msecs"
"Elapsed time: 2.450744 msecs"      "Elapsed time: 2.339638 msecs"
"Elapsed time: 2.223919 msecs"      "Elapsed time: 2.372942 msecs"

core/doseq                          doseq2
"Elapsed time: 28.869393 msecs"     "Elapsed time: 2.997713 msecs"
"Elapsed time: 22.414038 msecs"     "Elapsed time: 1.807955 msecs"
"Elapsed time: 21.913959 msecs"     "Elapsed time: 1.870567 msecs"
"Elapsed time: 22.357315 msecs"     "Elapsed time: 1.904163 msecs"
"Elapsed time: 21.138915 msecs"     "Elapsed time: 1.694175 msecs"
Comment by Ghadi Shayban [ 28/Jun/14 6:47 PM ]

It's good that the benchmarks contain empty doseq bodies in order to isolate the overhead of traversal. However, that represents 0% of actual real-world code.

At least for the first benchmark (large chunked seq), adding in some tiny amount of work did not change results signifantly. Neither for (map str [a])

(range 10000000) =>  (map str [a])
core/doseq
"Elapsed time: 586.822389 msecs"
"Elapsed time: 563.640203 msecs"
"Elapsed time: 369.922975 msecs"
"Elapsed time: 366.164601 msecs"
"Elapsed time: 373.27327 msecs"
doseq2
"Elapsed time: 419.704021 msecs"
"Elapsed time: 371.065783 msecs"
"Elapsed time: 358.779231 msecs"
"Elapsed time: 363.874448 msecs"
"Elapsed time: 368.059586 msecs"

nor for intrisics like (inc a)

(range 10000000)
core/doseq
"Elapsed time: 317.091849 msecs"
"Elapsed time: 272.360988 msecs"
"Elapsed time: 215.501737 msecs"
"Elapsed time: 206.639181 msecs"
"Elapsed time: 206.883343 msecs"
doseq2
"Elapsed time: 241.475974 msecs"
"Elapsed time: 193.154832 msecs"
"Elapsed time: 198.757873 msecs"
"Elapsed time: 197.803042 msecs"
"Elapsed time: 200.603786 msecs"

I still see reduce-based doseq ahead of the original, except for small seqs

Comment by Ghadi Shayban [ 04/Aug/14 2:55 PM ]

A form like the following will not work with this patch:

(go (doseq [c chs] (>! c :foo)))

as the go macro doesn't traverse fn boundaries. The only such code I know is core.async/mapcat*, a private fn supporting a fn that is marked deprecated.

Comment by Ghadi Shayban [ 07/Aug/14 2:09 PM ]

I see #'clojure.core/run! was just added, which has a similar limitation





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

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

Type: Defect Priority: Major
Reporter: Meikel Brandmeyer Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: interop

Attachments: Text File 0001-Fix-iterator-implementations-to-throw-NSEE-when-exha.patch     Text File 0001-Throw-NSEE-in-gvec-iterator.patch    
Patch: Code
Approval: Triaged

 Description   

Most implementations of Iterators for Clojure's collections do not implement the next method correctly. In case the iterator is exhausted the methods fail with some case dependent error, but not with the NoSuchElementException as required by the official Java contract for that method. This causes problems when working with Java libraries relying on that behaviour.

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

To reproduce:

(-> [] .iterator .next)

This throws a NPE instead of NSEE.

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

This throws an ArrayIndexOutOfBoundsException instead of NSEE.

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

Up-to-date patch file: 0001-Fix-iterator-implementations-to-throw-NSEE-when-exha.patch



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

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

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

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

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

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

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

That should be a quick fix.

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

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

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

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

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

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

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





[CLJ-1492] PersistentQueue objects are improperly eval'd and compiled Created: 06/Aug/14  Updated: 07/Aug/14

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

Type: Defect Priority: Minor
Reporter: Jon Distad Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler
Environment:

OS X 10.9.4
java version "1.7.0_60"
Java(TM) SE Runtime Environment (build 1.7.0_60-b19)
Java HotSpot(TM) 64-Bit Server VM (build 24.60-b09, mixed mode)


Attachments: Text File 0001-Exclude-PersistentQueue-from-IPersistentList-eval-co.patch    
Patch: Code and Test
Approval: Triaged

 Description   

PersistentQueue objects do not follow the correct evaluation path in the Compiler.

The simplest case:

user=> (def q (conj clojure.lang.PersistentQueue/EMPTY 1 2 3))
#'user/q
user=> q
#<PersistentQueue clojure.lang.PersistentQueue@7861>
user=> (eval q)
CompilerException java.lang.ClassCastException: clojure.lang.PersistentQueue cannot be cast to java.util.List, compiling:(NO_SOURCE_PATH:4:1)

And you get the same exception when embedding a PersistentQueue:

user=> (eval `(fn [] ~q))
CompilerException java.lang.ClassCastException: clojure.lang.PersistentQueue cannot be cast to java.util.List, compiling:(NO_SOURCE_PATH:2:1)

Instead of the expected:

CompilerException java.lang.RuntimeException: Can't embed unreadable object in code: #<PersistentQueue clojure.lang.PersistentQueue@7861>, compiling:(NO_SOURCE_PATH:3:1)

Since PersistentQueue implements IPersistentCollection and IPersistentList, and is not called out explicitly in the compiler, it is falling into the same compile path as a list. The exception comes from the call to emitValue inside the emitConstants portion of the FnExpr emit path. PersistentQueue does not implement java.util.List and thus the cast in emitListAsObjectArray (Compiler.java:4479) throws. Implementing List would NOT, however, resolve this issue, but would mask it by causing all eval'd PersistedQueues to be compiled as PersistentLists.

The first case is resolved by adding `&& !(form instanceof PersistentQueue)` to the IPersistentCollection branch of Compiler.eval() (Compiler.java:6695-8), allowing the PersistentQueue to fall through to the ConstantExpr case in analyze (Compiler.java:6459). The embedding case is resolved by adding `&& !(value instanceof PersistentQueue)` to the IPersistentList branch in ObjExpr's emitValue (Compiler.java:4639).

This bug also precludes definition of data-readers for PersistentQueue as the read object throws an exception when it is passed to the Compiler.

The attached patch includes the two changes mentioned above, and tests for each case that illustrates the bug.

Clojure-dev thread: https://groups.google.com/forum/#!topic/clojure-dev/LDUQfqjFg9w






[CLJ-1494] remove flatmap in favor of mapcat Created: 07/Aug/14  Updated: 07/Aug/14

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

Type: Enhancement Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File 0001-remove-flatmap-use-1-arity-mapcat-instead.patch    
Patch: Code

 Description   

While all the transducers functions are implemented as an arity in the matching clojure core sequence, for mapcat a new function has been added: flatmap.
The reason for this is, as Rich said in a HN comment, "because mapcat's signature was not amenable to the additional arity".
This patch changes the mapcat signature to take at least one collection so that it's possible to add the 1-arity for the transducer function, eliminating the need for a different function, flatmap.

There has been no loss by removing the 1-arity version of mapcat as a sequence function since trying to use (mapcat f) as currently defined (not as redefined with this patch) would fail before transducers, and after transducers:
Before transducers (mapcat f) would result in a call to (map f) which would fail with an ArityException
After transducers that (map f) call would return a function, which then would be used as an argument to (apply concat the-f), resulting in a IllegalArgumentException since apply expects a sequence but it's been given a fn.






[CLJ-1103] Make conj assoc dissoc and transient versions handle args similarly Created: 04/Nov/12  Updated: 06/Aug/14

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

Type: Enhancement Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: None

Attachments: File clj-1103-5.diff    
Patch: Code and Test

 Description   

Examples that work as expected:

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

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

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

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

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

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

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



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

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

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

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

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

So +1 from me

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

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

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

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

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

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

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

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

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

What do you mean code examples?

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

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

Similarly for the transient ! versions.

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

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

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





[CLJ-1386] Add transient? predicate Created: 17/Mar/14  Updated: 06/Aug/14

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

Type: Enhancement Priority: Minor
Reporter: Devin Walters Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections, transient
Environment:

N/A


Attachments: Text File 0001-Add-transient-predicate.patch     Text File 0002-Add-transient-predicate.patch     Text File 0003-Add-transient-predicate.patch    
Patch: Code and Test
Approval: Triaged

 Description   

I've encountered situations where I wanted to check whether something was transient in order to know whether I should call assoc! or assoc, conj! or conj, etc.

This patch adds `transient?` as a predicate fn.



 Comments   
Comment by Alex Miller [ 17/Mar/14 10:21 AM ]

Patch needs a docstring and a test.

Comment by Devin Walters [ 17/Mar/14 4:42 PM ]

Alex: I figured that would be the case! Sorry about that. I've updated the patch. It now includes a docstring and has tests of `transient?` for #{}, [], and {}.

Thanks!

Comment by Alex Miller [ 17/Mar/14 9:48 PM ]

Thanks - please don't use the labels "patch" or "test" - those are covered by the Patch field.

Comment by Devin Walters [ 18/Mar/14 9:17 AM ]

Ah, sorry for the mixup Alex. I assumed you removed "patch" as a label the first time around to flag this ticket as still needing a vetted patch. My mistake.

Comment by Andy Fingerhut [ 21/Mar/14 1:42 PM ]

Patch 0001-Add-transient-predicate.patch dated Mar 17, 2014 applies cleanly to latest Clojure master, but fails a test because the new function transient? has no :added metadata. See most other Clojure functions in clojure.core for examples.

It also generates a new warning while running tests:

WARNING: transient? already refers to: #'clojure.core/transient? in namespace: clojure.test-clojure.data-structures, being replaced by: #'clojure.test-clojure.data-structures/transient?

There is an older (but equivalent) definition of transient? in test file data_structures.clj that should be removed when adding it to clojure.core

Comment by Devin Walters [ 22/Mar/14 11:29 PM ]

@Andy, the reason I did not add :added metadata is because I do not know if/when this patch will be accepted, and as a result, I don't really know if it will sneak into 1.6.X or 1.7. For now, I've put it in as 1.7. If it's in the running to be added sooner than that, let me know and I'll adjust it.

RE: The warning. Good catch. I've submitted a new patch which removes the private version of transient? from data_structures.clj. All tests pass.

Edit to Add: The latest patch as of this comment is now 0002-Add-transient-predicate.patch.

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

Patch 0002-Add-transient-predicate.patch dated Mar 22 2014 no longer applies cleanly to latest Clojure master due to some changes committed earlier today. I haven't checked whether this patch is straightforward to update.

Comment by Devin Walters [ 06/Aug/14 4:11 PM ]

I've updated the patch to 0003-Add-transient-predicate.patch. This patch applies cleanly to the latest version of master.





[CLJ-1385] Docstrings for `conj!` and `assoc!` should suggest using the return value; effect not always in-place Created: 16/Mar/14  Updated: 06/Aug/14

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

Type: Enhancement Priority: Minor
Reporter: Pyry Jahkola Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: collections, docstring

Attachments: Text File CLJ-1385-reword-docstrings-on-transient-update-funct-2.patch     Text File CLJ-1385-reword-docstrings-on-transient-update-funct.patch    
Patch: Code

 Description   

The docstrings of both `assoc!` and `conj!` say "Returns coll.", suggesting the transient edit happens always in-place, `coll` being the first argument.

However, the fact that the following example omits the key `8` in its result proves that in-place edits aren't always the case:

(let [a (transient {})]
      (dotimes [x 9]
        (assoc! a x :ok))
      (persistent! a))
    ;;=> {0 :ok, 1 :ok, 2 :ok, 3 :ok, 4 :ok, 5 :ok, 6 :ok, 7 :ok}

Instead, programmers should be guided towards using constructs like `reduce` with transients:

(persistent! (reduce #(assoc! %1 %2 :ok)
                 (transient {})
                 (range 9)))
    ;;=> {0 :ok, 1 :ok, 2 :ok, 3 :ok, 4 :ok, 5 :ok, 6 :ok, 7 :ok, 8 :ok}

The easiest way to achieve this is by changing the docstrings of (at least) `conj!` and `assoc!` to not read "Returns coll." but instead tell that the change is destructive.



 Comments   
Comment by Alex Miller [ 16/Mar/14 8:49 AM ]

When modifying transient collections, it is required to use the collection returned from functions like assoc!. The ! here indicates its destructive nature. The transients page (http://clojure.org/transients) describes the calling pattern pretty explicitly: "You must capture and use the return value in the next call."

I do not agree that we should be guiding programmers away from using functions like assoc! – transients are used as a performance optimization and using assoc! or conj! in a loop is often the fastest version of that. However I do think it would be helpful to make the docstring more explicit.

Comment by Gary Fredericks [ 05/Apr/14 10:23 AM ]

Alex I think you must have misread the ticket – the OP is suggesting guiding toward using the return value of assoc!, not avoiding assoc! altogether.

And the docstring is not simply inexplicit, it's actually incorrect specifically in the case that the OP pointed out. conj! and assoc do not return coll at the point where array-maps transition to hash-maps, and the fact that they do otherwise is supposed to be an implementation detail as far as I understand it.

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

@Gary - you're right, I did misread that.

assoc and conj both explicitly say "return a new collection" whereas assoc! and conj! say "Returns coll." I read that as "returns the modified collection" without regard to whether it's the identical instance, but I can read it your way too.

Would saying "Returns updated collection." transmit the right idea? Using "collection" instead of "coll" removes the concrete tie to the variable and "updated" hints more strongly that you should use the return value.

Comment by Pyry Jahkola [ 05/Apr/14 12:47 PM ]

@Alex, that update makes it sound right to me, FWIW.

Comment by Gary Fredericks [ 05/Apr/14 2:37 PM ]

Yeah, I think that's better. Thanks Alex. I'd be happy to submit a patch for that but I'm assuming patches are too heavy for this kind of change?

Comment by Andy Fingerhut [ 06/Apr/14 3:35 PM ]

Patches are exactly what has been done in the past for this kind of change, if it is in a doc string and not on the clojure.org web page.

Comment by Alex Miller [ 06/Apr/14 4:13 PM ]

Yup, patch desired.

Comment by Gary Fredericks [ 06/Apr/14 5:32 PM ]

Glad I asked.

Patch is attached that also updates the docstring for pop! which had the same issue, though arguably it's less important since afaik pop! does always return the identical collection (but I don't think this is part of the contract).

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

Patch CLJ-1385-reword-docstrings-on-transient-update-funct.patch dated Apr 6 2014 no longer applies to latest Clojure master cleanly, due to some changes committed earlier today. I suspect it should be straightforward to update the patch to apply cleanly, given that they are doc string changes, but there may have been doc string changes committed to master, too.

Comment by Gary Fredericks [ 06/Aug/14 3:04 PM ]

Attached a new patch.





[CLJ-1412] Add 2-arity version of `cycle` that takes the numer of times to "repeat" the coll Created: 28/Apr/14  Updated: 06/Aug/14

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

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-Add-2-arity-version-of-cycle-that-takes-the-number-o.patch    
Patch: Code

 Description   

There are already similar arities for repeat/repeatedly and similar functions, this patch adds a 2-arity version of cycle that behaves like this:

user> (cycle 0 '(1 2))
()
user> (cycle -1 '(1 2))
()
user> (cycle 3 '(1 2))
(1 2 1 2 1 2)
user> (cycle 1 '(1 2))
(1 2)


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

Patch 0001-Add-2-arity-version-of-cycle-that-takes-the-number-o.patch dated Apr 28 2014 no longer applies cleanly to latest Clojure master due to some changes committed earlier today. This appears trivial to update, as it is likely only a couple of lines of diff context that have changed.

Comment by Nicola Mometto [ 06/Aug/14 2:36 PM ]

Updated patch to apply to HEAD





[CLJ-1444] Fix unquote splicing for empty seqs Created: 11/Jun/14  Updated: 06/Aug/14

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: 0
Labels: reader, syntax-quote

Attachments: Text File 0001-Fix-unquote-splicing-for-empty-seqs-This-required-ma.patch    
Patch: Code and Test

 Description   

Current behaviour:

user=> `(~@())
nil
user=> `[~@()]
[]

Expected behaviour:

user=> `(~@())
()
user=> `[~@()]
[]


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

Patch 0001-Fix-unquote-splicing-for-empty-seqs.patch dated Jun 11 2014 no longer applies cleanly to latest Clojure master due to some changes committed earlier today. I haven't checked whether this patch is straightforward to update.

Comment by Nicola Mometto [ 06/Aug/14 2:31 PM ]

Updated patch to apply to HEAD





[CLJ-1489] Implement var-symbol Created: 02/Aug/14  Updated: 06/Aug/14

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

Type: Enhancement Priority: Trivial
Reporter: Reid McKenzie Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-Implement-var-symbol.patch    

 Description   

var-symbol provides the obvious complement operation to resolve. Where resolve maps from a symbol to a var by resolving it in the environment, var-symbol allows a user to recover the root binding symbol from a var if the var is named. If the var is not named, var-symbol returns nil.

This is related to CLJ-1488 in that it handles the common case of symbolically manipulating Vars in terms of the Symbols they bind without requiring that users manually reconstruct the bound symbol. Futhermore this patch nicely handles the non-obvious implementation consequent case of an unnamed var.

Depends on CLJ-1488



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

Patch 0001-Implement-var-symbol.patch dated Aug 2 2014 does not apply cleanly. I haven't checked whether it used to apply cleanly before some commits made to Clojure master earlier today, but if it did, then those commits have made this patch become 'stale'.

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





[CLJ-1451] Add take-until Created: 20/Jun/14  Updated: 06/Aug/14

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

Type: Enhancement Priority: Minor
Reporter: Alexander Taggart Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

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





[CLJ-1115] multi arity into Created: 25/Nov/12  Updated: 06/Aug/14

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

Type: Enhancement Priority: Trivial
Reporter: Yongqian Li Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: File multi-arity-into.diff    
Patch: Code and Test

 Description   

Any reason why into isn't multi arity?

(into to & froms) => (reduce into to froms)

(into #{} [3 3 4] [2 1] ["a"]) looks better than (reduce into #{} [[3 3 4] [2 1] ["a"]])



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

Seems to be a valid enhancement. I can't see any issues we'd have with it. Vetted.

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

Added patch & test. This patch retains the old performance characteristics of into in the case that there is only one collection argument. For example: (into [] [1 2 3]) .

Since the multi-arity version will be slightly slower, I opted to provide it as a second body instead of unifying both into a single body. If someone has a problem with this, I can rewrite the patch. At least this way, into won't get slower.

Comment by Rich Hickey [ 09/Dec/12 7:39 AM ]

This is a good example of an idea for an enhancement I haven't approved, and thus is not yet vetted.

Comment by Yongqian Li [ 05/Feb/14 1:16 AM ]

What's the plan for this bug?

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

@Yongqian This ticket is one of several hundred open enhancements right now. It is hard to predict when it will be considered. Voting (or encouraging others to vote) on an issue will raise it's priority. The list at http://dev.clojure.org/display/community/Screening+Tickets indicates how we prioritize triage.

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

Patch multi-arity-into.diff dated Nov 29 2012 no longer applied cleanly to latest Clojure master due to commits made earlier today. Those changes add another arity to the function clojure.core/into, so the multi-arity generalization of into seems unlikely to be compatible with those changes.





[CLJ-1424] Feature Expressions Created: 15/May/14  Updated: 06/Aug/14

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

Type: Enhancement Priority: Major
Reporter: Ghadi Shayban Assignee: Alex Miller
Resolution: Unresolved Votes: 0
Labels: reader

Attachments: File CLJ-1424-2.diff     File clojure-feature-expressions.diff    
Patch: Code and Test
Approval: Vetted

 Description   

Feature expressions based directly on Common Lisp. See Clojure design docs, which includes discussion and links to Common Lisp documentation for feature expressions here: http://dev.clojure.org/display/design/Feature+Expressions

#+ #- and or not
are supported. Unreadable tagged literals are suppressed through the *suppress-read* dynamic var. For example, with *features* being #{:clj}, which is the default, the following should read :foo

#+cljs #js {:one :two} :foo

The initial *features* set can be augmented (clj will always be included) with the clojure.features System property:

-Dclojure.features=production,embedded

Patch: CLJ-1424-2.diff

Questions: Should *suppress-read* override *read-eval*?

Related: CLJS-27, TRDR-14



 Comments   
Comment by Jozef Wagner [ 16/May/14 2:19 AM ]

Has there been a decision that CL syntax is going to be used? Related discussion can be found at design page, google groups discussion and another discussion.

Comment by Alex Miller [ 16/May/14 8:34 AM ]

No, no decisions on anything yet.

Comment by Ghadi Shayban [ 19/May/14 7:25 PM ]

Just to echo a comment from TRDR-14:

This is WIP and just one approach for feature expressions. There seem to be at least two couple diverging approaches emerging from the various discussion (Brandon Bloom's idea of read-time splicing being the other.)

In any case having all Clojure platforms be ready for the change is probably essential. Also backwards compatibility of feature expr code to Clojure 1.6 and below is also not trivial.

Comment by Kevin Downey [ 04/Aug/14 1:39 PM ]

if you have ever tried to do tooling for a language where the "parser" tossed out information or did some partial evaluation, it is a pain. this is basically what the #+cljs style feature expressions and bbloom's read time splicing both do with clojure's reader. I think resolving this at read time instead of having the compiler do it before macro expansion is a huge mistake and makes the reader much less useful for reading code.

Comment by Ghadi Shayban [ 04/Aug/14 2:00 PM ]

Kevin, what kind of tooling use case are you alluding to?

Comment by Kevin Downey [ 04/Aug/14 3:24 PM ]

any use case that involves reading code and not immediately handing it off to the compiler. if I wanted to write a little snippet to read in a function, add an unused argument to every arity then pprint it back, reader resolved feature expressions would not round trip.

if I want to write snippet of code to generate all the methods for a deftype (not a macro, just at the repl write a `for` expression) I can generate a clojure data structure, call pprint on it, then paste it in as code, reader feature expressions don't have a representation as data so I cannot do that, I would have to generate strings directly.





[CLJ-1491] External type hint inconsistency between regular functions and primitive functions Created: 05/Aug/14  Updated: 05/Aug/14

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

Type: Defect Priority: Minor
Reporter: Gunnar Völkel Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, typehints

Attachments: Text File 0001-preserve-fn-meta-on-invokePrim.patch    
Patch: Code
Approval: Triaged

 Description   

Consider the following example.

(set! *warn-on-reflection* true)

(defn f [n] (java.util.ArrayList. (int n)))

(let [al ^java.util.ArrayList (f 10)]
  (.add al 23))

As expected this does not warn about reflection. The following example shows the same scenario for a primitive function.

(set! *warn-on-reflection* true)

(defn g [^long n] (java.util.ArrayList. n))

(let [al ^java.util.ArrayList (g 10)]
  (.add al 23))
; Reflection warning, NO_SOURCE_PATH:2:3 - call to method add on java.lang.Object can't be resolved (no such method).

So the behavior of external type hints is inconsistent for regular functions and primitive functions.
Most likely, the external type hint information is somehow ignored for primitive functions since the case where they return no primitive value is not treated separately.



 Comments   
Comment by Nicola Mometto [ 05/Aug/14 4:32 AM ]

The following patch preserves the original metadata of the invoke form on the transformed .invokePrim expression

Comment by Alex Miller [ 05/Aug/14 7:40 AM ]

Not challenging the premise at all but workaround:

(let [^java.util.ArrayList al (g 10)]
  (.add al 23))
Comment by Gunnar Völkel [ 05/Aug/14 8:09 AM ]

Well, the example above was already changed such that you can also place the type hint on the binding to check whether that works.
The actual problem arose when using the return value of the function exactly once without an additional binding.

Comment by Jozef Wagner [ 05/Aug/14 10:48 AM ]

Responding to Alex's comment, is there a consensus on which variant is (more) idiomatic? IMHO latter variant seems to be more reliable (as this issue shows, and for primitive hits too), and is consistent with 'place hint on a symbol' idiom which is applied when type hinting vars or fn args.

(let [symbol ^typehint expr] body)
(let [^typehint symbol expr] body)
Comment by Alex Miller [ 05/Aug/14 4:59 PM ]

They have different meanings. Generally the latter covers some cases that the former does not so it's probably the better one. I believe one of the cases is that if expr is a macro, the typehint is lost in the former.





[CLJ-1472] The locking macro fails bytecode verification on ART runtime Created: 23/Jul/14  Updated: 04/Aug/14

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

Type: Defect Priority: Major
Reporter: Adam Clements Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None
Environment:

Android ART runtime


Attachments: Text File 0001-CLJ-1472-Locking-macro-fails-bytecode-verification.patch    
Patch: Code
Approval: Triaged

 Description   

Android ART runs compile time verification on bytecode and was failing on any usage of the locking macro. Examination of the bytecode as compared to a java synchronized block shows up a number of differences:
https://gist.github.com/AdamClements/2ae6c4919964b71eb470

Having the monitor-enter inside the try block seems wrong to me, as surely if the lock fails to be acquired, it shouldn't be released with monitor-exit. Moving the monitor enter outside the try block seems to have resolved the issue and android no longer complains about usages of locking and all clojure tests still pass.

Java's generated code goes further and catches any exceptions generated by the monitor-exit itself and retries indefinitely (I believe the logic is that then at least your deadlock is in the right place, and not next time something else attempts to acquire a lock on the same object). I don't think that this can be replicated in clojure without getting down to the bytecode emitting level though and it doesn't seem to be an issue for the ART verifier.



 Comments   
Comment by Adam Clements [ 24/Jul/14 11:17 AM ]

After using this a little more, I've found that moving this outside the try block breaks nREPL.

Looking at the bytecode, the monitorenter for the locking in clojure.tools.nrepl.middleware.session/session-out and in a few other places ends up in an entirely different method definition and we now get a JVM IllegalMonitorStateException as well as an ART verification error for this function.

Comment by Andy Fingerhut [ 01/Aug/14 9:08 PM ]

Adam, I cannot comment on whether your patch is of interest or not, but it is true that no patch will be committed to Clojure if the author has not signed a Contributor Agreement, which can now be done on-line at http://clojure.org/contributing

Comment by Adam Clements [ 04/Aug/14 4:24 PM ]

Uploaded a new patch (and signed the contributor agreement). This passes both the JVM and ART bytecode verification, The extra try/catch around the monitor exit is optional (verification passes with or without it) but where the java version retries monitor-exit indefinitely and shows the deadlock at the right time, without catching errors in the monitor-exit an undetermined monitor-enter in the future might fail, not showing up the actual bug.

It's not very pretty, but without finer grained control of the generated bytecode, this is the best I could do.





[CLJ-803] IAtom interface Created: 27/May/11  Updated: 03/Aug/14

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

Type: Enhancement Priority: Minor
Reporter: Pepijn de Vos Assignee: Aaron Bedra
Resolution: Unresolved Votes: 4
Labels: None

Attachments: Text File 0001-atom-interface.patch     Text File 0001-CLJ-803-IAtom-interface-static-Atom-swap.patch     Text File iatom.patch    
Patch: Code

 Description   

Atom and the other reference types do not have interfaces and are marked final.

Use cases for interfaces for the reference types include database wrappers. CouchDB behaves exactly like compare-and-set! and is shared, synchronous, independent state, so it makes sense to use the Atom interface to update a CouchDB document.

I talked to Rich about this, and he said "patch welcome for IAtom", complete conversation: http://clojure-log.n01se.net/date/2010-12-29.html#10:04c



 Comments   
Comment by Stuart Halloway [ 27/May/11 2:33 PM ]

Please add a patch formatted by "git format-patch" so that attribution is included.

Comment by Pepijn de Vos [ 04/Jun/11 5:56 AM ]

I added the formatted patch a few days ago. Does 'no news is good news' apply here?

And, silly question, will it make it into 1.3? I can't figure out how to tell Jira to show me that.

Comment by Kevin Downey [ 04/Jul/11 9:06 PM ]

I fail to see the need for an IAtom, if you want something atom like for couchdb the interfaces are already there. Maybe I ICompareAndSwap. Atoms and couchdb are different so making them appear the same is a bad idea.

http://en.wikipedia.org/wiki/Fallacies_of_Distributed_Computing

http://clojure.org/state one of the distinctions between agents and actors raised in the section titled "Message Passing and Actors" is local vs. distributed and the same distinction can be made between couchdb (regardless of compare and swap) and atoms

Comment by Aaron Bedra [ 04/Jul/11 9:18 PM ]

This ticket has already been moved into approved backlog. It will be revisited again after the 1.3 release where we will take a closer look at things. For now, this will remain as is.

Comment by Aaron Craelius [ 10/Jul/14 12:15 PM ]

Any chance this patch could get implemented in an upcoming Clojure release. There is still continued interest, see this thread: https://groups.google.com/forum/#!topic/clojure-dev/y5QoMqd44Lc

One suggestion I would make is also removing the final marker from clojure.lang.Atom - I can see use cases where one would want to directly subclass Atom (to capture dependencies in reactive computations for instance).

Comment by Brandon Bloom [ 02/Aug/14 2:14 PM ]

I'd like to see an IAtom interface, but would prefer that `swap` not be part of it. Swapping can, and should, be defined in terms of `compareAndSet`. Seems like IAtom should only have `boolean compareAndSet(object oldval, object newval)` as well as `void reset(object newval)`.

Comment by Brandon Bloom [ 02/Aug/14 2:29 PM ]

Alternative patch that introduces IAtom and converts swap to be static.

Comment by Pepijn de Vos [ 03/Aug/14 2:59 AM ]

At the time I did the initial patch, I had the same idea to remove swap, but Rich said there where cases for having it, so it should stay in according to him.

Comment by Aaron Craelius [ 03/Aug/14 1:51 PM ]

One use case I can think of for overriding swap is if an IAtom is wrapping say a row of data stored in a database. Then comparing something like a version column (or transaction id in the case of datomic) is what should determine whether a swap is retried, not the actual value of the data. In this case then, compareAndSet would actually be a more complex operation than swap and it makes sense to define the two independently.

Comment by Aaron Craelius [ 03/Aug/14 1:56 PM ]

I should also mention my related issue: http://dev.clojure.org/jira/browse/CLJ-1470 which simply allows Atom (and also ARef) to be sub-classed. Both patches could ultimately work together to make the whole Atom/ARef infrastructure easier to extend.





[CLJ-1466] clojure.core/bean should implement Iterable Created: 16/Jul/14  Updated: 03/Aug/14

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

Type: Defect Priority: Minor
Reporter: Ambrose Bonnaire-Sergeant Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: File iterable-bean-v2.diff    
Patch: Code and Test
Approval: Triaged

 Description   

The changes in Clojure 1.6 hashing revealed that `bean` does not return a map that implements Iterable:

user=> (hash (bean (java.util.Date.)))

AbstractMethodError clojure.lang.APersistentMap.iterator()Ljava/util/Iterator;  clojure.core.proxy$clojure.lang.APersistentMap$ff19274a.iterator (:-1)

Patch adds `iterator` method to clojure.core/bean.



 Comments   
Comment by Alex Miller [ 16/Jul/14 10:22 AM ]

One workaround:

(hash (apply hash-map (bean (java.util.Date.))))

Interestingly, into does not help b/c into uses reduce, which internally uses the iterator too.

Comment by Alex Miller [ 16/Jul/14 11:01 AM ]

APersistentMap implements Iterable and expects subclasses to fulfill that contract. The bean proxy does not. Instead of changing APersistentMap, why not add:

(iterator [] (.iterator pmap)

to the bean proxy definition?

Comment by Ambrose Bonnaire-Sergeant [ 16/Jul/14 11:19 AM ]

It seemed like an oversight that APersistentMap lacked a default iterator method.

That said, I haven't used OO inheritance for 4 years. Should I change the patch?

Comment by Ambrose Bonnaire-Sergeant [ 16/Jul/14 11:47 AM ]

Added new patch that just adds iterator to bean.





[CLJ-1485] clojure.test.junit/with-junit-output doesn't handle multiple expressions Created: 29/Jul/14  Updated: 03/Aug/14

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

Type: Defect Priority: Minor
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: clojure.test

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

 Description   
(defmacro with-junit-output
  "Execute body with modified test-is reporting functions that write
  JUnit-compatible XML output."
  {:added "1.1"}
  [& body]
  `(binding [t/report junit-report
             *var-context* (list)
             *depth* 1]
     (t/with-test-out
       (println "<?xml version=\"1.0\" encoding=\"UTF-8\"?>")
       (println "<testsuites>"))
     (let [result# ~@body]
       (t/with-test-out (println "</testsuites>"))
       result#)))

From this description, and the use of ~@body, it's clear that the intent was to support a body containing multiple forms (for side-effects). However, the use inside the let, and with no supplied do, means that you must supply a single form, or be confrunted with an inscrutable compilation error about "clojure.core/let requires an even number of forms in binding vector" that's not obviously your fault, or easy to track down.



 Comments   
Comment by Howard Lewis Ship [ 29/Jul/14 4:59 PM ]

Patch for issue





[CLJ-1324] Allow leading slashes in unqualified symbol names Created: 15/Jan/14  Updated: 02/Aug/14

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

Type: Enhancement Priority: Minor
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reader
Environment:

All


Attachments: Text File clj-1324-1.patch    
Patch: Code and Test

 Description   

The proposal is to allow the reader to accept symbol names with leading slashes.

Problem: Leading slashes are frequently useful, e.g. in mathematical operators like "/" or "/="

Currently, only "/" is allowed as a special case, and is used for the division operator in clojure.core

This could be extended to allow all symbols to have names starting with a leading slash.

There should be no ambiguity with namespace-qualified symbols:
1) In the case of a leading slash, a symbol should be interpreted as an unqualified symbol e.g. "/="
2) In the case of a slash anywhere except in leading position, it should considered as namespace qualified, e.g. "clojure.core/+"
3) In the case of multiple non-leading slashes, the first slash is the namespace separator, e.g. "clojure.core.matrix.operators//="

Optionally, it also would be possible to allow multiple slashes after the leading slash in a name. This would allow symbols such as "/src/main/clojure" to become valid.



 Comments   
Comment by Paavo Parkkinen [ 10/Feb/14 7:32 AM ]

Attached patch to allow leading slashes in symbol names.

The patch changes the regexp pattern used to match symbols to accept characters after a slash in symbol names.

Tests pass, and the patch also adds a couple of new special cases to the symbol tests.

Comment by Andy Fingerhut [ 01/Aug/14 9:26 PM ]

Paavo, a commit made to Clojure master earlier today causes your patch clj-1324-1.patch to no longer apply cleanly. I haven't investigated in detail, but it might be straightforward to update the patch so that it applies cleanly again.

Comment by Paavo Parkkinen [ 02/Aug/14 7:37 PM ]

Attached updated patch.

Comment by Andy Fingerhut [ 02/Aug/14 8:40 PM ]

It would be less confusing if you could name the patches differently, or remove the older one.





[CLJ-1408] Add transient keyword to cached toString() value in _str Created: 19/Apr/14  Updated: 01/Aug/14

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

Type: Defect Priority: Minor
Reporter: Tomasz Nurkiewicz Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop

Attachments: Text File CLJ-1408-2.patch     Text File CLJ-1408.patch    
Patch: Code
Approval: Incomplete

 Description   

_str field in Keyword and Symbol classes lazily caches result of toString(). Because this field is not transient, serializing (using Java serialization) any keyword before and after calling toString() for the first time yields different results:

(import (java.io ByteArrayOutputStream ObjectOutputStream
                 ByteArrayInputStream  ObjectInputStream))

(defn- serialize [obj]
  (with-open [bos (ByteArrayOutputStream.)
              stream (ObjectOutputStream. bos)]
    (.writeObject stream obj)
    (-> bos .toByteArray seq)))

(def s1 (serialize :k))
;(println :k)
(def s2 (serialize :k))
(println (= s1 s2))

Uncomment (println :k) and suddenly s1 and s2 are no longer equal.

This issue came up when I was trying to use keywords as key in [Hazelcast](https://github.com/hazelcast/hazelcast) map. Hazelcast uses serialized keys in various scenarios, thus if I first put something to map under key :k and then print :k, I can no longer find such key.

Patch: CLJ-1408-2.patch

Screened by:



 Comments   
Comment by Alex Miller [ 19/Apr/14 7:28 AM ]

Hi Tomasz, it would be good to fix this, can you sign the CLA?

Comment by Tomasz Nurkiewicz [ 20/Apr/14 7:26 AM ]

Thanks, I'll sign and send CLA ASAP.

Comment by Tomasz Nurkiewicz [ 08/May/14 4:10 PM ]

My contributor greement arrived, please merge this patch whenever you find suitable.

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

Hi Tomasz, I noticed you added the private keyword - please remove that and update the patch.

Comment by Tomasz Nurkiewicz [ 09/May/14 3:55 PM ]

Removed `private` keyword

Comment by Alex Miller [ 20/Jun/14 9:22 AM ]

On second thought, it looks like we have most of the infrastructure for serialization testing anyways, so would appreciate an updated patch with the example turned into a serialization test. Please see test/clojure/test_clojure/serialization.clj for a place to put this (using existing roundtrip function).

Comment by Andy Fingerhut [ 01/Aug/14 9:29 PM ]

Tomasz, in addition to Alex's previous comment, it appears that a commit made to Clojure master earlier today causes your patches to no longer apply cleanly. I haven't looked to see whether updating the patches would be easy, but likely it is.





[CLJ-1449] Add starts-with? and ends-with? to clojure.string Created: 19/Jun/14  Updated: 01/Aug/14

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

Type: Enhancement Priority: Minor
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: string

Approval: Triaged

 Description   

Add clojure.string/starts-with? and ends-with? similar to java.lang.String's startsWith/endsWith. In addition to making these easier to find and use, this provides a place to add a portable ClojureScript variant.



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

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

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

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

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

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

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

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

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

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

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

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

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

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





[CLJ-124] GC Issue 120: Determine mechanism for controlling automatic shutdown of Agents, with a default policy and mechanism for changing that policy as needed Created: 17/Jun/09  Updated: 31/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Chas Emerick Assignee: Alex Miller
Resolution: Unresolved Votes: 4
Labels: agents

Attachments: Text File clj-124-v1.patch    
Patch: Code
Approval: Vetted
Waiting On: Rich Hickey

 Description   

The original description when this ticket was vetted is below, starting with "Reported by cemer...@snowtide.com, June 01, 2009". This prefix attempts to summarize the issue and discussion.

Description:

Several Clojure functions involving agents and futures, such as future, pmap, clojure.java.shell/sh, and a few others, create non-daemon threads in the JVM. These can cause a 60-second wait after a Clojure program is done before the process exits. Questions about this confusing behavior come up a couple of times per year on the Clojure Google group. Search for shutdown-agents to find most of these occurrences, since calling (shutdown-agents) at the end of one's program typically eliminates this 60-second wait.

Example:

% java -cp clojure.jar clojure.main -e "(println 1)"
1
[ this case exits quickly ]

% java -cp clojure.jar clojure.main -e "(println @(future 1))"
1
[ 60-second pause before process exits, at least on many platforms and JVMs ]

Summary of comments before July 2014:

Most of the comments on this ticket on or before August 23, 2010 were likely spread out in time before being imported from the older ticket tracking system into JIRA. Most of them refer to an older suggested patch that is not in JIRA, and compilation problems it had with JDK 1.5, which is no longer supported by Clojure 1.6.0. I think these comments can be safely ignored now.

Alex Miller blogged about this and related issues here: http://tech.puredanger.com/2010/06/08/clojure-agent-thread-pools/

Since then, two of the suggestions Alex raised have been addressed. One by CLJ-378 and one by the addition of set-agent-send-executor! and similar functions to Clojure 1.5.0: https://github.com/clojure/clojure/blob/master/changes.md#23-clojurecoreset-agent-send-executor-set-agent-send-off-executor-and-send-via

One remaining issue is the topic of this ticket, which is how best to avoid this 60-second pause.

One method is mentioned in Chas Emerick's original description below, suggested by Rich Hickey, but perhaps long enough ago he may no longer endorse it: Create a Var *auto-shutdown-agents* that when true (the default value), clojure.lang.Agent shutdown() is called after the clojure.main entry point. This removes the surprising wait for common methods of starting Clojure, while allowing expert users to change that value to false if desired.

Another method mentioned in the discussion is to change the threads created in agent thread pools to daemon threads by default, and perhaps to deprecate shutdown-agents or modify it to be less dangerous. That approach is discussed a bit more in Alex's blog post linked above, and in a comment from Alexander Taggart on July 11, 2011 below.

The only other comment before 2014 that is not elaborated in this summary is shoover's suggestion of an alternate approach dated Aug 23 2010.

Patch: clj-124-v1.patch

See the Jul 31 2014 comment when this patch was attached to the ticket for some description of its approach, which is an attempt to implement the *auto-shutdown-agents* method mentioned above.

Reported by cemer...@snowtide.com, Jun 01, 2009

There has been intermittent chatter over the past months from a couple of
people on the group (e.g.
http://groups.google.com/group/clojure/browse_thread/thread/409054e3542adc1f)
and in #clojure about some clojure scripts hanging, either for a constant
time (usually reported as a minute or so with no CPU util) or seemingly
forever (or until someone kills the process).

I just hit a similar situation in our compilation process, which invokes
clojure.lang.Compile from ant.  The build process for this particular
project had taken 15 second or so, but after adding a couple of pmap calls,
that build time jumped to ~1:15, with roughly zero CPU utilization over the
course of that last minute.

Adding a call to Agent.shutdown() in the finally block in
clojure.lang.Compile/main resolved the problem; a patch including this
change is attached.  I wouldn't suspect anyone would have any issues with
such a change.

-----
In general, it doesn't seem like everyone should keep tripping over this
problem in different directions.  It's a very difficult thing to debug if
you're not attuned to how clojure's concurrency primitives work under the
hood, and I would bet that newer users would be particularly affected.

After discussion in #clojure, rhickey suggested adding a
*auto-shutdown-agents* var, which:

- if true when exiting one of the main entry points (clojure.main, or the
legacy script/repl entry points), Agent.shutdown() would be called,
allowing for the clean exit of the application

- would be bound by default to true

- could be easily set to false for anyone with an advanced use-case that
requires agents to remain active after the main thread of the application
exits.

This would obviously not help anyone initializing clojure from a different
entry point, but this may represent the best compromise between
least-surprise and maximal functionality for advanced users.

------

In addition to the above, it perhaps might be worthwhile to change the
keepalive values used to create the Threadpools used by c.l.Actor's
Executors.  Currently, Actor uses a default thread pool executor, which
results in a 60s keepalive.  Lowering this to something much smaller (1s?
5s?) would additionally minimize the impact of Agent's threadpools on Java
applications that embed clojure directly (and would therefore not benefit
from *auto-shutdown-agents* as currently conceived, leading to puzzling
'hanging' behaviour).  I'm not in a position to determine what impact this
would have on performance due to thread churn, but it would at least
minimize what would be perceived as undesirable behaviour by users that are
less familiar with the implementation details of Agent and code that
depends on it.

Comment 1  by cemer...@snowtide.com, Jun 01, 2009

Just FYI, I'd be happy to provide patches for either of the suggestions mentioned
above...


 Comments   
Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/124
Attachments:
compile-agent-shutdown.patch - https://www.assembla.com/spaces/clojure/documents/a56S2ow4ur3O2PeJe5afGb/download/a56S2ow4ur3O2PeJe5afGb
124-compilation.diff - https://www.assembla.com/spaces/clojure/documents/aqn0IGxZSr3RUGeJe5aVNr/download/aqn0IGxZSr3RUGeJe5aVNr

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

oranenj said: [file:a56S2ow4ur3O2PeJe5afGb]

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

richhickey said: Updating tickets (#8, #19, #30, #31, #126, #17, #42, #47, #50, #61, #64, #69, #71, #77, #79, #84, #87, #89, #96, #99, #103, #107, #112, #113, #114, #115, #118, #119, #121, #122, #124)

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

cemerick said: (In [[r:fa3d24973fc415b35ae6ec8d84b61ace76bd4133]]) Add a call to Agent.shutdown() at the end of clojure.lang.Compile/main Refs #124

Signed-off-by: Chouser <chouser@n01se.net>

Branch: master

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

chouser@n01se.net said: I'm closing this ticket to because the attached patch solves a specific problem. I agree that the idea of an auto-shutdown-agents var sounds like a positive compromise. If Rich wants a ticket to track that issue, I think it'd be best to open a new ticket (and perhaps mention this one there) rather than use this ticket to track further changes.

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

scgilardi said: With both Java 5 and Java 6 on Mac OS X 10.5 Leopard I'm getting an error when compiling with this change present.

Java 1.5.0_19
Java 1.6.0_13

For example, when building clojure using "ant" from within my clone of the clojure repo:

[java] java.security.AccessControlException: access denied (java.lang.RuntimePermission modifyThread)
[java] at java.security.AccessControlContext.checkPermission(AccessControlContext.java:264)
[java] at java.security.AccessController.checkPermission(AccessController.java:427)
[java] at java.util.concurrent.ThreadPoolExecutor.shutdown(ThreadPoolExecutor.java:894)
[java] at clojure.lang.Agent.shutdown(Agent.java:34)
[java] at clojure.lang.Compile.main(Compile.java:71)

I reproduced this on two Mac OS X 10.5 machines. I'm not aware of having any enhanced security policies along these lines on my machines. The compile goes fine for me with Java 1.6.0_0 on an Ubuntu box.

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

chouser@n01se.net said: I had only tested it on my ubuntu box – looks like that was openjdk 1.6.0_0. I'll test again with sun-java5 and sun-java6.

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

chouser@n01se.net said: 1.6.0_13 worked fine for me on ubuntu, but 1.5.0_18 generated an the exception Steve pasted. Any suggestions? Should this patch be backed out until someone has a fix?

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

achimpassen said: [file:aqn0IGxZSr3RUGeJe5aVNr]

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

chouser@n01se.net said: With Achim's patch, clojure compiles for me on ubuntu using java 1.5.0_18 from sun, and still works on 1.6.0_13 sun and 1.6.0_0 openjdk. I don't know anything about ant or the security error, but this is looking good to me.

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

achimpassen said: It works for me on 1.6.0_13 and 1.5.0_19 (32 and 64 bit) on OS X 10.5.7.

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

chouser@n01se.net said: (In [[r:895b39dabc17b3fd766fdbac3b0757edb0d4b60d]]) Rev fa3d2497 causes compile to fail on some VMs – back it out. Refs #124

Branch: master

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

mikehinchey said: I got the same compile error on both 1.5.0_11 and 1.6.0_14 on Windows. Achim's patch fixes both.

See the note for "permissions" on http://ant.apache.org/manual/CoreTasks/java.html . I assume ThreadPoolExecutor.shutdown is the problem, it would shutdown the main Ant thread, so Ant disallows that. Forking avoids the permissions limitation.

In addition, since the build error still resulted in "BUILD SUCCESSFUL", I think failonerror="true" should also be added to the java call so the build would totally fail for such an error.

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

chouser@n01se.net said: I don't know if the <java fork=true> patch is a good idea or not, or if there's a better way to solve the original problem.

Chas, I'm kicking back to you, but I guess if you don't want it you can reassign to "nobody".

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

richhickey said: Updating tickets (#8, #42, #113, #2, #20, #94, #96, #104, #119, #124, #127, #149, #162)

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

shoover said: I'd like to suggest an alternate approach. There are already well-defined and intuitive ways to block on agents and futures. Why not deprecate shutdown-agents and force users to call await and deref if they really want to block? In the pmap situation one would have to evaluate the pmap form.

The System.exit problem goes away if you configure the threadpools to use daemon threads (call new ThreadPoolExecutor and pass a thread factory that creates threads and sets daemon to true). That way the user has an explicit means of blocking and System.exit won't hang.

Comment by Assembla Importer [ 24/Aug/10 12:45 AM ]

alexdmiller said: I blogged about these issues at:
http://tech.puredanger.com/2010/06/08/clojure-agent-thread-pools/

I think that:

  • agent thread pool threads should be named (see ticket #378)
  • agent thread pools must be daemon threads by default
  • having ways to specify an customized executor pool for an agent send/send-off is essential to customize threading behavior
  • (shutdown-agents) should be either deprecated or made less dangerous
Comment by Alexander Taggart [ 11/Jul/11 9:33 PM ]

Rich, what is the intention behind using non-daemon threads in the agent pools?

If it is because daemon threads could terminate before their work is complete, would it be acceptable to add a shutdown hook to ensure against such premature termination? Such a shutdown hook could call Agent.shutdown(), then awaitTermination() on the pools.

Comment by Christopher Redinger [ 27/Nov/12 3:47 PM ]

Moving this ticket out of approval "OK" status, and dropping the priority. These were Assembla import defaults.

Also, Chas gets to be the Reporter now.

Comment by Chas Emerick [ 27/Nov/12 5:56 PM ]

Heh, blast from the past.

The comment import appears to have set their timestamps to the date of the import, so the conversation is pretty hard to follow, and obviously doesn't benefit from the intervening years of experience. In addition, there have been plenty of changes to agents, including some recent enhancements that address some of the pain points that Alex Miller mentioned above.

I propose closing this as 'invalid' or whatever, and opening one or more new issues to track whatever issues still persist (presumably based on fresh ML discussion, etc).

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

Rereading the original description of this ticket, without reading all of the comments that follow, that description is still right on target for the behavior of latest Clojure master today.

People send messages to the Clojure Google group every couple of months hitting this issue, and one even filed CLJ-959 because of hitting it. I have updated the examples on ClojureDocs.org for future, and also for pmap and clojure.java.shell/sh which use future in their implementations, to warn people about this and explain that they should call (shutdown-agents), but making it unnecessary to call shutdown-agents would be even better, at least as the default behavior. It sounds fine to me to provide a way for experts on thread behavior to change that default behavior if they need to.

Comment by Andy Fingerhut [ 31/Jul/14 6:39 PM ]

Patch clj-124-v1.patch dated Jul 31 2014 implements the approach of calling clojure.lang.Agent#shutdown when the new Var *auto-shutdown-agents* is true, which is its default value.

I don't see any benefit to making this Var dynamic. Unless I am missing something, only the root binding value is visible after clojure.main/main returns, not any binding that would be pushed on top of that if it were dynamic. It seems to require alter-var-root to change it to false in a way that this patch would avoid calling clojure.lang.Agent#shutdown.

This patch only adds the shutdown call to clojure.main#main, but can easily be added to the legacy_repl and legacy_script methods if desired.





[CLJ-1486] Make fnil var-arg Created: 31/Jul/14  Updated: 31/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-make-fnil-vararg.patch    
Patch: Code and Test

 Description   

Currently fnil is defined only for 1 to 3 args, this patch makes it var-arg






[CLJ-1400] Error "Can't refer to qualified var that doesn't exist" should name the bad symbol Created: 09/Apr/14  Updated: 29/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Howard Lewis Ship Assignee: Scott Bale
Resolution: Unresolved Votes: 0
Labels: Compiler, errormsgs
Environment:

OS X


Attachments: File clj-1400-1.diff    
Patch: Code and Test
Approval: Vetted

 Description   

Def of var with a ns that doesn't exist will yield this error:

user> (def foo/bar 1)
CompilerException java.lang.RuntimeException: Can't refer to qualified var that doesn't exist, compiling:(NO_SOURCE_PATH:1:1)

Cause: Compiler.lookupVar() returns null if the ns in a qualified var does not exist yet.

Proposed: The error message would be improved by naming the symbol and throwing a CompilerException with file/line/col info. It's not obvious, but this may be the only case where this error occurs. If so, the error message could be more specific that the ns is the part that doesn't exist.

Patch:

Screened by:



 Comments   
Comment by Scott Bale [ 25/Jun/14 9:58 AM ]

This looks to me like relatively low hanging fruit unless I'm missing something; assigning to myself.

Comment by Scott Bale [ 26/Jun/14 11:23 PM ]

Patch clj-1400-1.diff to Compiler.java.

With this patch the example would now look like:

user> (def foo/bar 1)
CompilerException java.lang.RuntimeException: Qualified symbol foo/bar refers to nonexistent namespace: foo, compiling:(NO_SOURCE_PATH:1:1)

I'm not sure the if(namesStaticMember(sym)) [see below], and the 2nd branch, is even necessary. Just by inspection I suspect it is not.

[footnote]

public static boolean namesStaticMember(Symbol sym){
	return sym.ns != null && namespaceFor(sym) == null;
}
Comment by Scott Bale [ 26/Jun/14 11:24 PM ]

patch: code and test

Comment by Scott Bale [ 26/Jun/14 11:27 PM ]

I tested on an actual source file, and the exception message included the file/line/col info as desired:

user=> CompilerException java.lang.RuntimeException: Qualified symbol goo/bar refers to nonexistent namespace: goo, compiling:(/home/scott/dev/foo.clj:3:1)




[CLJ-1477] Fixed a typo Created: 27/Jul/14  Updated: 29/Jul/14

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

Type: Enhancement Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: documentation

Attachments: Text File 0001-Fix-a-typo.patch    
Patch: Code
Approval: Triaged

 Description   

Just a simple typo fix - "directy" -> "directly".






[CLJ-1479] Typo in filterv example Created: 27/Jul/14  Updated: 29/Jul/14

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

Type: Enhancement Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: documentation

Attachments: Text File 0001-Fix-a-typo.patch    
Patch: Code
Approval: Triaged

 Description   

filter -> filterv






[CLJ-1478] Doc typo Created: 27/Jul/14  Updated: 29/Jul/14

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

Type: Enhancement Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Attachments: Text File 0001-Fix-a-typo.patch    
Patch: Code
Approval: Triaged

 Description   

Another small typo fix.






[CLJ-1480] Incorrect param name reference in defmulti's docstring Created: 27/Jul/14  Updated: 29/Jul/14

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

Type: Defect Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Attachments: Text File 0001-Fix-param-name-reference-in-defmulti-s-docstring.patch    
Patch: Code
Approval: Triaged

 Description   

attribute-map should actually be attr-map






[CLJ-1481] Typo in type-reflect's docstring Created: 27/Jul/14  Updated: 29/Jul/14

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

Type: Defect Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Attachments: Text File 0001-Fix-a-typo.patch    
Patch: Code
Approval: Triaged

 Description   

membrer -> member






[CLJ-1483] Clarify the usage of replace(-first) with a function Created: 27/Jul/14  Updated: 29/Jul/14

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

Type: Enhancement Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, string

Attachments: Text File 0001-Clarify-the-usage-of-replace-first-with-pattern-func.patch    
Patch: Code
Approval: Triaged

 Description   

The documentation of replace and replace-first didn't feature any example usage of the pattern + function combo so I've added one.






[CLJ-1475] :post condition causes compiler error with recur Created: 25/Jul/14  Updated: 29/Jul/14

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

Type: Defect Priority: Minor
Reporter: Steve Miner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler

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

 Description   

Michael O'Keefe <michael.p.okeefe@gmail.com> posted on the mailing list an example of code that causes a compiler error only if a :post condition is added. Here's my slightly modified version:

(defn g
  [xs acc]
  {:pre [(or (nil? xs) (sequential? xs))]
   :post [(number? %)]}
  (if (seq xs)
     (recur (next xs) (+ (first xs) acc))
     acc))

CompilerException java.lang.UnsupportedOperationException: Can only recur from tail position

The work-around is to wrap the body in a loop that simply rebinds the original args.



 Comments   
Comment by Steve Miner [ 25/Jul/14 9:53 AM ]

A macro expansion shows that body is placed in a let form to capture the result for later testing with the post condition, but the recur no longer has a proper target. The work-around of using a loop form is easy once you understand what's happening but it's a surprising limitation.

Comment by Steve Miner [ 25/Jul/14 9:55 AM ]

Use a local fn* around the body and call it with the original args so that the recur has a proper target. Update: not good enough for handling destructuring. Patch withdrawn.

Comment by Michael Patrick O'Keefe [ 25/Jul/14 10:37 AM ]

Link to the original topic discussion: https://groups.google.com/d/topic/clojure/Wb1Nub6wVUw/discussion

Comment by Steve Miner [ 25/Jul/14 1:42 PM ]

Patch withdrawn because it breaks on destructured args.

Comment by Steve Miner [ 25/Jul/14 5:27 PM ]

While working on a patch, I came up against a related issue: Should the :pre conditions apply to every recur "call". Originally, I thought the :pre conditions should be checked just once on the initial function call and never during a recur. People on the mailing list pointed out that the recur is semantically like calling the function again so the :pre checks are part of the contract. But no one seemed to want the :post check on every recursion, so the :post would happen only at the end.

That means automatically wrapping a loop (or nested fn* call) around the body is not going to work for the :pre conditions. A fix would have to bring the :pre conditions inside the loop.

Comment by Steve Miner [ 26/Jul/14 8:54 AM ]

I'm giving up on this bug. My approach was adding too much complexity to handle an edge case. I recommend the "loop" work-around to anyone who runs into this problem.

(defn g2
  [xs acc]
  {:pre [(or (nil? xs) (sequential? xs))]
   :post [(number? %)]}
  (loop [xs xs acc acc]
    (if (seq xs)
       (recur (next xs) (+ (first xs) acc))
       acc)))
Comment by Ambrose Bonnaire-Sergeant [ 26/Jul/14 10:29 AM ]

Add patch that handles rest arguments and destructuring.

Comment by Michael Patrick O'Keefe [ 26/Jul/14 10:57 AM ]

With regard to Steve's question on interpreting :pre, to me I would expect g to act like the case g3 below which uses explicit recursion (which does work and does appear to check the :pre conditions each time and :post condition once):

(defn g3
  [xs acc]
  {:pre [(or (sequential? xs) (nil? xs)) (number? acc)]
   :post [(number? %)]}
  (if (seq xs)
    (g3 (next xs) (+ (first xs) acc))
    acc))
Comment by Ambrose Bonnaire-Sergeant [ 26/Jul/14 11:42 AM ]

Patch clj-1475.diff handles destructuring, preconditions and rest arguments

Comment by Steve Miner [ 26/Jul/14 4:04 PM ]

The clj-1475.diff patch looks good to me.

Comment by Alex Miller [ 27/Jul/14 7:18 AM ]

Please don't use "patch" as a label - that is the purpose of the Patch field. There is a list of good and bad labels at http://dev.clojure.org/display/community/Creating+Tickets

Comment by Steve Miner [ 27/Jul/14 11:32 AM ]

More knowledgeable commenters might take a look at CLJ-701 just in case that's applicable to the proposed patch.

Comment by Kevin Downey [ 29/Jul/14 1:35 AM ]

re clj-701

it is tricky to express loop expression semantics in jvm byte code, so the compiler sort of punts, hoisting expression loops in to anonymous functions that are immediately invoked, closing over whatever is in scope that is required by the loop, this has some problems like those seen in CLJ-701, losing type data which the clojure compiler doesn't track across functions, the additional allocation of function objects (the jit may deal with that pretty well, I am not sure) etc.

where the world of clj-701 and this ticket collide is the patch on this ticket lifts the function body out as a loop expression, which without the patch in clj-701 will have the issues I listed above, but we already have those issues anywhere something that is difficult to express in bytecode as an expression (try and loop) is used as an expression, maybe it doesn't matter, or maybe clj-701 will get fixed in some way to alleviate those issues.

general musings

it seems like one feature people like from asserts is the ability to disable them in production (I have never actually seen someone do that with clojure), assert and :pre/:post have some ability to do that (it may only work at macroexpansion time, I don't recall) since the hoisting of the loop could impact performance it might be nice to have some mechanism to disable it (maybe using the same flag assert does?).





[CLJ-1422] Recur around try boxes primitives Created: 14/May/14  Updated: 28/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Kyle Kingsbury Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, performance, typehints


 Description   

Primitive function and recur variables can't pass through a (try) cleanly; they're boxed to Object instead. This causes reflection warnings for fns or loops that use primitive types.

user=> (set! *warn-on-reflection* true)
true
 
user=> (fn [] (loop [t 0] (recur t)))
#<user$eval676$fn__677 user$eval676$fn__677@3d80023a>
 
user=> (fn [] (loop [t 0] (recur (try t))))
NO_SOURCE_FILE:1 recur arg for primitive local: t is not matching primitive, had: Object, needed: long
Auto-boxing loop arg: t
#<user$eval680$fn__681 user$eval680$fn__681@5419323a>

user=> (fn [^long x] (recur (try x)))
NO_SOURCE_FILE:1 recur arg for primitive local: x is not matching primitive, had: Object, needed: long

CompilerException java.lang.IllegalArgumentException:  recur arg for primitive local: x is not matching primitive, had: Object, needed: long, compiling:(NO_SOURCE_PATH:1:1)


 Comments   
Comment by David James [ 15/Jun/14 10:27 PM ]

Without commenting on the most desirable behavior, the following code does not cause reflection warnings:

user=> (set! *warn-on-reflection* true)
true
user=> (fn [] (loop [t 0] (recur (long (try t)))))
#<user$eval673$fn__674 user$eval673$fn__674@4e56c411>
Comment by Nicola Mometto [ 16/Jun/14 6:33 AM ]

Similar ticket http://dev.clojure.org/jira/browse/CLJ-701

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

try/catch in the compiler only implements Expr, not MaybePrimitiveExpr, looking at extending TryExpr with MaybePrimitiveExpr it seems simple enough, but it turns out recur analyzes it's arguments in the statement context, which causes (try ...) to essentially wrap itself in a function like ((fn [] (try ...))), at which point it is an invokeexpr which is much harder to add maybeprimitiveexpr too and it reduces to the same case as CLJ-701

Comment by Kevin Downey [ 22/Jul/14 9:27 PM ]

http://dev.clojure.org/jira/browse/CLJ-701 has a patch that I think solves this

Comment by Alex Miller [ 28/Jul/14 1:56 PM ]

Should I dupe this to CLJ-701?

Comment by Kevin Downey [ 28/Jul/14 5:22 PM ]

if you want the fixes for try out of the return context to be part of CLJ-701 then yes it is a dupe, if you are unsure or would prefer 701 to stay more focused (my patch may not be acceptable, or may be too large and doing too much) then no it wouldn't be a dupe. I sort of took it on myself to solve both in the patch on CLJ-701 because I came to CLJ-701 via Nicola's comment here, and the same compiler machinery can be used for both.

I think the status is pending on the status of CLJ-701.





[CLJ-1482] Replace a couple of (filter (complement ...) ...) usages with (remove ...) Created: 27/Jul/14  Updated: 27/Jul/14

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

Type: Enhancement Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: enhancement

Attachments: Text File 0001-Replace-a-couple-of-filter-complement-usages-with-re.patch    
Patch: Code

 Description   

The title basically says it all - remove exists so we can express our intentions more clearly.






[CLJ-401] Promote "seqable?" from contrib? Created: 13/Jul/10  Updated: 26/Jul/14

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

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


 Description   

This was vaguely discussed here and could potenntially help this ticket as well as be generally useful.

I don't speak for everyone but when I saw sequential? I assumed it would have the semantics that seqable? does. Just my opinion, I'd love to hear someone's who is more informed than mine.

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 #tok1-block-tok}}}})
(1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18)



 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.





[CLJ-701] Compiler loses 'loop's return type in some cases Created: 03/Jan/11  Updated: 26/Jul/14

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

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

Clojure commit 9052ca1854b7b6202dba21fe2a45183a4534c501, version 1.3.0-master-SNAPSHOT


Attachments: 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    
Patch: Code
Approval: Vetted

 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



 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





[CLJ-1192] vec function is substantially slower than into function Created: 06/Apr/13  Updated: 25/Jul/14

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

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

Approval: Vetted

 Description   

(vec coll) and (into [] coll) do exactly the same thing. However, due to into using transients, it is substantially faster. On my machine:

(time (dotimes [_ 100] (vec (range 100000))))
"Elapsed time: 732.56 msecs"

(time (dotimes [_ 100] (into [] (range 100000))))
"Elapsed time: 491.411 msecs"

This is consistently repeatable.

Since vec's sole purpose is to transform collections into vectors, it should do so at the maximum speed available.



 Comments   
Comment by Andy Fingerhut [ 07/Apr/13 5:50 PM ]

I am pretty sure that Clojure 1.5.1 also uses transient vectors for (vec (range n)) (probably also some earlier versions of Clojure, too).

Look at vec in core.clj. It checks whether its arg is a java.util.Collection, which lazy seqs are, so calls (clojure.lang.LazilyPersistentVector/create coll).

LazilyPersistentVector's create method checks whether its argument is an ISeq, which lazy seqs are, so it calls PersistentVector.create(RT.seq(coll)).

All 3 of PersistentVector's create() methods use transient vectors to build up the result.

I suspect the difference in run times are not because of transients or not, but because of the way into uses reduce, and perhaps may also have something to do with the perhaps-unnecessary call to RT.seq in LazilyPersistentVector's create method (in this case, at least – it is likely needed for other types of arguments).

Comment by Alan Malloy [ 14/Jun/13 2:17 PM ]

I'm pretty sure the difference is that into uses reduce: since reducers were added in 1.5, chunked sequences know how to reduce themselves without creating unnecessary cons cells. PersistentVector/create doesn't use reduce, so it has to allocate a cons cell for each item in the sequence.

Comment by Gary Fredericks [ 08/Sep/13 1:55 PM ]

Is there any downside to (defn vec [coll] (into [] coll)) (or the inlined equivalent)?

Comment by Ghadi Shayban [ 11/Apr/14 5:13 PM ]

While I agree that there are improvements and possibly low-hanging fruit, FWIW https://github.com/clojure/tools.analyzer/commit/cf7dda81a22f4c9c1fe64c699ca17e7deed61db4#commitcomment-5989545

showed a 5% slowdown from a few callsites in tools.analyzer.

This ticket's benchmark is incomplete in that it covers a single type of argument (chunked range), and flawed as it timing the expense of realizing the range. (That could be a legit benchmark case, but it shouldn't be the only one).

Sorry to rain on a parade. I promise like speed too!

Comment by Greg Chapman [ 25/Apr/14 5:23 PM ]

One thing to note is that vec has a subtle difference from into when the collection is an Object array of length <= 32. In that case, vec aliases the supplied array, rather than copying it (this is noted in the warning here: http://clojuredocs.org/clojure_core/clojure.core/vec). I believe I read some place that this behavior is intentional, but I can't find the citation.

Comment by Andy Fingerhut [ 25/Apr/14 10:18 PM ]

Greg, CLJ-893 might be what you remember. That is the ticket that was closed by a patch updating the documentation of vec.

Comment by Mike Anderson [ 18/May/14 7:41 AM ]

I think there are quite a few performance improvements that can be made to vec in general. For example, if given a List it should use PersistentVector.create(List) rather than producing an unnecessary seq, which appears to be the case at the moment. Also it should probably return the same object if passed an existing IPersistentVector.

Basically there are a number of cases that we could be handling more efficiently....

I'm taking a look at this now.... will propose a quick patch if it seems there is a good solution.

Comment by Mike Anderson [ 24/Jul/14 4:01 AM ]

I've looked at this issue and it is quite complex. There are multiple types that need to potentially be converted into vectors, and doing so efficiently will often require making use of reduce-style operations on the source collections.

Doing this efficiently will probably in turn require making use of the IReduce interface, which doesn't yet seem to be fully utilised across the Clojure code based. If we do this, lots of operations (not just vec!) can be made faster but it will be quite a major change.

I have a branch that implements some of this but would appreciate feedback if this is the right direction before I take it any further:
https://github.com/mikera/clojure/tree/clj-1192-vec-performance

Comment by Alex Miller [ 24/Jul/14 9:45 AM ]

Thanks Mike! It may take a few days before I can get back to you about this.

Comment by Mike Anderson [ 25/Jul/14 3:44 AM ]

Basically the approach I am proposing is:

  • Make various collections implement IReduce efficiently (if they don't already). Especially applied to chunked seqs etc.
  • Have RT.reduce(...) methods that implement reduce on the Java side
  • Make the Clojure side use IReduce where relevant (should be as simple as extending the existing protocols)
  • Implement vec (and other similar operations) in terms of IReduce - which will solve this specific issue

If we really care about pushing vector performance even further, we can also consider:

  • Create specialised small vector types where appropriate - e.g. a specialised SmallPersistentVector class for <32 elements. This should outperform the more generic PersistentVector which is better suited for large vectors.
  • Some dedicated construction functions that know how to efficiently exploit knowledge about the data source (e.g. creating a vec from a segment of a big Object array can be done with a bunch of arraycopys into 32-element chunks and then constructing a PersistentVector around these)

This should give us a decent speedup overall (of course it would need benchmarking... but I'd hope to see some sort of measurable improvement on a macro benchmark like building and testing Clojure).





[CLJ-1473] Bad pre/post conditions silently passed Created: 24/Jul/14  Updated: 24/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: errormsgs

Attachments: Text File 0001-Validate-that-pre-and-post-conditions-are-vectors.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Before:

user=> ((fn [x] {:pre (pos? x)} x) -5) ; ouch!
-5
user=> ((fn [x] {:pre [(pos? x)]} x) -5) ; meant this
AssertionError Assert failed: (pos? x)  user/eval4075/fn--4076 (form-init5464179453862723045.clj:1)

After:

user=> ((fn [x] {:pre (pos? x)} x) -5)
CompilerException java.lang.IllegalArgumentException: Pre and post conditions should be vectors, compiling:(NO_SOURCE_PATH:1:2) 
user=> ((fn [x] {:pre [(pos? x)]} x) -5)                                  
AssertionError Assert failed: (pos? x)  user/eval2/fn--3 (NO_SOURCE_FILE:2)
user=> ((fn [x] {:post (pos? x)} x) -5)
CompilerException java.lang.IllegalArgumentException: Pre and post conditions should be vectors, compiling:(NO_SOURCE_PATH:3:2) 
user=> ((fn [x] {:post [(pos? x)]} x) -5)              
AssertionError Assert failed: (pos? x)  user/eval7/fn--8 (NO_SOURCE_FILE:4)





[CLJ-308] protocol-ize with-open Created: 21/Apr/10  Updated: 23/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 6
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-1470] Make Atom and ARef easy to subclass Created: 20/Jul/14  Updated: 23/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Aaron Craelius Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJ-1470-v1.patch    
Patch: Code

 Description   

Atom is currently defined as final and ARef.validate() is package-private. This makes it impossible to define a subclass of an Atom and difficult subclass ARef (if validate() needs to be called).

I propose removing the final modifier from Atom, making ARef.validate() protected and also making Atom.state protected (it is currently package-private).

I'm not sure if there is a specific reason why Atom is final - if this is for performance reasons or to prevent someone from doing strange things with Atom's, but I can see a use case for sub-classing it.

One use-case is to create reactive Atom that allows derefs to be tracked (as in reagent). I have some Clojure (not Clojurescript) code where I'm trying to play with this idea and I've had to copy the entire Atom class (because it's sealed) and place it in the clojure.lang package (because ARef.validate() is package-private): https://github.com/aaronc/freactive/blob/master/src/java/clojure/lang/ReactiveAtom.java. In addition, I need to copy the defns for swap! and reset! into my own namespace. This seems a bit inconvenient.



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

related to http://dev.clojure.org/jira/browse/CLJ-803





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

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

Type: Defect Priority: Major
Reporter: Tassilo Horn Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: typehints

Approval: Triaged

 Description   

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

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

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

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

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

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



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

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

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

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

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

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

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

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

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

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

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

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

-> #user/hinted

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

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

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

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

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

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

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





[CLJ-1298] Add more type predicate fns to core Created: 21/Nov/13  Updated: 22/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Alex Fowler Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: None


 Description   

Add more built-in type predicates:

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

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



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

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

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

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

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

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

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

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

vs

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

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

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

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

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

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

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

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

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

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





[CLJ-891] make (symbol foo "bar") work with foo being a namespace Created: 05/Dec/11  Updated: 22/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Joe Gallo Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None

Attachments: Text File 0001-add-Symbol.intern-arity-for-a-Namespace-and-String.patch    
Patch: Code

 Description   

I've run across the need to do (symbol ns "bar") a few times, and the existing approach (symbol (name (ns-name ns)) "bar") just doesn't seem like it ought to be the only way to do the job. Includes a patch to make this work, by adding a new arity to Symbol.intern().

Some discussion on this idea, here: https://groups.google.com/forum/#!topic/clojure/n25aZ5HA7hc/discussion



 Comments   
Comment by Stuart Halloway [ 09/Dec/11 9:39 AM ]

I am not sure I like this, but I would like a rethink of names and namespaces. Doing a lot of cross language work, it would be great to have protocols for "I have a name" and for "I have a namespace".

With such protocols in place, it would also be possible to separately consider implementing symbol et al in terms of them.

Comment by Kevin Downey [ 09/Dec/11 12:24 PM ]

Named being a protocol or an interface seems orthogonal to being able to create a symbol qualified with a namespace when you have a namespace in hand.

I don't think the patch goes far enough, not only should (symbol ns "foo") be supported, but also (symbol ns 'foo), given that (symbol 'foo) works and (symbol "foo") works, (symbol 'bar 'foo) should also work, but doesn't.

if Named is a protocol, and if you extend it to String, and if you make the symbol function create symbols from one or two Named things you still end up having to do (symbol (ns-name ns) 'foo) or (symbol (ns-name ns) "foo")

Comment by Joe Gallo [ 16/Dec/11 4:18 PM ]

Stuart, I'm not opposed to the idea of separate protocols for Named and Namespaced. Where should I go about creating a proposal to create those protocols and get them into clojure? I'm interested in doing the leg-work, or being a part of it. But as an outsider, I don't know what to do next – creating a ticket in Jira exhausted my knowledge of the process.

Comment by Frank Siebenlist [ 02/Mar/12 4:53 PM ]

The same enhancement that joe suggests for symbol, would also apply to keyword.

See: http://groups.google.com/group/clojure/browse_thread/thread/222e4abc16df8b20

Probably same/similar solution applies to both issues.

-FrankS.

Comment by Ambrose Bonnaire-Sergeant [ 22/Jul/14 1:10 AM ]

I did vote on this, but I retracted my vote. Strings for both arguments is clearly a correct implementation to me, but combinations of symbols/namespaces/named/namespaced things are not so clear.





[CLJ-1471] Option to print type info Created: 21/Jul/14  Updated: 21/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Pascal Germroth Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: pprint


 Description   

I've had an issue with defrecord-types being converted into ordinary maps somewhere, which was relatively hard to track down inside a deep structure since they are pprinted as the same thing by default.
The following code patches into the pprint dispatch and prints the type around values; it turned out to be quite useful, but feels hackish.
Maybe something like that would be useful to integrate into clojure.pprint directly (there are a number of cosmetic options already), i.e. into clojure.pprint/write-out.

Only printing (type) may not be enough in some cases; so an option to print all metadata would be nice.
Maybe something like :metadata nil as default, :metadata :type to print types (but also for non-IMetas, using (type) and :metadata true to print metadata for IMetas using (meta).

(defn pprint-with-type
  ([object] (pprint object *out*))
  ([object writer]
   ; keep original dispatch.
   ; calling it directly will print only that object,
   ; but return to our dispatch for subobjects.
   (let [dispatch clojure.pprint/*print-pprint-dispatch*]
     (binding [clojure.pprint/*print-pprint-dispatch*
               (fn [obj]
                 (if (instance? clojure.lang.IMeta obj)
                   (do (print "^{:type ")
                       (dispatch (type obj))
                       (print "} ")
                       (clojure.pprint/pprint-newline :fill)
                       (dispatch obj))
                   (do (print "(^:type ")
                       (dispatch (type obj))
                       (print " ")
                       (clojure.pprint/pprint-newline :fill)
                       (dispatch obj)
                       (print ")"))))]
       (clojure.pprint/pprint object writer)))))





[CLJ-1469] Emit KeywordInvoke callsites only when keyword is not namespaced Created: 18/Jul/14  Updated: 18/Jul/14

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

Type: Enhancement Priority: Trivial
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File kwinvoke.patch    
Patch: Code

 Description   

KeywordInvoke callsites exist to fastpath lookups into records, but involve a lot of callsite machinery. Records don't have any fastpath for namespaced keyword accesses. This patch just falls back to an InvokeExpr when the keyword access is namespaced.

(:ns/kw foo)






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

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

Type: Enhancement Priority: Major
Reporter: Pascal Germroth Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

PersistentVector implements Comparable already.






[CLJ-1464] Incorrectly named parameter to fold function in reducers.clj Created: 12/Jul/14  Updated: 14/Jul/14

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

Type: Defect Priority: Trivial
Reporter: Jason Jackson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File 0001-Rename-a-function-parameter-to-reflect-the-fold-func.patch    
Patch: Code
Approval: Triaged

 Description   

https://github.com/clojure/clojure/blob/master/src/clj/clojure/core/reducers.clj#L95
The 2-arity fold accepts reducef as parameter and then uses it as a combinef.
Instead it should accept combinef as parameter and then use it as a reducef, as every combine fn (monoid) is a reduce fn, but not every reduce fn is a combine fn (it's not associative).



 Comments   
Comment by Jason Jackson [ 12/Jul/14 2:58 PM ]

this is my first patch for clojure please double check everything. CA is done.

Comment by Andy Fingerhut [ 12/Jul/14 7:29 PM ]

Everything gets double checked whether it is your first patch or your 50th

At least as far as the format of the patch being correct, that it applies cleanly to the latest version of Clojure on the master branch, compiles and passes all tests cleanly, all of that is good.

Whether there is interest in taking your proposed change is to be decided by others. It may be some time before it is examined further.

Comment by Jozef Wagner [ 13/Jul/14 4:11 AM ]

This is not a defect. Quoting Rich, "If no combining fn is supplied, the reducing fn is used." (source)

There are three user supplied operations in fold: getting identity element (combinef :: -> T), reducing function (reducef :: T * E -> T) and combining function (combinef :: T * T -> T). For reduce, combining function is not needed but the rest two operations are needed. Thus reducing function (reducef) supplies identity element for reducers and only in folders the identity element is produced by combining function. In case where reducing fn is used for both reducing and combining, it must of course be associative and must handle objects of types T and E as a second argument.

Comment by Jason Jackson [ 14/Jul/14 12:14 AM ]

@Jozef I appreciate the feedback I still think my patch is correct, although I admit everyone's time is better spent not debating this small refactoring so feel free to close it.

One perspective to view my patch from is if we had protocols p/Monoid and p/Reducer. It's possible to reify any object that implements p/Monoid into p/Reducer, but not the other way around since not every p/Reducer is associative.

Comment by Jason Jackson [ 14/Jul/14 12:30 AM ]

From that perspective you could also say that in the 2-arity case the parameter "reducef" requires objects that implement both p/Monoid and p/Reducer, but in the 3-arity case the parameter "reducef" only requires p/Reducer

Comment by Jozef Wagner [ 14/Jul/14 1:38 AM ]

Note that reducef and combinef take different type of second argument, so not every combining function can be used as a reducing one. Your proposal is thus no better than the status quo. Consider following example:

(fold clojure.set/union conj [1 1 2 2 3 3 4 4])




[CLJ-1209] clojure.test does not print ex-info in error reports Created: 11/May/13  Updated: 12/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Thomas Heller Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None

Attachments: Text File 0002-CLJ-1209-show-ex-data-in-clojure-test.patch     File clj-test-print-ex-data.diff     Text File output-with-0002-patch.txt    
Patch: Code
Approval: Triaged

 Description   

clojure.test does not print the data attached to ExceptionInfo in error reports.

Approach: In clojure.stacktrace, which clojure.test uses for printing exceptions, add a check for ex-data and pr it.

Patch: 0002-CLJ-1209-show-ex-data-in-clojure-test.patch



 Comments   
Comment by Alex Miller [ 20/Dec/13 9:53 AM ]

Great idea, thx for the patch!

Comment by Alex Miller [ 20/Dec/13 9:54 AM ]

Would be great to see a before and after example of the output.

Comment by Ivan Kozik [ 12/Jul/14 10:35 PM ]

Attaching sample output





[CLJ-1210] error message for (clojure.java.io/reader nil) — consistency for use with io/resource Created: 23/May/13  Updated: 12/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Trevor Wennblom Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: errormsgs, io

Attachments: File extend-io-factory-to-nil.diff    
Patch: Code and Test
Approval: Triaged

 Description   

This seems to a common idiom:

(clojure.java.io/reader (clojure.java.io/resource "myfile"))

When a file is available these are the behaviors:

=> (clojure.java.io/reader "resources/myfile")
#<BufferedReader java.io.BufferedReader@1f291df0>

=> (clojure.java.io/resource "myfile")
#<URL file:/project/resources/myfile>

=> (clojure.java.io/reader (clojure.java.io/resource "myfile"))
#<BufferedReader java.io.BufferedReader@1db04f7c>

If the file (resource) is unavailable:

=> (clojure.java.io/reader "resources/nofile")
FileNotFoundException resources/nofile (No such file or directory) java.io.FileInputStream.open (FileInputStream.java:-2)

=> (clojure.java.io/resource "nofile")
nil

=> (clojure.java.io/reader (clojure.java.io/resource "nofile"))
IllegalArgumentException No implementation of method: :make-reader of protocol: #'clojure.java.io/IOFactory found for class: nil clojure.core/-cache-protocol-fn (core_deftype.clj:541)

The main enhancement request is to have a better error message from `(clojure.java.io/reader nil)`. I'm not sure if io/resource should return something like 'resource "nofile" not found' or if io/reader could add a more helpful suggestion.



 Comments   
Comment by Alexander Redington [ 14/Feb/14 3:13 PM ]

This patch extends IOFactory to nil, providing error messages consistent with the default error messages provided for Object.

Comment by Benjamin Peter [ 15/Feb/14 1:31 PM ]

Looks like a good solution to me as a user. Thanks for the effort!

Comment by Dennis Schridde [ 12/Jul/14 2:01 AM ]

I would also be interested in a solution, as I am currently running into this with the ClojureScript compiler.





[CLJ-1463] Providing own ClassLoader for eval is broken Created: 10/Jul/14  Updated: 10/Jul/14

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

Type: Defect Priority: Minor
Reporter: Volkert Oakley Jurgens Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler
Environment:

Clojure 1.6.0



 Description   

clojure.lang.Compiler has a method with the signature

public static Object eval(Object form, boolean freshLoader)

but the freshLoader argument is ignored since https://github.com/clojure/clojure/commit/2c2ed386ed0f6f875342721bdaace908e298c7f3

Is there a good reason this still needs to be "hotfixed" like this?

We would like to provide our own ClassLoader for eval to manage the lifecycle of the generated classes.






[CLJ-1462] cl-format throws ClassCastException: Writer cannot be cast to Future/IDeref Created: 07/Jul/14  Updated: 09/Jul/14

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

Type: Defect Priority: Minor
Reporter: Pascal Germroth Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: print


 Description   

Using ~I and ~_ etc fails in many situations, the most trivial one being:

Clojure 1.6.0 and 1.5.1:

user=> (clojure.pprint/cl-format true "~I")
ClassCastException java.io.PrintWriter cannot be cast to java.util.concurrent.Future  clojure.core/deref-future (core.clj:2180)
user=> (clojure.pprint/cl-format nil "~I")
ClassCastException java.io.StringWriter cannot be cast to java.util.concurrent.Future  clojure.core/deref-future (core.clj:2180)
user=> (clojure.pprint/cl-format nil "~_")
ClassCastException java.io.StringWriter cannot be cast to java.util.concurrent.Future  clojure.core/deref-future (core.clj:2180)

Clojure 1.4.0

user=> (clojure.pprint/cl-format true "~I")
ClassCastException java.io.OutputStreamWriter cannot be cast to clojure.lang.IDeref  clojure.core/deref (core.clj:2080)
user=> (clojure.pprint/cl-format nil "~I")
ClassCastException java.io.StringWriter cannot be cast to clojure.lang.IDeref  clojure.core/deref (core.clj:2080)
user=> (clojure.pprint/cl-format nil "~_")
ClassCastException java.io.StringWriter cannot be cast to clojure.lang.IDeref  clojure.core/deref (core.clj:2080)

These work in other implementations, i.e. clisp, creating empty output in these trivial cases:

> (format t "~I")
NIL
> (format nil "~I")
""
> (format nil "~_")
""


 Comments   
Comment by Andy Fingerhut [ 07/Jul/14 11:01 AM ]

The tilde-underscore sequence is for "conditional newline", according to the CLHS here: http://www.lispworks.com/documentation/lw51/CLHS/Body/22_cea.htm

Tilde-capital-letter-I is for indent: http://www.lispworks.com/documentation/lw51/CLHS/Body/22_cec.htm

Comment by Pascal Germroth [ 07/Jul/14 12:09 PM ]

Ah, didn't think to try that. It fails without cl-format as well:

user=> (clojure.pprint/pprint-newline :linear)
ClassCastException java.io.PrintWriter cannot be cast to java.util.concurrent.Future  clojure.core/deref-future (core.clj:2180)
user=> (clojure.pprint/pprint-indent :block 0)
ClassCastException java.io.PrintWriter cannot be cast to java.util.concurrent.Future  clojure.core/deref-future (core.clj:2180)

Manually creating a pretty writer does work though:

user=> (binding [*out* (clojure.pprint/get-pretty-writer *out*)] (clojure.pprint/pprint-newline :linear))
nil

In the get-pretty-writer doc it says:

Generally, it is unnecessary to call this function, since pprint,
write, and cl-format all call it if they need to.

Which appears to not be true for cl-format, and it would be nice if it would be applied automatically for all functions that need a pretty writer.

Comment by Pascal Germroth [ 09/Jul/14 6:37 PM ]

More bad news!
Manually creating a pretty-writer doesn't do the trick either, because it is not being properly flushed:

user=> (binding [*out* (get-pretty-writer *out*)] (cl-format true "hello ~_world~%"))
hello world
nil
user=> (binding [*out* (get-pretty-writer *out*)] (cl-format true "hello ~_world"))
hellonil
user=> (binding [*out* (get-pretty-writer *out*)] (cl-format true "hello ~_world") (.ppflush *out*))
hello worldnil

The ~% inserts an unconditional newline like \n, which also works as expected.

Insert ~_ before and it only prints up to that one. But I've also managed to get it to abort at other ~_ s, maybe because other commands flushed it.

Manually flushing it, like the inexplicably private with-pretty-writer macro does works though.
I don't understand why get-pretty-writer is exposed but not the macro that is needed to use it properly. Also all functions using pretty-writer facilities should use with-pretty-writer, that's what it appears to be specifically designed for. Then there's no need to expose it (or get-pretty-writer).





[CLJ-1459] records should support transient Created: 05/Jul/14  Updated: 06/Jul/14

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

Type: Enhancement Priority: Major
Reporter: Yongqian Li Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: defrecord


 Description   

user=> (defrecord R [a])
user.R
user=> (transient (->R nil))
ClassCastException user.R cannot be cast to clojure.lang.IEditableCollection clojure.core/transient (core.clj:3060)






[CLJ-971] Jar within a jar throws a runtime error Created: 10/Apr/12  Updated: 06/Jul/14

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

Type: Defect Priority: Minor
Reporter: Ron Romero Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None
Environment:

Maven using the one-jar plugin


Attachments: Text File clj-971-1.patch     Text File clj-971-2.patch    
Patch: Code
Approval: Incomplete

 Description   

The code in RT.load() will load a class by name in several situations. One of these is when a .class resource exists, a .clj resource exists and the class is newer than the clj file. To determine the age of the class and clj, a call is made into RT.lastModified with the resource URL. If the URL is a jar protocol, then RT.lastModified() will try to open a connection to that jar, find the entry in the jar, and return its time.

Sometimes deployment occurs using nested jar files (for example: http://one-jar.sourceforge.net/) and custom classloaders. In this usage, the jar file can be opened, but the entry will not be found and an NPE will result during load:

Caused by: java.lang.NullPointerException
    at clojure.lang.RT.lastModified(RT.java:374)
    at clojure.lang.RT.load(RT.java:408)
    at clojure.lang.RT.load(RT.java:398)
    at clojure.lang.RT.doInit(RT.java:434)
    at clojure.lang.RT.<clinit>(RT.java:316)
... 7 more

Proposal: Make lastModified() more tolerant of finding a null entry and falling back to return the last modified date of the jar file itself.

Patch: clj-971-2.patch

Screened by:



 Comments   
Comment by Anders Sveen [ 11/Nov/13 3:29 AM ]

Would be awesome if you could get this working. Wanting to use som Clojure libs in Java so Leiningen uberjar is not an option right now.

Comment by Paavo Parkkinen [ 24/Nov/13 7:11 AM ]

Took the code change in the description and rolled it into a patch to hopefully push this forward a little bit.

Comment by Alex Miller [ 24/Nov/13 10:06 AM ]

Thanks Paavo - on a quick scan, it looks like in the case you're interested in the updated code would now call url.openConnection() twice - perhaps that could be factored out?

Comment by Paavo Parkkinen [ 25/Nov/13 6:19 AM ]

New patch with duplicate calls to url.openConnection() factored out.

Comment by Gleb Kanterov [ 13/Dec/13 4:30 AM ]

I had the same issue, adding following line to manifest file worked for me

One-Jar-URL-Factory: com.simontuffs.onejar.JarClassLoader$OneJarURLFactory
Comment by Stuart Halloway [ 31/Jan/14 1:04 PM ]

Can we get a test showing the normal path and the can't-read-inside path?

Comment by Sean Shubin [ 17/Apr/14 9:34 PM ]

I notice this solution falls back on the last modified date for the entire jar, while it is still possible to get the date of the individual file, albeit less efficiently.
I posted a way to get the individual file date in CLJ-1405.
Perhaps you don't need the date of the individual file, but I am having a hard time groking the calling code so I am not sure what its intent is.
I did notice in the calling code that the if statement
(classURL != null && (cljURL == null || lastModified(classURL, classfile) > lastModified(cljURL, cljfile))) || classURL == null
Is logically equivalent to the much simpler
classURL == null || cljURL == null || lastModified(classURL, classfile) > lastModified(cljURL, cljfile)

Comment by Alex Miller [ 23/May/14 10:56 AM ]

Why is the solution that Gleb proposed not sufficient without changing Clojure? It seems fairly trivial to add a manifest line in the one-jar packaging step:

One-Jar-URL-Factory: com.simontuffs.onejar.JarClassLoader$OneJarURLFactory

Stu - I doubt the effort of setup is worth a test in the clojure tests but it might be useful to have documented steps to repro manually.

Sean - I think the effort on CLJ-1405 of scanning the full jar is going too far and not needed if we decide to go forward with this.

Comment by Sean Shubin [ 06/Jul/14 12:32 AM ]

Alex,

Regarding scanning the jar, I was worried about the behavior not matching the intent of the code. As I have not had time to get familiar with the code, I figured this route was safer. If scanning the jar turns out to be overkill, so be it, we could just have the lastModified return 0 or something. Either way the code should be under test coverage, I will try to block out some time for that next weekend. Also, as I mentioned in CLJ-1405 (sorry about the duplicate issue), I did document the steps to reproduce the issue here: https://github.com/SeanShubin/clojure-one-jar.

Regarding Gleb's solution, that seems like a decent workaround, but I figured it would be better to fix the bug where it is rather than rely on a workaround. Shouldn't Clojure be changed if it is legitimately a bug in Clojure?

Comment by Alex Miller [ 06/Jul/14 6:06 PM ]

Onejar creates a new kind of classpath container (which happens to have the same type as a .jar) but works differently. As such, resource urls that start with the jar protocol no longer work in certain ways (in this case returning null for lastModified). Java provides a mechanism to customize this behavior with a new url protocol, which is what the One-Jar-URL-Factory installs and uses. It thus seems to me that onejar introduces the issue and also provides the solution. As far as I see it, this is not a bug in Clojure itself.

If there were situations (without custom jar types or protocols) where lastModified() really does throw an NPE, then I would be more convinced that this was a scenario where Clojure should change behavior.

To modify the greeter example given in CLJ-1405:
1) bump the onejar-maven-plugin version to at least 1.4.5
2) add the following to the executions/execution/configuration for the plugin:

<manifestEntries>
      <One-Jar-URL-Factory>com.simontuffs.onejar.JarClassLoader$OneJarURLFactory</One-Jar-URL-Factory>
    </manifestEntries>




[CLJ-1460] Clojure transforms literals of custom IPersistentCollections not created via deftype/defrecord to their generic clojure counterpart Created: 06/Jul/14  Updated: 06/Jul/14

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

Approval: Triaged

 Description   
user=> (class (eval (sorted-map 1 1)))
clojure.lang.PersistentArrayMap ;; expected: clojure.lang.PersistentTreeMap


 Comments   
Comment by Alex Miller [ 06/Jul/14 5:35 PM ]

Seems related to CLJ-1093.

Comment by Nicola Mometto [ 06/Jul/14 5:51 PM ]

The symptoms are indeed similar but there are differences: CLJ-1093 affects all empty IPersistentCollections, this one affects all {ISeq,IPersistentList,IPersistentMap,IPersistentVector,IPersistentSet} collections that are not IRecord/IType.





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

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

Approval: Triaged

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

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






[CLJ-1093] Empty PersistentCollections get incorrectly evaluated as their generic clojure counterpart Created: 24/Oct/12  Updated: 06/Jul/14

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

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: collections, compiler

Attachments: Text File 0001-CLJ-1093-fix-compilation-of-empty-PersistentCollecti.patch     Text File clj-1093-fix-empty-record-literal-patch-v2.txt    
Patch: Code and Test
Approval: Incomplete

 Description   
user> (defrecord x [])
user.x
user> #user.x[]   ;; expect: #user.x{}
{}
user> #user.x{}   ;; expect: #user.x{}
{}
user> #clojure.lang.PersistentTreeMap[]
{}
user> (class *1)  ;; expect: clojure.lang.PersistentTreeMap
clojure.lang.PersistentArrayMap

Cause: Compiler's ConstantExpr parser returns an EmptyExpr for all empty persistent collections, even if they are of types other than the core collections (for example: records, sorted collections, custom collections). EmptyExpr reports its java class as one the classes - IPersistentList/IPersistentVector/IPersistentMap/IPersistentSet rather than the original type.

Proposed: If one of the Persistent* classes, then create EmptyExpr as before, otherwise retain the ConstantExpression of the original collection.
Since EmptyExpr is a compiler optimization that applies only to some concrete clojure collections, making EmptyExpr dispatch on concrete types rather than on generic interfaces makes the compiler behave as expected

Patch: 0001-CLJ-1093-fix-compilation-of-empty-PersistentCollecti.patch

Screened by:



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

Unable to reproduce this bug on latest version of master. Most likely fixed by some of the recent changes to data literal readers.

Marking Not-Approved.

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

Could not reproduce in master.

Comment by Nicola Mometto [ 01/Mar/13 1:23 PM ]

I just checked, and the problem still exists for records with no arguments:

Clojure 1.6.0-master-SNAPSHOT
user=> (defrecord a [])
user.a
user=> #user.a[]
{}

Admittedly it's an edge case and I see little usage for no-arguments records, but I think it should be addressed aswell since the current behaviour is not what one would expect

Comment by Herwig Hochleitner [ 02/Mar/13 8:14 AM ]

Got the following REPL interaction:

% java -jar ~/.m2/repository/org/clojure/clojure/1.5.0/clojure-1.5.0.jar
user=> (defrecord a [])
user.a
user=> (a.)
#user.a{}
user=> #user.a{}
{}
#user.a[]
{}

This should be reopened or declined for another reason than reproducability.

Comment by Nicola Mometto [ 10/Mar/13 2:18 PM ]

I'm reopening this since the bug is still there.

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

Patch clj-1093-fix-empty-record-literal-patch-v2.txt dated Mar 13, 2013 is identical to Bronsa's patch 001-fix-empty-record-literal.patch dated Oct 24, 2012, except that it applies cleanly to latest master. I'm not sure why the older patch doesn't but git doesn't like something about it.

Comment by Nicola Mometto [ 26/Jun/13 8:06 PM ]

Patch 0001-CLJ-1093-fix-empty-records-literal-v2.patch solves more issues than the previous patch that was not evident to me at the time.

Only collections that are either PersistentList or PersistentVector or PersistentHash[Map|Set] or PersistentArrayMap can now be EmptyExpr.
This is because we don't want every IPersistentCollection to be emitted as a generic one eg.

user=> (class #clojure.lang.PersistentTreeMap[])
clojure.lang.PersistentArrayMap

Incidentally, this patch also solves CLJ-1187
This patch should be preferred over the one on CLJ-1187 since it's more general

Comment by Jozef Wagner [ 09/Aug/13 2:08 AM ]

Maybe this is related:

user=> (def x `(quote ~(list 1 (clojure.lang.PersistentTreeMap/create (seq [1 2 3 4])))))
#'user/x
user=> x
(quote (1 {1 2, 3 4}))
user=> (class (second (second x)))
clojure.lang.PersistentTreeMap
user=> (eval x)
(1 {1 2, 3 4})
user=> (class (second (eval x)))
clojure.lang.PersistentArrayMap

Even if the collection is not evaluated, it is still converted to the generic clojure counterpart.

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

In the change for ObjectExpr.emitValue() where you've added PersistentArrayMap to the PersistentHashMap case, should the IPersistentVector case below that be PersistentVector instead, otherwise it would snare a custom IPersistentVector that's not a PersistentVector, right?

This line: "else if(form instanceof ISeq)" at the end of the Compiler diff has different leading tabs which makes the diff slightly more confusing than it could be.

Would be nice to add a test for the sorted map case in the description.

Marking incomplete to address some of these.

Comment by Alex Miller [ 13/May/14 10:43 PM ]

bump

Comment by Nicola Mometto [ 14/May/14 4:24 AM ]

Attached patch 0001-CLJ-1093-fix-empty-collection-literal-evaluation.patch which implements your suggestions.

replacing IPersistentVector with PersistentVector in ObjectExpr.emitValue() exposes a print-dup limitation: it expects every IPersistentCollection to have a static "create" method.

This required special casing for MapEntry and APersistentVector$SubVector

Comment by Nicola Mometto [ 16/May/14 3:57 PM ]

I updated the patch adding print-dups for APersistentVector$SubVec and other IPersistentVectors rather than special casing them in the compiler

Comment by Alex Miller [ 23/May/14 4:21 PM ]

All of the checks on concrete classes in the Compiler parts of this patch don't sit well with me. I understand how you got to this point and I don't have an alternate recommendation (yet) but all of that just feels like the wrong direction.

We want to be built on abstractions such that internal collections are not special; they should conform to the same expectations as an external collection and both should follow the same paths in the compiler - needing to check for those types is a flag for me that something is amiss.

I am marking Incomplete for now based on these thoughts.

Comment by Nicola Mometto [ 06/Jul/14 10:01 AM ]

I've been thinking for a while about this issue and I've come to the conclusion that in my later patches I've been trying to incorporate fixes for 3 different albeit related issues:

1- Clojure transforms all empty IPersistentCollections in their generic Clojure counterpart

user> (defrecord x [])
user.x
user> #user.x[]   ;; expected: #user.x{}
{}
user> #user.x{}   ;; expected: #user.x{}
{}
user> #clojure.lang.PersistentTreeMap[]
{}
user> (class *1)  ;; expected: clojure.lang.PersistentTreeMap
clojure.lang.PersistentArrayMap

2- Clojure transforms all the literals of collections implementing the Clojure interfaces (IPersistentList, IPersistentVector ..) that are NOT defined with deftype or defrecord, to their
generic Clojure counterpart

user=> (class (eval (sorted-map 1 1)))
clojure.lang.PersistentArrayMap ;; expected: clojure.lang.PersistentTreeMap

3- print-dup is broken for some Clojure persistent collections

user=> (print-dup (subvec [1] 0) *out*)
#=(clojure.lang.APersistentVector$SubVector/create [1])
user=> #=(clojure.lang.APersistentVector$SubVector/create [1])
IllegalArgumentException No matching method found: create  clojure.lang.Reflector.invokeMatchingMethod (Reflector.java:53)

I'll keep this ticket regarding issue #1 and open two other tickets for issue #2 and #3

Comment by Nicola Mometto [ 06/Jul/14 10:15 AM ]

I've attached a new patch fixing only this issue, the approach is explained in the description





[CLJ-1416] Support transients in gvec Created: 06/May/14  Updated: 05/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Michał Marczyk Assignee: Michał Marczyk
Resolution: Unresolved Votes: 0
Labels: collections, transient

Attachments: Text File 0001-CLJ-1416-transients-hash-caching-for-gvec-Object-met.patch     Text File 0002-CLJ-1416-transients-hash-caching-interop-improvement.patch    
Patch: Code
Approval: Triaged

 Description   

Vectors of primitives produced by vector-of do not support transients.

core.rrb-vector implements transient support for vectors of primitives. Such transient-enabled vectors of primitives can be obtained in a number of ways: (1) using a gvec instance as an argument to fv/catvec (if RRB concatenation happens, which is not guaranteed) or fv/subvec; (2) passing a gvec instance to fv/vec, which as of core.rrb-vector 0.0.11 will simply rewrap the gvec tree in an RRB wrapper; (3) using fv/vector-of instead of clojure.core/vector-of. Native support in gvec would still be useful as part of an effort to make supported functionality consistent across vector flavours (see CLJ-787 in this connection); gvec is also simpler and still has (and is likely to maintain) a performance edge.

A port of core.rrb-vector's transient support to gvec is available here:

https://github.com/michalmarczyk/clojure/tree/transient-gvec

I'll bring it up to date with current master shortly.

See the clojure-dev thread for some benchmarks:

https://groups.google.com/d/msg/clojure-dev/9ozYI1e5SCM/BAIazVOkUmcJ



 Comments   
Comment by Michał Marczyk [ 13/May/14 5:32 AM ]

Here's the current version of the patch (0001-CLJ-1416-transients-hash-caching-for-gvec-Object-met.patch). It includes a few additional changes – here's the commit message:

CLJ-1416: transients, hash caching for gvec, Object methods for gvec seqs

  • Adds transient support to gvec
  • Adds hash{eq,Code} caching to gvec and gvec seqs
  • Implements hashCode, equals, toString for gvec seqs

https://github.com/michalmarczyk/clojure/tree/transient-gvec-1.6

Comment by Michał Marczyk [ 05/Jul/14 2:59 AM ]

Here's an updated patch with some additional interop-related improvements.

The new commit message:

CLJ-1416: transients, hash caching, interop improvements for gvec

  • Adds transient support to gvec
  • Adds hash{eq,Code} caching to gvec and gvec seqs
  • Implements hashCode, equals, toString for gvec seqs
  • Correctly implements iterator-related methods for gvec and gvec seqs
  • Introduces throw-unsupported and caching-hash (both marked private)




[CLJ-1458] Use transients in merge and merge-with Created: 04/Jul/14  Updated: 04/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Yongqian Li Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: newbie, performance

Approval: Triaged

 Description   

It would be nice if merge used transients.






[CLJ-1286] Fix reader spec and regex to match code for keywords starting with digits Created: 31/Oct/13  Updated: 02/Jul/14

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

Type: Defect Priority: Minor
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: reader


 Description   

The reader page at http://clojure.org/reader states that symbols (and keywords) cannot start with a number and the regex used in LispReader (and EdnReader) also has this intention. CLJ-1252 addressed this by fixing the broken reader regex to match the spec. However, that broke some existing code so we rolled back the change. There is still a disconnect here and this ticket serves to decide what to do instead.

I presume that we are effectively deciding that keywords like :5 are ok to read. If so, we should alter the regex to more accurately capture that intent - right now it allows these purely by accident due to backtracking. A secondary question is whether the Clojure and EDN reader spec should also explicitly allow these as valid. My preference would be to have the reader and the spec match, so I would lobby to loosen the reader spec.



 Comments   
Comment by Nicola Mometto [ 12/Nov/13 4:50 PM ]

what about keywords like :1/1 or :1/a? Clojure currently accepts the latter but not the former.

Comment by Francis Avila [ 02/Jul/14 3:13 PM ]

There's more discussion of this problem (and symbol/keyword parsing in general) in the context of cljs.reader at CLJS-677.

Comment by Andy Fingerhut [ 02/Jul/14 3:27 PM ]

Francis, can you double-check that ticket number? The one you mention (CLJS-667) doesn't seem to have any discussion of this problem.

Comment by Francis Avila [ 02/Jul/14 3:39 PM ]

Sincere apologies, it's CLJS-677. (Original post corrected too.)





[CLJ-1452] clojure.core/*rand* for seedable randomness Created: 20/Jun/14  Updated: 01/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: 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 output and interpretation from the README are reproduced below for posterity.

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:

$ 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


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





[CLJ-1457] once the compiler pops the dynamic classloader from the stack, attempts to read record reader literals will fail Created: 30/Jun/14  Updated: 01/Jul/14

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

Type: Defect Priority: Minor
Reporter: Kevin Downey Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

reproduction case

java -jar target/clojure-1.7.0-master-SNAPSHOT.jar -e "(do (ns foo.bar) (defrecord Foo []) (defn -main [] (prn (->Foo)) (read-string \"#foo.bar.Foo[]\")))" -m foo.bar

result

#'foo.bar/-main
#foo.bar.Foo{}
Exception in thread "main" java.lang.ClassNotFoundException: foo.bar.Foo
	at java.net.URLClassLoader$1.run(URLClassLoader.java:372)
	at java.net.URLClassLoader$1.run(URLClassLoader.java:361)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.net.URLClassLoader.findClass(URLClassLoader.java:360)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
	at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:308)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
	at java.lang.Class.forName0(Native Method)
	at java.lang.Class.forName(Class.java:340)
	at clojure.lang.RT.classForNameNonLoading(RT.java:2076)
	at clojure.lang.LispReader$CtorReader.readRecord(LispReader.java:1195)
	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)
	at clojure.lang.RT.readString(RT.java:1737)
	at clojure.core$read_string.invoke(core.clj:3497)
	at foo.bar$_main.invoke(NO_SOURCE_FILE:1)
	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.core$apply.invoke(core.clj:624)
	at clojure.main$main_opt.invoke(main.clj:315)
	at clojure.main$main.doInvoke(main.clj:420)
	at clojure.lang.RestFn.invoke(RestFn.java:457)
	at clojure.lang.Var.invoke(Var.java:394)
	at clojure.lang.AFn.applyToHelper(AFn.java:165)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)

what happens is the evaluator pushes a dynamicclassloader, evaluates some code, then -m foo.bar causes foo.bar/-main to be called, which tries to read in a literal for the just defined record, but it fails because when foo.bar/-main is called clojure.lang.Compiler/LOADER is unbound so RT uses the sun.misc classloader to try and find the class, which it knows nothing about



 Comments   
Comment by Kevin Downey [ 01/Jul/14 11:42 AM ]

this means that you cannot depend on ever being able to deserialize a record with read unless you are at the repl (the only place clojure.lang.Compiler/LOADER is guaranteed to be bound).

1. print/read support for records is broken
2. behavior is inconsistent between the repl and other environments
which will drive people crazy when the try to figure out why their code isn't working

Comment by Alex Miller [ 01/Jul/14 4:43 PM ]

I would appreciate more understanding about how this affects code run in a more normal scenario (than calling clojure.main with -e and -m).





[CLJ-1255] Support Abstract Base Classes with Java-only variant of "reify" Created: 06/Sep/13  Updated: 01/Jul/14

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

Type: Enhancement Priority: Major
Reporter: Mike Anderson Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: interop

Approval: Triaged

 Description   

Problem:

  • Various Java APIs depend on extension of abstract base classes rather than interfaces
  • "proxy" has limitations (no access to protected fields or super)
  • "proxy" has performance overhead because of an extra layer of functions / parameter boxing etc.
  • "gen-class" is complex and is complected with compilation / bytecode generation

In summary: Clojure does not currently have a good / convenient way to extend a Java abstract base class dynamically.

The proposal is to create a variant of "reify" that allows the extension of a single abstract base class (optionally also with interfaces/protocols). Code generation would occur as if the abstract base class had been directly extended in Java (i.e. with full access to protected members and with fully type-hinted fields).

Since this is a JVM-only construct, it should not affect the portable extension methods in Clojure (deftype etc.). We propose that it is placed in an separate namespace that could become the home for other JVM-specific interop functionality, e.g. "clojure.java.interop"



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

From Rich: we do not want to support abstract classes in a portable construct (reify, deftype). However, this would be considered as a new Java-only construct (extend-class or reify-class). If you could modify the ticket appropriately, will move back to Triaged.





[CLJ-1454] Companion to swap! which returns the old value Created: 28/Jun/14  Updated: 30/Jun/14

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

Type: Enhancement Priority: Minor
Reporter: Philip Potter Assignee: Unassigned
Resolution: Unresolved Votes: 2
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-1456] The compiler ignores too few or too many arguments to throw Created: 30/Jun/14  Updated: 30/Jun/14

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

Type: Enhancement Priority: Major
Reporter: Alf Kristian Støyle Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler

Attachments: Text File 0001-CLJ-1456-counting-forms-to-catch-malformed-throw-for.patch     Text File 0001-CLJ-1456-counting-forms-to-catch-malformed-throw-for.patch    
Patch: Code and Test
Approval: Triaged

 Description   

The compiler does not fail on "malformed" throw forms:

user=> (defn foo [] (throw))
#'user/foo

user=> (foo)
NullPointerException   user/foo (NO_SOURCE_FILE:1)

user=> (defn bar [] (throw Exception baz))
#'user/bar

user=> (bar)
ClassCastException java.lang.Class cannot be cast to java.lang.Throwable  user/bar (NO_SOURCE_FILE:1)

; This one works, but ignored-symbol, should probably not be ignored
user=> (defn quux [] (throw (Exception. "Works!") ignored-symbol))
#'user/quux

user=> (quux)
Exception Works!  user/quux (NO_SOURCE_FILE:1)

The compiler can easily avoid these by counting forms.



 Comments   
Comment by Alf Kristian Støyle [ 30/Jun/14 11:56 AM ]

Not sure how to create a test for the attached patch. Will happily do so if anyone has a suggestion.

Comment by Alex Miller [ 30/Jun/14 12:23 PM ]

Re testing, I think the examples you give are good - you should add tests to test/clojure/test_clojure/compilation.clj that eval the form and expect compilation errors. I'm sure you can find similar examples.

Comment by Alf Kristian Støyle [ 30/Jun/14 2:01 PM ]

Newest patch also contains a few tests.





[CLJ-1330] Class name clash between top-level functions and defn'ed ones Created: 22/Jan/14  Updated: 30/Jun/14

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

Type: Defect Priority: Critical
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 7
Labels: aot, compiler

Attachments: Text File 0001-Fix-CLJ-1330-make-top-level-named-functions-classnam.patch     File demo1.clj    
Patch: Code
Approval: Vetted

 Description   

Named anonymous fn's are not guaranteed to have unique class names when AOT-compiled.

For example:

(defn g [])
(def xx (fn g []))

When AOT-compiled both functions will emit user$g.class, the latter overwriting the former.

Demonstration script: demo1.clj

Patch: 0001-Fix-CLJ-1330-make-top-level-named-functions-classnam.patch

Approach: Generate unique class names for named fn's the same way as for unnamed anonymous fn's.

See also: This patch also fixes the issue reported in CLJ-1227.



 Comments   
Comment by Ambrose Bonnaire-Sergeant [ 22/Jan/14 11:12 AM ]

This seems like the reason why jvm.tools.analyzer cannot analyze clojure.core. On analyzing a definline, there is an "attempted duplicate class definition" error.

This doesn't really matter, but I thought it may or may not be useful information to someone.

Comment by Nicola Mometto [ 22/Jan/14 11:35 AM ]

Attached a fix.

This also fixes AOT compiling of code like:

(def x (fn foo []))
(fn foo [])
Comment by Nicola Mometto [ 22/Jan/14 11:39 AM ]

Cleaned up patch

Comment by Alex Miller [ 22/Jan/14 12:43 PM ]

It looks like the patch changes indentation of some of the code - can you fix that?

Comment by Nicola Mometto [ 22/Jan/14 3:57 PM ]

Updated patch without whitespace changes

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

Thanks, that's helpful.

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

There is consensus that this is a problem, however this is an area of the code with broad impacts as it deals with how classes are named. To that end, there is some work that needs to be done in understanding the impacts before we can consider it.

Some questions we would like to answer:

1) According to Rich, naming of (fn x []) function classes used to work in the manner of this patch - with generated names. Some code archaeology needs to be done on why that was changed and whether the change to the current behavior was addressing problems that we are likely to run into.

2) Are there issues with recursive functions? Are there impacts either in AOT or non-AOT use cases? Need some tests.

3) Are there issues with dynamic redefinition of functions? With the static naming scheme, redefinition causes a new class of the same name which can be picked up by reload of classes compiled to the old definition. With the dynamic naming scheme, redefinition will create a differently named class so old classes can never pick up a redefinition. Is this a problem? What are the impacts with and without AOT? Need some tests.

Comment by Nicola Mometto [ 24/Jan/14 11:39 AM ]

Looks like the current behaviour has been such since https://github.com/clojure/clojure/commit/4651e60808bb459355a3a5d0d649c4697c672e28

My guess is that Rich simply forgot to consider the (def y (fn x [] ..)) case.

Regarding 2 and 3, the dynamic naming scheme is no different than what happens for anonymous functions so I don't see how this could cause any issue.

Recursion on the fn arg is simply a call to .invoke on "this", it's classname unaware.

I can add some tests to test that

(def y (fn x [] 1))
and
(fn x [] 2)
compile to different classnames but other than that I don't see what should be tested.

Comment by Stuart Halloway [ 27/Jun/14 2:17 PM ]

incomplete pending the answers to Alex Miller's questions in the comments

Comment by Nicola Mometto [ 27/Jun/14 3:20 PM ]

I believe I already answered his questions, I'll try to be a bit more explicit:
I tracked the relevant commit from Rich which added the dynamic naming behaviour https://github.com/clojure/clojure/commit/4651e60808bb459355a3a5d0d649c4697c672e28#diff-f17f860d14163523f1e1308ece478ddbL3081 which clearly shows that this bug was present since then so.

Regarding redefinitions or recursive functions, both of those operations never take in account the generated fn name so they are unaffected.





[CLJ-1455] Postcondition in defrecord: Compiler unable to resolve symbol % Created: 28/Jun/14  Updated: 29/Jun/14

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

Type: Enhancement Priority: Minor
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: defrecord


 Description   

Clojure's postconditions[1] are a splendiferous, notationally
idiot-proof way to scrutinize a function's return value without
inadvertently causing it to return something else.

Functions (implementing protocols) for a record type may be defined in
its defrecord or with extend-type. In functions defined in
extend-type, postconditions work as expected. Therefore, it is a
surprise that functions defined in defrecord cannot use
postconditions.

Actually it appears defrecord sees a pre/postcondition map as ordinary
code, so the postcondition runs at the beginning of the function (not
the end) and the symbol % (for return value) is not bound.

The code below shows a protocol and two record types that implement
it. Type "One" has an in-the-defrecord function definition where the
postcondition does not compile. Type "Two" uses extend-type and the
postcondition works as expected.

Unable to find source-code formatter for language: clojure. Available languages are: javascript, sql, xhtml, actionscript, none, html, xml, java
(defprotocol ITimesThree
  (x3 [a]))

;; defrecord with functions inside cannot use postconditions.
(defrecord One
    []
  ITimesThree
  (x3 [a]
    {:pre [(do (println "One x3 pre") 1)] ;; (works fine)
     :post [(do (println "One x3 post, %=" %) 1)]
     ;; Unable to resolve symbol: % in this context.
     ;; With % removed, it compiles but runs at start, not end.
     }
    (* 1 3)))

;; extend-type can add functions with postconditions to a record.
(defrecord Two
    [])
(extend-type Two
  ITimesThree
  (x3 [a]
    {:pre [(do (println "Two x3 pre") 1)] ;; (works fine)
     :post [(do (println "Two x3 post, %=" %) 1)] ;; (works fine)
     }
    (* 2 3)))

(defn -main
  "Main"
  []
  (println (x3 (->One)))
  (println (x3 (->Two))))

[1] http://clojure.org/special_forms, in the fn section.






[CLJ-1169] Report line,column, and source in defmacro errors Created: 22/Feb/13  Updated: 28/Jun/14

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

Type: Enhancement Priority: Major
Reporter: Andrei Kleschinski Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: errormsgs
Environment:

Windows


Attachments: Text File 0001-CLJ-1169-proposed-patch.patch     Text File 0002-CLJ-1169-fix-unit-tests.patch     Text File CLJ-1169-code-and-test-1.patch     File defn_error_message.clj    
Patch: Code
Approval: Screened

 Description   

Summary This patch grew out of a desire to have defn report filename and line numbers for parameter declaration errors, but the approach chosen does something more broad, and likely very useful: Anytime defmacro is throwing a non-CompilerException, wrap it in a CompilerException that captures LINE, COLUMN, and SOURCE. Presumably this would improve reporting for many other macros as well. The patch also tweaks errors messages to add quotes, e.g. "problem" instead of problem, which seems useful.

Screened By Stu
Patch CLJ-1169-code-and-test-1.patch, which aggregates the work in other patches to a single patch that works on current master.

When mistyping parameter list in defn declaration, e.g.

(defn test
 (some-error))

error message shows name of parameter (without quotes), but not function name, filename or line number:

Exception in thread "main" java.lang.IllegalArgumentException: Parameter declaration some-error should be a vector
        at clojure.core$assert_valid_fdecl.invoke(core.clj:6567)
        at clojure.core$sigs.invoke(core.clj:220)
        at clojure.core$defn.doInvoke(core.clj:294)
        at clojure.lang.RestFn.invoke(RestFn.java:467)
        at clojure.lang.Var.invoke(Var.java:427)
        at clojure.lang.AFn.applyToHelper(AFn.java:172)
        at clojure.lang.Var.applyTo(Var.java:532)
        at clojure.lang.Compiler.macroexpand1(Compiler.java:6366)
        at clojure.lang.Compiler.macroexpand(Compiler.java:6427)
        at clojure.lang.Compiler.eval(Compiler.java:6495)
        at clojure.lang.Compiler.load(Compiler.java:6952)
        at clojure.lang.Compiler.loadFile(Compiler.java:6912)
        at clojure.main$load_script.invoke(main.clj:283)
        at clojure.main$init_opt.invoke(main.clj:288)
        at clojure.main$initialize.invoke(main.clj:316)
        at clojure.main$null_opt.invoke(main.clj:349)
        at clojure.main$main.doInvoke(main.clj:427)
        at clojure.lang.RestFn.invoke(RestFn.java:421)
        at clojure.lang.Var.invoke(Var.java:419)
        at clojure.lang.AFn.applyToHelper(AFn.java:163)
        at clojure.lang.Var.applyTo(Var.java:532)
        at clojure.main.main(main.java:37)


 Comments   
Comment by Andrei Kleschinski [ 22/Feb/13 5:39 AM ]

Proposed patch for issue
Process exceptions in macroexpand1 and wraps them in CompilerException with source,line,column information.

Also patch adds quotes around invalid symbol name in error message to make them more distinguishable from rest of message.

Comment by Andy Fingerhut [ 01/Mar/13 9:32 AM ]

Patch 0001-CLJ-1169-proposed-patch.patch dated Feb 22 2013 causes several tests to fail. Run "./antsetup.sh" then "ant" to see which ones (or "mvn package").

Comment by Andrei Kleschinski [ 01/Mar/13 10:25 AM ]

Fix for failed unit-tests

Comment by Stuart Halloway [ 27/Jun/14 2:40 PM ]

Andrei, can you please sign the CA (e-form at http://clojure.org/contributing) so that we can consider this patch?

Thanks!

Comment by Andrei Kleschinski [ 27/Jun/14 3:05 PM ]

Ok, I have signed the CA.

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

I can confirm that Andrei has signed the CA. Back in Vetted.





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

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

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

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

 Description   

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

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

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

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



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

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

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

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

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

I found these problems via the following reasoning:

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

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

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

Questions addressed, back to Vetted.





[CLJ-1250] Reducer (and folder) instances hold onto the head of seqs Created: 03/Sep/13  Updated: 27/Jun/14

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

Type: Defect Priority: Critical
Reporter: Christophe Grand Assignee: Unassigned
Resolution: Unresolved Votes: 9
Labels: memory, reducers

Attachments: Text File after-change.txt     Text File before-change.txt     Text File CLJ-1250-20131211.patch     Text File CLJ-1250-AllInvokeSites-20140204.patch     Text File CLJ-1250-AllInvokeSites-20140320.patch    
Patch: Code
Approval: Vetted

 Description   

Problem Statement
A shared function #'clojure.core.reducers/reducer holds on to the head of a reducible collection, causing it to blow up when the collection is a lazy sequence.

Cause: #'reducer closes over a collection when in order to reify CollReduce, and the closed-over is never cleared. When code attempts to reduce over this anonymous transformed collection, it will realize the tail while the head is stored in the closed-over.

Reproduction steps:
Compare the following calls:

(time (reduce + 0 (map identity (range 1e8))))
(time (reduce + 0 (r/map identity (range 1e8))))

The second call should fail on a normal or small heap.

(If reducers are faster than seqs, increase the range.)

Patch
CLJ-1250-AllInvokeSites-20140320.patch takes Approach #2

Approaches:

1) Reimplement the #'reducer (and #'folder) transformation fns similar to the manner that Christophe proposes here:

(defrecord Reducer [coll xf])

(extend-protocol 
  clojure.core.protocols/CollReduce
  Reducer
      (coll-reduce [r f1]
                   (clojure.core.protocols/coll-reduce r f1 (f1)))
      (coll-reduce [r f1 init]
                   (clojure.core.protocols/coll-reduce (:coll r) ((:xf r) f1) init)))

(def rreducer ->Reducer) 

(defn rmap [f coll]
  (rreducer coll (fn [g] 
                   (fn [acc x]
                     (g acc (f x))))))

Advantages: Relatively non-invasive change.
Disadvantages: Not evident code. Additional protocol dispatch, though only incurred once

2) Clear the reference to 'this' on the stack just before a tail call occurs

When a callee is in return position, clear the local variable reference to 'this' in the caller's stack frame, which will make the caller and all its closed-overs eligible for reclamation.

Patch 1211 takes this approach for InvokeExpr call sites in return position. Patch 1214 takes the same approach for InvokeExprs and also static and instance interop calls.

Here is the code that performs the clearing excerpted from the 1214 patch:

void emitClearThis(GeneratorAdapter gen) {
		gen.visitInsn(Opcodes.ACONST_NULL);
		gen.visitVarInsn(Opcodes.ASTORE, 0);
	}

Tail calls wrapped inside a try/catch/finally clause cannot have 'this' cleared, because closed-overs/locals may need to be emitted for exception handling blocks. Both patches consider and handle this edge case.

Advantages: Fixes this case with no user code changes. Enables GC to do reclaim closed-overs references earlier.
Disadvantages: A compiler change.

3) Alternate approach

from Christophe Grand:
Another way would be to enhance the local clearing mechanism to also clear "this" but it's complex since it may be needed to keep a reference to "this" for caches long after obvious references to "this" are needed.

Advantages: Fine-grained
Disadvantages: Complex, invasive, and the compiler is hard to hack on.

Mitigations
Avoid reducing on lazy-seqs and instead operate on vectors / maps, or custom reifiers of CollReduce or CollFold. This could be easier with some implementations of common collection functions being available (like iterate and partition).

See https://groups.google.com/d/msg/clojure-dev/t6NhGnYNH1A/2lXghJS5HywJ for previous discussion.



 Comments   
Comment by Gary Fredericks [ 03/Sep/13 8:53 AM ]

Fixed indentation in description.

Comment by Ghadi Shayban [ 11/Dec/13 11:08 PM ]

Adding a patch that clears "this" before tail calls. Verified that Christophe's repro case is fixed.

Will upload a diff of the bytecode soon.

Any reason this juicy bug was taken off 1.6?

Comment by Ghadi Shayban [ 11/Dec/13 11:17 PM ]

Here's the bytecode for the clojure.core.reducers/reducer reify before and after the change... Of course a straight diff isn't useful because all the line numbers changed. Kudos to Gary Trakhman for the no.disassemble lein plugin.

Comment by Christophe Grand [ 12/Dec/13 6:58 AM ]

Ghadi, I'm a bit surprised by this part of the patch: was the local clearing always a no-op here?

-		if(context == C.RETURN)
+		if(shouldClear)
 			{
-			ObjMethod method = (ObjMethod) METHOD.deref();
-			method.emitClearLocals(gen);
+                            gen.visitInsn(Opcodes.ACONST_NULL);
+                            gen.visitVarInsn(Opcodes.ASTORE, 0);
 			}

The problem with this approach (clear this on tail call) is that it adds yet another special case. To me the complexity stem from having to keep this around even if the user code doesn't refer to it.

Comment by Ghadi Shayban [ 12/Dec/13 7:19 AM ]

Thank you - I failed to mention this in the commit message: it appears that emitClearLocals() belonging to both ObjMethod and FnMethod (its child) are empty no-ops. I believe the actual local clearing is on line 4855.

I agree re: another special case in the compiler.

Comment by Alex Miller [ 12/Dec/13 8:56 AM ]

Ghadi re 1.6 - this ticket was never in the 1.6 list, it has not yet been vetted by Rich but is ready to do so when we open up again after 1.6.

Comment by Ghadi Shayban [ 12/Dec/13 8:59 AM ]

Sorry I confused the critical list with the Rel1.6 list.

Comment by Ghadi Shayban [ 14/Dec/13 11:16 AM ]

New patch 20131214 that handles all tail invoke sites (InvokeExpr + StaticMethodExpr + InstanceMethodExpr). 'StaticInvokeExpr' seems like an old remnant that had no active code path, so that was left as-is.

The approach taken is still the same as the original small patch that addressed only InvokeExpr, except that it is now using a couple small helpers. The commit message has more details.

Also a 'try' block with no catch or finally clause now becomes a BodyExpr. Arguably a user error, historically accepted, and still accepted, but now they are a regular BodyExpr, instead of being wrapped by a the no-op try/catch mechanism. This second commit can be optionally discarded.

With this patch on my machine (4/8 core/thread Ivy Bridge) running on bare clojure.main:
Christophe's test cases both run i 3060ms on a artificially constrained 100M max heap, indicating a dominant GC overhead. (But they now both work!)

When max heap is at a comfortable 2G the reducers version outpaces the lazyseq at 2100ms vs 2600ms!

Comment by Ghadi Shayban [ 13/Jan/14 10:48 AM ]

Updating stale patch after latest changes to master. Latest is CLJ-1250-AllInvokeSites-20140113

Comment by Ghadi Shayban [ 04/Feb/14 3:50 PM ]

Updating patch after murmur changes

Comment by Tassilo Horn [ 13/Feb/14 4:52 AM ]

Ghadi, I suffer from the problem of this issue. Therefore, I've applied your patch CLJ-1250-AllInvokeSites-20140204.patch to the current git master. However, then I get lots of "java.lang.NoSuchFieldError: array" errors when the clojure tests are run:

     [java] clojure.test-clojure.clojure-set
     [java] 
     [java] java.lang.NoSuchFieldError: array
     [java] 	at clojure.core.protocols$fn__6026.invoke(protocols.clj:123)
     [java] 	at clojure.core.protocols$fn__5994$G__5989__6003.invoke(protocols.clj:19)
     [java] 	at clojure.core.protocols$fn__6023.invoke(protocols.clj:147)
     [java] 	at clojure.core.protocols$fn__5994$G__5989__6003.invoke(protocols.clj:19)
     [java] 	at clojure.core.protocols$seq_reduce.invoke(protocols.clj:31)
     [java] 	at clojure.core.protocols$fn__6017.invoke(protocols.clj:48)
     [java] 	at clojure.core.protocols$fn__5968$G__5963__5981.invoke(protocols.clj:13)
     [java] 	at clojure.core$reduce.invoke(core.clj:6213)
     [java] 	at clojure.set$difference.doInvoke(set.clj:61)
     [java] 	at clojure.lang.RestFn.invoke(RestFn.java:442)
     [java] 	at clojure.test_clojure.clojure_set$fn__1050$fn__1083.invoke(clojure_set.clj:109)
     [java] 	at clojure.test_clojure.clojure_set$fn__1050.invoke(clojure_set.clj:109)
     [java] 	at clojure.test$test_var$fn__7123.invoke(test.clj:704)
     [java] 	at clojure.test$test_var.invoke(test.clj:704)
     [java] 	at clojure.test$test_vars$fn__7145$fn__7150.invoke(test.clj:721)
     [java] 	at clojure.test$default_fixture.invoke(test.clj:674)
     [java] 	at clojure.test$test_vars$fn__7145.invoke(test.clj:721)
     [java] 	at clojure.test$default_fixture.invoke(test.clj:674)
     [java] 	at clojure.test$test_vars.invoke(test.clj:718)
     [java] 	at clojure.test$test_all_vars.invoke(test.clj:727)
     [java] 	at clojure.test$test_ns.invoke(test.clj:746)
     [java] 	at clojure.core$map$fn__2665.invoke(core.clj:2515)
     [java] 	at clojure.lang.LazySeq.sval(LazySeq.java:40)
     [java] 	at clojure.lang.LazySeq.seq(LazySeq.java:49)
     [java] 	at clojure.lang.Cons.next(Cons.java:39)
     [java] 	at clojure.lang.RT.boundedLength(RT.java:1655)
     [java] 	at clojure.lang.RestFn.applyTo(RestFn.java:130)
     [java] 	at clojure.core$apply.invoke(core.clj:619)
     [java] 	at clojure.test$run_tests.doInvoke(test.clj:761)
     [java] 	at clojure.lang.RestFn.applyTo(RestFn.java:137)
     [java] 	at clojure.core$apply.invoke(core.clj:617)
     [java] 	at clojure.test.generative.runner$run_all_tests$fn__527.invoke(runner.clj:255)
     [java] 	at clojure.test.generative.runner$run_all_tests$run_with_counts__519$fn__523.invoke(runner.clj:251)
     [java] 	at clojure.test.generative.runner$run_all_tests$run_with_counts__519.invoke(runner.clj:251)
     [java] 	at clojure.test.generative.runner$run_all_tests.invoke(runner.clj:253)
     [java] 	at clojure.test.generative.runner$test_dirs.doInvoke(runner.clj:304)
     [java] 	at clojure.lang.RestFn.applyTo(RestFn.java:137)
     [java] 	at clojure.core$apply.invoke(core.clj:617)
     [java] 	at clojure.test.generative.runner$_main.doInvoke(runner.clj:312)
     [java] 	at clojure.lang.RestFn.invoke(RestFn.java:408)
     [java] 	at user$eval564.invoke(run_tests.clj:3)
     [java] 	at clojure.lang.Compiler.eval(Compiler.java:6657)
     [java] 	at clojure.lang.Compiler.load(Compiler.java:7084)
     [java] 	at clojure.lang.Compiler.loadFile(Compiler.java:7040)
     [java] 	at clojure.main$load_script.invoke(main.clj:274)
     [java] 	at clojure.main$script_opt.invoke(main.clj:336)
     [java] 	at clojure.main$main.doInvoke(main.clj:420)
     [java] 	at clojure.lang.RestFn.invoke(RestFn.java:408)
     [java] 	at clojure.lang.Var.invoke(Var.java:379)
     [java] 	at clojure.lang.AFn.applyToHelper(AFn.java:154)
     [java] 	at clojure.lang.Var.applyTo(Var.java:700)
     [java] 	at clojure.main.main(main.java:37)
Comment by Ghadi Shayban [ 13/Feb/14 8:23 AM ]

Can you give some details about your JVM/environment that can help reproduce? I'm not encountering this error.

Comment by Tassilo Horn [ 13/Feb/14 9:41 AM ]

Sure. It's a 64bit ThinkPad running GNU/Linux.

% java -version
java version "1.7.0_51"
OpenJDK Runtime Environment (IcedTea 2.4.5) (ArchLinux build 7.u51_2.4.5-1-x86_64)
OpenJDK 64-Bit Server VM (build 24.51-b03, mixed mode)
Comment by Ghadi Shayban [ 13/Feb/14 10:19 AM ]

Strange, that is exactly my mail env, OpenJDK7 on Arch, 64-bit. I have also tested on JDK 6/7/8 on OSX mavericks. Are you certain that the git tree is clean besides the patch? (Arch users unite!)

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

Yes, the tree is clean. But now I see that I get the same error also after resetting to origin/master, so it's not caused by your patch at all. Oh, now the error vanished after doing a `mvn clean`! So problem solved.

Comment by Nicola Mometto [ 19/Feb/14 12:32 PM ]

Ghandi, FnExpr.parse should bind IN_TRY_BLOCK to false before analyzing the fn body, consider the case

(try (do something (fn a [] (heap-consuming-op a))) (catch Exception e ..))

Here in the a function the this local will never be cleared even though it's perfectly safe to.
Admittedly this is an edge case but we should cover this possibility too.

Comment by Ghadi Shayban [ 19/Feb/14 2:06 PM ]

You may have auto-corrected my name to Ghandi instead of Ghadi. I wish I were that wise =)

I will update the patch for FnExpr (that seems reasonable), but maybe after 1.6 winds down and the next batch of tickets get scrutiny. It would be nice to get input on a preferred approach from Rich or core after it gets vetted – or quite possibly not vetted.

Comment by Nicola Mometto [ 19/Feb/14 6:11 PM ]

hah, sorry for the typo on the name

Seems reasonable to me, in the meantime I just pushed to tools.analyzer/tools.emitter complete support for "this" clearing, I'll test this a bit in the next few days to make sure it doesn't cause unexpected problems.

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

Patch CLJ-1250-AllInvokeSites-20140204.patch no longer applies cleanly to latest master as of Feb 23, 2014. It did on Feb 14, 2014. Most likely some of its context lines are changed by the commit to Clojure master on Feb 23, 2014 – I haven't checked in detail.

Comment by Ghadi Shayban [ 20/Mar/14 4:39 PM ]

Added a patch that 1) applies cleanly, 2) binds the IN_TRY_EXPR to false initially when analyzing FnExpr and 3) uses RT.booleanCast





[CLJ-1297] try to catch using - instead of _ in filenames so the compiler can give a better error message for people who don't know that you need to use _ in file names Created: 19/Nov/13  Updated: 27/Jun/14

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

Type: Enhancement Priority: Major
Reporter: Kevin Downey Assignee: Unassigned
Resolution: Unresolved Votes: 11
Labels: compiler, errormsgs

Attachments: File better-error-messages-for-require.diff    
Patch: Code and Test
Approval: Screened

 Description   

Screener's Note: This works as advertised, but I have reservations about the approach. We could accept the patch as-is, or a much simpler patch that handles the only important (IMO) case: a-b-c to a_b_c – without generating and testing for unlikely errors like a-b_c. Please advise.

Problem: Clojure requires the files that back a namespace that has dashes in it to have the dashes replaced with underscores on the filesystem (ie a.b_c.clj for namespace a.b-c). If you require a file that has been mistakenly saved as b-c.clj instead, you will get an error message:

Exception in thread "main" java.io.FileNotFoundException: Could not locate a/b_c__init.class or a/b_c.clj on classpath:
	at clojure.lang.RT.load(RT.java:443)
	at clojure.lang.RT.load(RT.java:411)
	at clojure.core$load$fn__5018.invoke(core.clj:5530)
	at clojure.core$load.doInvoke(core.clj:5529)
	at clojure.lang.RestFn.invoke(RestFn.java:408)
	at clojure.core$load_one.invoke(core.clj:5336)
	at clojure.core$load_lib$fn__4967.invoke(core.clj:5375)
	at clojure.core$load_lib.doInvoke(core.clj:5374)
	at clojure.lang.RestFn.applyTo(RestFn.java:142)
	at clojure.core$apply.invoke(core.clj:619)
	at clojure.core$load_libs.doInvoke(core.clj:5413)
	at clojure.lang.RestFn.applyTo(RestFn.java:137)
	at clojure.core$apply.invoke(core.clj:619)
	at clojure.core$require.doInvoke(core.clj:5496)

Proposed:

  • When loading the resource-root of lib throws a FileNotFoundException, the lib is analyzed...
  • ... if the lib was a name that would be munged, it examines the combinatorial explosion of munge candidates and .clj or .class files in the classpath ...
  • ... if any of these candidates exist, it informs the user of the file's existence, and that a change to that filename would lead to that resource being loaded.
  • ... if none of these candidates exist, it throws the original exception.

It also modifies clojure.lang.RT to expose the behavior around finding clj or class files from a resource root.

Patch: better-error-messages-for-require.diff



 Comments   
Comment by Joshua Ballanco [ 20/Nov/13 12:15 AM ]

A perhaps even better solution would be to simply allow the use of dashes in *.clj[s] filenames. I can't imagine the extra disk access per-namespace would be a huge performance burden, and (since dashes aren't allowed currently) I don't think there would be any issues with backwards compatibility.

Comment by Gary Fredericks [ 20/Nov/13 8:40 AM ]

It's worth mentioning the combinatorial explosion for namespaces with multiple dashes – if I (require 'foo-bar.baz-bang), should clojure search for all four possible filenames? Does the jvm have a way to search for files by regex or similar to avoid nasty degenerate cases (like (require 'foo-------------))?

Comment by Joshua Ballanco [ 20/Nov/13 11:08 AM ]

According to the docs, the FileSystem class's "getPathMatcher" method accepts path globs, so you'd merely have to replace each instance of "-" or "_" with "{-,_}". Actual runtime characteristics would likely depend on the underlying filesystem's implementation.

Comment by Alex Miller [ 20/Nov/13 12:02 PM ]

I don't think the FileSystem stuff applies when looking up classes on the classpath. Note that Java class names cannot contain "-".

Comment by Phil Hagelberg [ 21/Nov/13 12:05 PM ]

According to the spec, Java class names can't contain dashes (though IIRC OpenJDK and Oracle's JDK accept them anyway) but the requirement that Clojure source files have names which align with their AOT'd class file eqivalents is something we've imposed upon ourselves. Introducing the disconnect between .clj files and .class files makes way more sense than disconnecting namespaces and .clj files, but arguably it's too late to fix that mistake.

In any case a check for dashed files (resulting only in a more informative compiler error, not a more permissive compiler) which only triggers when a .clj file cannot be found imposes zero overhead in the case where things are already working.

Comment by scott tudd [ 09/Dec/13 2:19 PM ]

As Clojure seems to be idiomatic to have sometimes-dashed-namespace-and-function-names as opposed to the ubiquitous camelCaseFunctionNames in java ... I agree to have the compiler automagically handle 'knowing' to look in dir_struct AND dir-struct for requisite files.

or at the least print out a nice message explaining the quirk when files "can't" be found ... WHEN there are dashes and underscores involved... anything to aid in helping things "just work" as one would think they're supposed to.

Comment by Obadz [ 12/Dec/13 5:28 AM ]

I would have saved a few hours as well.

Comment by Alexander Redington [ 14/Feb/14 2:29 PM ]

This patch changes clojure.core/load such that:

  • When loading the resource-root of lib throws a FileNotFoundException, the lib is analyzed...
  • ... if the lib was a name that would be munged, it examines the combinatorial explosion of munge candidates and .clj or .class files in the classpath ...
  • ... if any of these candidates exist, it informs the user of the file's existance, and that a change to that filename would lead to that resource being loaded.
  • ... if none of these candidates exist, it throws the original exception.

It also modifies clojure.lang.RT to expose the behavior around finding clj or class files from a resource root.

Comment by Andy Fingerhut [ 20/Mar/14 1:16 PM ]

I do not know whether it handles all of the cases proposed in this discussion, but I encourage folks to check out the filename/namespace consistency checking in the latest Eastwood release (version 0.1.1) to see if it catches the cases they would hope to catch. It does a static check based on the files in a Leiningen project, nothing at run time. https://github.com/jonase/eastwood

Of course changes to Clojure itself to give warnings about such things can still be very useful, since not everyone will be using a 3rd party tool to check for such things.

Comment by Alex Miller [ 27/Jun/14 2:24 PM ]

Re the screener's note at the top, my preference would be for the simpler approach.





[CLJ-1274] Unable to set compiler options via system properties except for AOT compilation Created: 02/Oct/13  Updated: 27/Jun/14

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

Type: Defect Priority: Major
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: compiler

Attachments: Text File CLJ-1274.patch    
Patch: Code
Approval: Screened

 Description   

The code that converts JVM system properties into keys under the *compiler-options* var is present only inside the clojure.lang.Compile class. This is a problem when using a debugger inside an IDE and not AOT compiling; specifying -Dclojure.compiler.disable-locals-clearing=true has no effect here when it would be most useful!

Patch: CLJ-1274.patch
Screened by: Stu



 Comments   
Comment by Howard Lewis Ship [ 02/Oct/13 4:52 PM ]

Obviously, that's supposed to be *compiler-options*.

Comment by Howard Lewis Ship [ 02/Dec/13 4:03 PM ]

Changes initialization of *compiler-options* to occur statically inside Compiler; now available to all forms of Clojure, not just AOT compilation; however, the initial *compiler-options* value is now defined as a root binding, rather than a per-thread binding, which has slightly different semantics.

Comment by Stuart Halloway [ 27/Jun/14 1:45 PM ]

Patch is straightforward, marking screened.

I am left wondering if other options that are set only in Compile.java ought also to be moved.





[CLJ-700] contains? broken for transient collections Created: 01/Jan/11  Updated: 27/Jun/14

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

Type: Defect Priority: Major
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: None

Attachments: Java Source File 0001-Refactor-of-some-of-the-clojure-.java-code-to-fix-CL.patch     File clj-700-7.diff     File clj-700.diff     Text File clj-700-patch4.txt     Text File clj-700-patch6.txt    
Patch: Code and Test
Approval: Vetted

 Description   

Behavior with Clojure 1.6.0:

user=> (contains? (transient {:x "fine"}) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentArrayMap$TransientArrayMap  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient (hash-map :x "fine")) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentHashMap$TransientHashMap  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient [1 2 3]) 0)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentVector$TransientVector  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (contains? (transient #{:x}) :x)
IllegalArgumentException contains? not supported on type: clojure.lang.PersistentHashSet$TransientHashSet  clojure.lang.RT.contains (RT.java:724)
;; expected: true

user=> (:x (transient #{:x}))
nil
;; expected: :x

user=> (get (transient #{:x}) :x)
nil
;; expected: :x

Behavior with latest Clojure master as of Jun 27 2014 (same as Clojure 1.6.0) plus patch clj-700-7.diff. In all cases it matches the expected results shown in comments above:

user=> (contains? (transient {:x "fine"}) :x)
true
user=> (contains? (transient (hash-map :x "fine")) :x)
true
user=> (contains? (transient [1 2 3]) 0)
true
user=> (contains? (transient #{:x}) :x)
true
user=> (:x (transient #{:x}))
:x
user=> (get (transient #{:x}) :x) 
:x

Analysis by Alexander Redington: This is caused by expectations in clojure.lang.RT regarding the type of collections for some methods, e.g. contains() and getFrom(). Checking for contains looks to see if the instance passed in is Associative (a subinterface of PersistentCollection), or IPersistentSet.

This patch refactors several of the Clojure interfaces so that logic abstract from the issue of immutability is pulled out to a general interface (e.g. ISet, IAssociative), but preserves the contract specified (e.g. Associatives only return Associatives when calling assoc()).

With more general interfaces in place the contains() and getFrom() methods were then altered to conditionally use the general interfaces which are agnostic of persistence vs. transience. Includes tests in transients.clj to verify the changes fix this problem.

Questions on this approach from Stuart Halloway to Rich Hickey:

1. this represents working back from the defect to rethinking abstractions (good!). Does it go far enough?

2. what are good names for the interfaces introduced here?

Alex Miller: Should also keep an eye on CLJ-787 as it may have some collisions with this one.

Patch: clj-700-7.diff

One 'trailing whitespace' warning is perfectly normal when applying this patch to latest Clojure master as of Jun 27 2014, as shown below. This is simply because of carriage returns at the end of lines in file Associative.java. I know of no way to avoid such a warning without removing CRs from all Clojure source files (e.g. CLJ-1026):

% git am -s --keep-cr --ignore-whitespace < ~/clj/patches/clj-700-7.diff 
Applying: Refactor of some of the clojure .java code to fix CLJ-700.
/Users/admin/clj/latest-clj/clojure/.git/rebase-apply/patch:29: trailing whitespace.
public interface Associative extends IPersistentCollection, IAssociative{
warning: 1 line adds whitespace errors.
Applying: more CLJ-700: refresh to use hasheq


 Comments   
<
Comment by Herwig Hochleitner [ 01/Jan/11 8:01 PM ]

the same is also true for TransientVectors

{{(contains? (transient [1 2 3]) 0)}}

false

Comment by Herwig Hochleitner [ 01/Jan/11 8:25 PM ]

As expected, TransientSets have the same issue; plus an additional, probably related one.

(:x (transient #{:x}))

nil

(get (transient #{:x}) :x)

nil

Comment by Alexander Redington [ 07/Jan/11 2:07 PM ]

This is caused by expectations in clojure.lang.RT regarding the type of collections for some methods, e.g. contains() and getFrom(). Checking for contains looks to see if the instance passed in is Associative (a subinterface of PersistentCollection), or IPersistentSet.

This patch refactors several of the Clojure interfaces so that logic abstract from the issue of immutability is pulled out to a general interface (e.g. ISet, IAssociative), but preserves the contract specified (e.g. Associatives only return Associatives when calling assoc()).

With more general interfaces in place the contains() and getFrom() methods were then altered to conditionally use the general interfaces which are agnostic of persistence vs. transience. Includes tests in transients.clj to verify the changes fix this problem.

Comment by Stuart Halloway [ 28/Jan/11 10:35 AM ]

Rich: Patch doesn't currently apply, but I would like to get your take on approach here. In particular:

  1. this represents working back from the defect to rethinking abstractions (good!). Does it go far enough?
  2. what are good names for the interfaces introduced here?
Comment by Alexander Redington [ 25/Mar/11 7:44 AM ]

Rebased the patch off the latest pull of master as of 3/25/2011, it should apply cleanly now.

Comment by Stuart Sierra [ 17/Feb/12 2:59 PM ]

Latest patch does not apply as of f5bcf647

Comment by Andy Fingerhut [ 17/Feb/12 5:59 PM ]