<< Back to previous view

[CLJ-1270] Print control for Java collection types Created: 30/Sep/13  Updated: 18/Oct/13  Resolved: 18/Oct/13

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

Type: Enhancement Priority: Critical
Reporter: Alex Miller Assignee: Stuart Halloway
Resolution: Duplicate Votes: 0
Labels: None

Attachments: Text File clj-1270-1.txt    
Approval: Vetted

 Description   

This feature is described at:
http://dev.clojure.org/display/design/Print+Control+for+Java+Collection+Types



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

clj-1270-1.txt is just a quick swipe at changing the printing behavior for Java objects with classes implementing the java.util.Queue interface, so pr/pr-str shows them similarly to how they show Clojure lists and seqs. java.util.Set and java.util.List already pr/pr-str similarly to Clojure lists, vectors, or sets.

No change is made in this patch to the behavior of print/print-str, or to how other objects are printed whose classes implement java.util.Collection, but not also one of java.util.Set, .List, or .Queue.

A few tests are added to verify the desired behavior when print-length is set to a value other than nil.

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

Just a comment: Clojure 1.5.1's behavior regarding print-readably and Java collections does seem a bit odd, perhaps backwards:

user=> (import '[java.util HashSet])
java.util.HashSet
user=> (def x (HashSet. (range 10)))
#'user/x
user=> (binding [*print-readably* true] (pr-str x))
"#{0 1 2 3 4 5 6 7 8 9}"
user=> (binding [*print-readably* false] (pr-str x))
"#<HashSet [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]>"
Comment by Alex Miller [ 18/Oct/13 7:59 AM ]

Rich and Stu said this should be in scope for 1.6. Rich said that Stu would likely do the work.

Comment by Stuart Halloway [ 18/Oct/13 8:41 AM ]

This was actually implemented in 1.5. Andy's suggestion about Queue (see patch and https://groups.google.com/forum/#!topic/clojure-dev/q7h4QJCHEvw) could be considered as a separate feature request.

Comment by Stuart Halloway [ 18/Oct/13 8:42 AM ]

This work was done in commit 92b4fc76e59e68d3bdae4ebd5b8deec915cb7ab5

Comment by Alex Miller [ 18/Oct/13 9:21 AM ]

Ah, sweet - thanks Stu!





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

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

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

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

 Description   

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

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

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

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

Patch: CLJ-1154.patch



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

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

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

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

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

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

(ns foo)

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

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





[CLJ-1150] Make some PersistentVector, SubVector internals public Created: 19/Jan/13  Updated: 01/Mar/13  Resolved: 04/Feb/13

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

Type: Enhancement Priority: Major
Reporter: Michał Marczyk Assignee: Michał Marczyk
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-Make-some-PersistentVector-s-and-APersistentVector.S.patch    
Patch: Code
Approval: Ok

 Description   

This should help with RRBT-PV interop.






[CLJ-1140] {:as x} destructuring with an empty list raises exception Created: 30/Dec/12  Updated: 01/Mar/13  Resolved: 01/Feb/13

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

Type: Defect Priority: Major
Reporter: Toby Crawley Assignee: Unassigned
Resolution: Completed Votes: 3
Labels: None

Attachments: File empty-list-destructuring-CLJ-1140-12.30.12.diff    
Patch: Code and Test
Approval: Ok

 Description   
user=> (clojure-version)
"1.4.0"
user=> (let [{:as x} '()] x)
{}

...

user=> (clojure-version)
"1.5.0-RC1"
user=> (let [{:as x} '()] x)
IllegalArgumentException No value supplied for key: null  clojure.lang.PersistentHashMap.create (PersistentHashMap.java:77)

The bug was introduced by a change[1] to support duplicate keys in map
destructuring. Using PersistentHashMap/create here introduces the above
bug, since it does not properly handle empty lists.

[1]: https://github.com/clojure/clojure/commit/93c795fe10ee5c92a36b6ec6373b3c80a31135c4



 Comments   
Comment by Toby Crawley [ 02/Jan/13 11:02 AM ]

There's been some discussion on clojure-dev around this issue: https://groups.google.com/d/topic/clojure-dev/qdDRNfEVfQ8/discussion

Comment by Toby Crawley [ 01/Feb/13 10:05 AM ]

An issue I brought up in the email thread is consistency: vector binding works with anything nthable, map binding works with anything associative. With my current patch (empty-list-destructuring-CLJ-1140-12.30.12.diff), only ISeqs are supported for kwarg map binding.

I'd prefer it work with anything seqable, and can provide a patch that does that. I would go ahead and do so, but now that this ticket is now Approval: OK, I didn't want to alter what had been OK'ed. Let me know if you want a patch that adds support for anything seqable.





[CLJ-1135] Add previous changelog items back to changes.md Created: 19/Dec/12  Updated: 02/Feb/13  Resolved: 02/Feb/13

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

Type: Enhancement Priority: Trivial
Reporter: Cosmin Stejerean Assignee: Stuart Halloway
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File add-changelog-items.patch    
Patch: Code
Approval: Vetted




[CLJ-1116] More REPL-friendly 'ns macro Created: 29/Nov/12  Updated: 01/Mar/13  Resolved: 11/Dec/12

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

Type: Defect Priority: Major
Reporter: Laurent Petit Assignee: Unassigned
Resolution: Completed Votes: 3
Labels: None

Attachments: File dynamic-ns-patch2.diff    
Patch: Code
Approval: Ok

 Description   

The 'ns macro is not as dynamic as it could be.
For instance, the following line typed in a repl (ns a)(ns b (:require a)) currently (1.4, 1.3, etc.) fails with an exception because the (:require a) call tries to reach the filesystem for file a.clj or a__init.class.

The attached patch ( dynamic-ns-patch2.diff ) allows a successful call to (ns a) behave the same as a successful call to (require 'a), adding namespace a to the list of loaded-libs.

Discussion on googlegroup's mailing list: http://groups.google.com/group/clojure-dev/browse_thread/thread/fb231e6fab4a5ad



 Comments   
Comment by Rich Hickey [ 29/Nov/12 9:31 AM ]

screeners, please make sure this has tests before passing on

Comment by Laurent Petit [ 29/Nov/12 10:41 AM ]

Tests added

Comment by Timothy Baldridge [ 30/Nov/12 11:35 AM ]

Patch applies, fixes the bug. All tests pass.

Screened

Comment by Herwig Hochleitner [ 03/Dec/12 7:52 AM ]

I see this has already been screened, but FWIW I tested the repl with this patch and the behavior works for me.





[CLJ-1111] Loops returning primtives are boxed even in return position Created: 21/Nov/12  Updated: 22/Dec/12  Resolved: 22/Dec/12

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

Type: Defect Priority: Major
Reporter: Christophe Grand Assignee: Christophe Grand
Resolution: Completed Votes: 3
Labels: None

Attachments: File prim-loop.diff    
Patch: Code
Approval: Ok

 Description   

Reported here: https://groups.google.com/d/topic/clojure/atoFzbyuyos/discussion

(defn first-bit?
  {:inline (fn [n] `(== 1 (clojure.lang.Numbers/and ~n, 1)) )}
  [^long n]
  (== 1 (clojure.lang.Numbers/and n, 1)))

(defn exp-int
  ^double [^double x, ^long c]
  (loop [result 1.0, factor x, c c]
    (if (> c 0)
        (recur
         (if (first-bit? c)
           (* result factor)
           result),
         (* factor factor),
         (bit-shift-right c 1))
      result)))

Last lines of the Java bytecode of `exp-int`:

59 dload 5;               /* result */
61 invokestatic 40;       /* java.lang.Double java.lang.Double.valueOf(double c) */
64 checkcast 85;          /* java.lang.Number */
67 invokevirtual 89;      /* double doubleValue() */
70 dreturn;

The compiler doesn't currently infer the primitive type as soon as there is a recur:

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

Patch attached.



 Comments   
Comment by Stuart Halloway [ 25/Nov/12 7:12 PM ]

Tests pass, code looks reasonable. Would appreciate additional review.

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

Tests also pass here. Looked through the code and played with a patched version of Clojure. I can't see a problem with it.

Comment by Christophe Grand [ 27/Nov/12 4:40 AM ]

FYI, gvec.clj has two loops which return primitives and thus was formerly boxed.





[CLJ-1106] Broken equality for sets Created: 09/Nov/12  Updated: 13/Sep/13  Resolved: 08/Feb/13

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

Type: Defect Priority: Major
Reporter: Justin Kramer Assignee: Aaron Bedra
Resolution: Completed Votes: 4
Labels: None

Attachments: Text File 0001-CLJ-1106-Use-Util.hasheq-in-hashCode-for-persistent-.patch     Text File 0001-Fixing-set-equality.patch     File setequals.diff    
Patch: Code and Test
Approval: Ok

 Description   

Equality for sets is not consistent with other persistent collections when the hashCode differs:

(= {(Integer. -1) :foo} {(Long. -1) :foo})
=> true

(= [(Integer. -1)] [(Long. -1)])
=> true

(= #{(Integer. -1)} #{(Long. -1)})
=> false

Attached is what I believe to be a basic fix. It removes a hashCode check from APersistentSet.equals. A similar check was removed from APersistentMap in the following commit:

https://github.com/clojure/clojure/commit/b5f5ba2e15dc2f20e14e05141f7de7c6a3d91179

With this patch, set equality works as expected and all tests pass.

My understanding of the implications of this change are limited, so someone more knowledgeable would need to chime in about its correctness.



 Comments   
Comment by Andy Fingerhut [ 09/Nov/12 9:48 AM ]

Can you add some unit tests that fail with the current code but succeed with the change?

Comment by Justin Kramer [ 09/Nov/12 10:24 AM ]

Revised patch with unit test that fails in master and succeeds with patch

Comment by Timothy Baldridge [ 03/Dec/12 9:04 AM ]

vetting

Comment by Gary Fredericks [ 29/Jan/13 5:32 PM ]

This came up when we were trying to diff two datasets by putting them in sets and comparing. We had Integers because they came back from JDBC that way.

Comment by Aaron Bedra [ 29/Jan/13 5:35 PM ]

I'm going to take a look and try to get this shipped. It hit us and I would love to to see it in 1.5

Comment by Paul Stadig [ 29/Jan/13 7:14 PM ]

I applied the patch on master (ca465d6d) and it applied cleanly and fixes the issue.

Comment by Bo Jeanes [ 30/Jan/13 11:19 AM ]

Instead of removing m.hashCode() != hashCode(), perhaps we should use `Util.hasheq()` instead. It already seems to special case numbers (https://github.com/clojure/clojure/blob/f48d024e97/src/jvm/clojure/lang/Util.java#L164-L172) and won't have the potential performance impact that skipping hashCode checks could bring.

Comment by Bo Jeanes [ 30/Jan/13 11:39 AM ]

Attaching a patch with an alternate fix for this issue that does not skip hashCode checking.

It passes all existing tests and fixes this issue.

I want to benchmark the difference, too.

Comment by Bo Jeanes [ 30/Jan/13 1:32 PM ]

On further thought and comparison to APersistentMap, I'm not sure if that's the best use of Util.hasheq(). I can't find good reference on the semantic differences and am not familiar enough with the Clojure source to infer it, yet.

Comment by Aaron Bedra [ 02/Feb/13 12:33 PM ]

Bo, I don't see you listed on the contributors list. Did you send in a CA?

Comment by Aaron Bedra [ 02/Feb/13 1:36 PM ]

Both of the patches above are not complete. APersistentSet equiv calls equals. APersistentMap has two separate ways of handling this. This is the path that the fix should take.

Comment by Aaron Bedra [ 02/Feb/13 2:37 PM ]

This patch addresses the difference between equals and equiv.

Comment by Stuart Halloway [ 04/Feb/13 9:55 PM ]

The Set equality code currently in Clojure uses .hashCode to quickly bail out of comparisons in both .equals and .equiv. This feels wrong since .equals and .equiv presume different hashes. The map equality code avoids the problem by not checking any hash.

Aaron's 2/13 patch makes set equality work like map equality, not checking the hash. (I think a much shorter patch that accomplishes the same thing merely drops the call to hash.)

It seems to me a separate problem that both hash and set are broken for Java .equals rules, because of the equiv-based handling of their keys.

I think this needs more consideration.

Comment by Stuart Halloway [ 05/Feb/13 8:57 AM ]

Notes from discussion with Rich:

1. Aaron's 2/2/13 patch, which mimics map logic into set, is the preferred approach.

2. The broader issue I was worried about is the semantic collision between Map's containsKey and Associative's containsKey. This is out of scope here, and perhaps ever.

Comment by Aaron Bedra [ 05/Feb/13 9:38 AM ]

Any chance this will make 1.5?

Comment by Gary Fredericks [ 13/Sep/13 9:42 AM ]

This is apparently still present wrt BigInteger:

(let [n1 -5, n2 (BigInteger. "-5")]
  [(= n1 n2) (= #{n1} #{n2}) (= [n1] [n2])])
;; => [true false true]

Should this be a new ticket?

Comment by Gary Fredericks [ 13/Sep/13 9:48 AM ]

Created CLJ-1262.





[CLJ-1098] Extend CollFold and IKVReduce to nil Created: 31/Oct/12  Updated: 01/Mar/13  Resolved: 25/Jan/13

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

Type: Enhancement Priority: Minor
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Completed Votes: 3
Labels: None

Attachments: Text File 0001-CLJ-1098-Implement-IKVReduce-and-CollFold-for-nil.patch    
Patch: Code and Test
Approval: Ok

 Description   

Currently, reduce-kv and fold throw when used on nil, because their respective protocols don't extend to nil. This seems strange, since Clojure tends to handle nils gracefully where possible, especially in places where collections are expected.

See thread https://groups.google.com/d/topic/clojure/tGI8sIKQoh0/discussion



 Comments   
Comment by Herwig Hochleitner [ 31/Oct/12 8:44 PM ]

Attached patch with tests

Comment by Andy Fingerhut [ 14/Jan/13 11:04 AM ]

Another discussion thread: https://groups.google.com/forum/?fromgroups=#!topic/clojure/xPDDybYGvmc





[CLJ-1092] New function re-quote-replacement has incorrect :added metadata Created: 22/Oct/12  Updated: 22/Dec/12  Resolved: 22/Dec/12

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

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

Attachments: Text File fix-added-metadata-for-re-quote-replacement.txt    
Patch: Code
Approval: Ok

 Description   

I created the patch for CLJ-870 that added the function re-quote-replacement before Clojure 1.4 was released, and I was apparently hoping it would get into that release. It is in now, but incorrectly states that fn re-quote-replacement was added in 1.4.



 Comments   
Comment by Andy Fingerhut [ 22/Oct/12 9:01 PM ]

And before creating this patch, I did a diff between Clojure 1.4 and 1.5-beta1 source code, and verified that every other function with :added metadata that has been added since 1.4 says "1.5". Only re-quote-replacement was mislabeled in this way.

Comment by Christopher Redinger [ 27/Nov/12 3:42 PM ]

As you'd expect, this patch applies cleanly and improves the correctness of the documentation.





[CLJ-1091] update changes.md to include 1.5 changes Created: 22/Oct/12  Updated: 11/Dec/12  Resolved: 11/Dec/12

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

Type: Task Priority: Major
Reporter: Stuart Halloway Assignee: Timothy Baldridge
Resolution: Completed Votes: 0
Labels: None

Attachments: File changes-draft-v10.md     File changes-draft-v11.md     File changes-draft-v5.md    
Approval: Ok

 Comments   
Comment by Andy Fingerhut [ 15/Nov/12 11:01 AM ]

changes-draft-v1.md is not a patch, but an outline of a proposed new changes.md file for Clojure 1.5 with some of the content fleshed out. It still has lots of occurrences of "TBD" for "To Be Documented" in it.

It does mention every ticket that had a patch committed since Clojure 1.4.0 until Nov 15 2012.

I am hoping someone who is more knowledgeable of the "TBD" parts takes this and runs with it.

Comment by Andy Fingerhut [ 22/Nov/12 7:16 PM ]

changes-draft-v2.md is very similar to the earlier changes-draft-v1.md described above, but has a few additions.

Comment by Timothy Baldridge [ 26/Nov/12 2:41 PM ]

V3 of the draft...should be almost good to go. Someone with more info on reducers should take a look at that section as have yet to actually use them much.

Comment by Andy Fingerhut [ 26/Nov/12 8:47 PM ]

Removed V2 of the draft as Timothy's V3 had all of it and more.

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

Fleshed out the reducers section a bit.

Comment by Timothy Baldridge [ 27/Nov/12 8:53 AM ]

Alright, I think this is ready for a final review by someone besides me.

Comment by Christopher Redinger [ 28/Nov/12 12:48 PM ]

Editorial cleanup

Comment by Andy Fingerhut [ 28/Nov/12 12:56 PM ]

Christopher, isn't this file intended to be in Markdown format, not HTML?

Comment by Christopher Redinger [ 28/Nov/12 12:58 PM ]

uploading the correct md file

Comment by Andy Fingerhut [ 28/Nov/12 1:12 PM ]

changes-draft-v6.md same as v5, except for a couple of spelling corrections.

Comment by Timothy Baldridge [ 29/Nov/12 8:38 AM ]

Missing desc on when->. Fixed in v7 of the doc

Comment by Rich Hickey [ 02/Dec/12 6:37 PM ]

"1.1 Clojure 1.5 requires Java 1.6 or later"

did you mean "building Clojure 1.5"?

I don't know that anything requires 1.6

Comment by Rich Hickey [ 02/Dec/12 6:41 PM ]

test->, let-> when->

are now:

cond->, as-> and some->

Comment by Andy Fingerhut [ 02/Dec/12 6:49 PM ]

I'll put up an updated version soon, but that headline wasn't properly updated to match the later occurrence of it in the body, which is: "Clojure 1.5 reducers library requires Java 6 or later". Is it true that the ForkJoin library requires Java 6 or later? If not, how can it be made to work with Java 5?

Comment by Andy Fingerhut [ 02/Dec/12 8:42 PM ]

changes-draft-v8.md updates the table of contents headings to match those in the body, and updates names of new threading macros.

Comment by Steve Miner [ 03/Dec/12 9:40 AM ]

changes-draft-v8.md, section 2.4, needs an update for description of some->. The document says "logical true" where it should say "not nil". Same comment applies to some->>. The point is that false will thread through the forms. This is a change from the replaced when-> (in 1.5-beta1).

Comment by Timothy Baldridge [ 03/Dec/12 10:02 AM ]

updated to reflect changes to some-> and some->>

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

Sorry to nit pick, but there are two more "logical true" phrases in v9 that need to change to "not nil" in those some-> and some->> descriptions.

Comment by Timothy Baldridge [ 03/Dec/12 10:22 AM ]

Not a problem. Thanks for looking it over!

Comment by Stuart Halloway [ 03/Dec/12 1:30 PM ]

I find the following sentence a little vague: "Clojure 1.5 can still be compiled and run with Java 5, but the test suite will not pass due to the lack of support for ForkJoin."

The problem is the application of the "but" to both parts of the conjunction. Isn't the intent actually: "Clojure can run with Java version 1.5 or later. Running the Clojure build requires 1.6, in order to test features that work with ForkJoin."

Comment by Andy Fingerhut [ 03/Dec/12 1:52 PM ]

Stuart H, that is almost the intent, and probably close enough for this kind of documentation.

The full gory details are as follows:

Clojure can build and run with Java version 1.5 or later. A Java version 1.5 build of Clojure only works if you do not run the test suite, e.g. by using the command "ant jar". To build Clojure including running the test suite, or to use the new reducers library, requires Java 1.6 or later.

If the patch for CLJ-1109 is applied, the full gory details become simpler to state:

Clojure can build and run with Java version 1.5 or later. Using the new reducers library requires Java 1.6 or later.

Comment by Andy Fingerhut [ 03/Dec/12 1:59 PM ]

changes-draft-v11.md changes the description of Java 5 limitations to match the current gory details, and hopefully address Stuart H's concerns raised above.





[CLJ-1085] clojure.main/repl unconditionally refers REPL utilities into *ns* Created: 10/Oct/12  Updated: 20/Oct/12  Resolved: 20/Oct/12

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

Type: Defect Priority: Major
Reporter: Chas Emerick Assignee: Unassigned
Resolution: Completed Votes: 3
Labels: None

Attachments: File CLJ-1085.diff     File CLJ-1085-refactor.diff    
Patch: Code
Approval: Ok

 Description   

A number of vars from clojure.repl, clojure.java.javadoc, and clojure.pprint are unconditionally referred into *ns* by clojure.main/repl. This is fine when it is being used e.g. as the primary driver of a terminal-bound Clojure REPL, but other usages can end up bringing those utility vars into namespaces other than 'user. This can cause problems if clojure.main/repl is used to drive a REPL within namespaces that already have referred or interned vars with the same names as those utility vars, e.g.:

$ java -jar ~/.m2/repository/org/clojure/clojure/1.5.0-alpha6/clojure-1.5.0-alpha6.jar
Clojure 1.5.0-alpha6
user=> (ns foo)
nil
foo=> (defn pp [] "hi!")
#'foo/pp
foo=> (pp)
"hi!"
foo=> (clojure.main/repl)
foo=> (pp)
nil
nil
foo=> (defn pp [] "whoops")
CompilerException java.lang.IllegalStateException: pp already refers to: #'clojure.pprint/pp in namespace: foo, compiling:(NO_SOURCE_PATH:7:1)

Worse, nREPL uses clojure.main/repl (in large part to maximize the consistency of REPL behaviour across different Clojure versions), where each user expression is evaluated through a separate clojure.main/repl invocation. This leads to the same problems as above, but for every nREPL user, session, and expression (reported @ NREPL-31).

A simple fix for this is to perform these refers only if *ns* is 'user (which, AFAICT, was the only intended effect of CLJ-310, CLJ-454, and https://github.com/clojure/clojure/commit/04764db, the changes that added these automatic implicit refers to clojure.main/repl).



 Comments   
Comment by Chas Emerick [ 10/Oct/12 1:36 PM ]

Patch attached to only refer in the utility vars if in the user namespace.

Comment by Steve Miner [ 10/Oct/12 2:03 PM ]

It would probably be better to test with ns-name (as opposed to comparing strings):

(= 'user (ns-name ns))

Comment by Kevin Downey [ 10/Oct/12 2:18 PM ]

I don't follow how it is required to call `clojure.main/repl` for every input

Comment by Chas Emerick [ 10/Oct/12 2:19 PM ]

Gah, of course. :-/ Patch updated.

Comment by Chas Emerick [ 10/Oct/12 2:32 PM ]

@Kevin: It's not required, but I found it far more straightforward to not try to pretend that the underlying transport was stream-based when it's actually message-based. It also means that sessions can be very lightweight: unless code is being evaluated within a session, it is not occupying a thread, and takes up only as much space as its map of thread-local bindings.

Comment by Chas Emerick [ 15/Oct/12 5:10 AM ]

Based on discussion on clojure-dev, I have attached an alternative patch ({{CLJ-1085-refactor.diff}}), which:

  • breaks out the libspecs specifying the implicit refers into their own var (so that they can be consistently applied by other REPL implementations)
  • moves the default require of the libspecs to be invoked only when REPL is started from clojure.main
Comment by Stuart Halloway [ 19/Oct/12 2:12 PM ]

Screened and liked the second "refactor" patch.





[CLJ-1084] (object-array [1]) is ~3x slower than (object-array (rseq [1])) Created: 09/Oct/12  Updated: 20/Oct/12  Resolved: 20/Oct/12

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

Type: Defect Priority: Minor
Reporter: Paul Stadig Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: patch,

Attachments: Text File 0001-Make-PersistentVector-ChunkedSeq-implement-Counted.patch     Text File CLJ-1084-tests.patch    
Patch: Code and Test
Approval: Ok

 Description   

{{user=> (time (dotimes [_ 10000000] (object-array [1])))
"Elapsed time: 1178.116257 msecs"
nil
user=> (time (dotimes [_ 10000000] (object-array (rseq [1]))))
"Elapsed time: 422.42248 msecs"
nil}}

This appears to be because PersistentVector$ChunkedSeq does not implement Counted, so RT.count is iterating the ChunkedSeq to get its count.



 Comments   
Comment by Paul Stadig [ 09/Oct/12 2:11 PM ]

I don't believe this is Major priority, but I cannot edit the ticket after having created it.

Comment by Tassilo Horn [ 11/Oct/12 10:17 AM ]

This patch makes PersistentVector$ChunkedSeq implement Counted.

Performance before:

(let [v (vec (range 10000))
      vs (seq v)]
  (time (dotimes [_ 10000]
          (count v)))
  (time (dotimes [_ 10000]
          (count vs))))
;"Elapsed time: 0.862259 msecs"
;"Elapsed time: 7228.72486 msecs"

Performance after (with the patch):

(let [v (vec (range 10000))
      vs (seq v)]
  (time (dotimes [_ 10000]
          (count v)))
  (time (dotimes [_ 10000]
          (count vs))))
;"Elapsed time: 0.967301 msecs"
;"Elapsed time: 0.99391 msecs"

Also with Paul's test case.

Before:

(time (dotimes [_ 10000000] (object-array [1])))
;"Elapsed time: 1668.346997 msecs"
(time (dotimes [_ 10000000] (object-array (rseq [1]))))
;"Elapsed time: 662.820591 msecs"

After:

(time (dotimes [_ 10000000] (object-array [1])))
;"Elapsed time: 757.084577 msecs"
(time (dotimes [_ 10000000] (object-array (rseq [1]))))
;"Elapsed time: 680.602921 msecs"
Comment by Stuart Halloway [ 19/Oct/12 1:46 PM ]

Two patches to be applied together, the 10/19 patch adds tests and updates to latest test.generative.





[CLJ-1071] ExceptionInfo does no abstraction Created: 17/Sep/12  Updated: 21/Sep/12  Resolved: 21/Sep/12

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

Type: Defect Priority: Major
Reporter: Stuart Halloway Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File clj-1071-iexceptioninfo.patch    
Patch: Code and Test
Approval: Ok

 Description   

There oughtta be an interface



 Comments   
Comment by Stuart Sierra [ 18/Sep/12 8:44 AM ]

Screened.

Comment by Fogus [ 18/Sep/12 8:46 AM ]

Looks fine to me.





[CLJ-1070] PersistentQueue's hash function does not match its equality Created: 15/Sep/12  Updated: 20/Oct/12  Resolved: 20/Oct/12

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

Type: Defect Priority: Major
Reporter: Philip Potter Assignee: Philip Potter
Resolution: Completed Votes: 0
Labels: None

Attachments: File 001-fix-PersistentQueue-hash.clj     File 001-make-PersistentQueue-hash-match-equiv.diff     File 002-make-PersistentQueue-hash-match-equiv.diff    
Patch: Code and Test
Approval: Ok

 Description   

There are two issues:

1) PersistentQueue's hash function doesn't match its equiv function:

(def iq (conj clojure.lang.PersistentQueue/EMPTY (Integer. -1)))
(def lq (conj clojure.lang.PersistentQueue/EMPTY (Long. -1)))
[(= iq lq) (= (hash iq) (hash lq))]
;=> [true false]

2) PersistentQueue's hash function doesn't match PersistentVector's hash:

(def q (conj clojure.lang.PersistentQueue/EMPTY 1 2 3))
[(= [1 2 3] q) (= (hash [1 2 3]) (hash q))]
;=> [true false]



 Comments   
Comment by Philip Potter [ 15/Sep/12 1:52 PM ]

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

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

Attached patch 001-make-PersistentQueue-hash-match-equiv.diff, 15/Sep/12.

Comment by Philip Potter [ 15/Sep/12 4:56 PM ]

Attached 002-make-PersistentQueue-hash-match-equiv.diff, 15/Sep/12. This patch supercedes 001-make-PersistentQueue-hash-match-equiv.diff.

Replaced test code which calls to Util/hasheq with code which calls hash instead.

This patch has a minor conflict with my patch for CLJ-1059, since they both add an interface to PersistentQueue (IHashEq here, List for CLJ-1059).

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

Thanks for the patch, Philip, it looks good to me.

It really is a pity the implementations of hashCode and hasheq are duplicated. I wonder how important it is to extend Obj? Regardless, that's the approach PersistentQueue was already taking, so changing that would be outside the scope of this ticket anyway.

I can't apply this patch with "git am", but "patch -p 1" works fine. I'm hoping this is a configuration problem on my end, so I'm marking this screened.





[CLJ-1066] No dependency on jsr166y Created: 11/Sep/12  Updated: 20/Oct/12  Resolved: 20/Oct/12

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

Type: Defect Priority: Major
Reporter: Wolodja Wentland Assignee: Stuart Halloway
Resolution: Completed Votes: 4
Labels: patch
Environment:

$ java -version
java version "1.7.0_03"
OpenJDK Runtime Environment (IcedTea7 2.1.2) (7u3-2.1.2-2)
OpenJDK 64-Bit Server VM (build 22.0-b10, mixed mode)
$ lein version
Leiningen 2.0.0-preview10 on Java 1.7.0_03 OpenJDK 64-Bit Server VM


Attachments: Text File 0001-Don-t-AOT-compile-clojure.core.reducers.patch    
Patch: Code
Approval: Ok

 Description   

The Clojure 1.5.0-alpha4 jars that have been deployed to maven.org seem to have been compiled against JDK6 which causes
an exception if one tries to use reducers/fold.

— snip —
Setup:

user=> (require '[clojure.core.reducers :as r])
nil
user=> (def v (vec (range 10000)))
#'user/v

;; JDK7 + clojure 1.5.0-alpha4 from maven.org
;; → :dependencies [[org.clojure/clojure "1.5.0-alpha4"]] in project.clj

user=> (r/fold + (r/map inc v))
ClassNotFoundException jsr166y.ForkJoinTask java.net.URLClassLoader$1.run (URLClassLoader.java:366)

;; JDK7 + clojure 1.5.0-alpha4 from maven.org
;; → :dependencies [[org.clojure/clojure "1.5.0-alpha4"]
;; [org.codehaus.jsr166-mirror/jsr166y "1.7.0"]]

user=> (r/fold + (r/map inc v))
50005000

;; JDK7 + clojure 1.5.0-alpha4 locally compiled (mvn install) against OpenJDK7
;; → :dependencies [[org.clojure/clojure "1.5.0-alpha4"]] in project.clj

user=> (r/fold + (r/map inc v))
5000050000
— snip —

It would be wonderful if this issue could be fixed before the release of 1.5.0.

Have a nice day

Wolodja



 Comments   
Comment by Tassilo Horn [ 12/Sep/12 9:44 AM ]

That's really a nasty problem. I wrote the code that makes it possible to compile clojure with either a JDK7 and native ForkJoin or an older JDK + jsr166y. Unfortunately, if you build clojure with a JDK7, reducers' fold requires a JDK7 at runtime, and if you build clojure with a JDK <7 + jsr166y, fold also requires jsr166y at runtime.

The essential problem is that clojure is AOT-compiled to class-files and those are put into the release jars. Ordinary clojure libraries are distributed as jar files containing clj files that are compiled when loaded. There, the implemented approach works just fine. For example, again your example with 1.5.0-alpha4:

;; 1.5.0-master4 compiled with JDK6 + jsr166y running on a JDK7
user=> (require '[clojure.core.reducers :as r])
nil
user=> (def v (vec (range 10000)))
#'user/v
user=> (r/fold + (r/map inc v))
ClassNotFoundException jsr166y.ForkJoinTask  java.net.URLClassLoader$1.run (URLClassLoader.java:366)
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;; Now load the reducers.clj source code again, so that it
;; picks the right ForkJoin-impl for the current platform
(load-file "/home/horn/Repos/clj/clojure/src/clj/clojure/core/reducers.clj")
nil
user=> (r/fold + (r/map inc v))
50005000

So IMO, the best thing we can do is to omit AOT-compilation for reducers but to include only its clj file so that it'll be compiled automatically on the platform it eventually runs.

Comment by Tassilo Horn [ 12/Sep/12 11:55 AM ]

This patch removes the clojure.core.reducers namespace from the namespaces to be AOT compiled. So now this namespace will be JIT-compiled when being required, and at that point either the JDK7 ForkJoin classes or the jsr166y classes need to be there.

Comment by Stuart Halloway [ 21/Sep/12 1:31 PM ]

Rich: what is the right approach here?





[CLJ-1065] Allow duplicate set elements and map keys for all set and map constructors Created: 08/Sep/12  Updated: 04/Oct/12  Resolved: 04/Oct/12

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

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

Attachments: Text File clj-1065-do-map-constant-duplicate-key-checks-compile-time-only-v1.txt     Text File clj-1065-set-map-constructors-allow-duplicates-v2.txt    
Patch: Code and Test
Approval: Ok

 Description   

See description here http://dev.clojure.org/display/design/Allow+duplicate+map+keys+and+set+elements



 Comments   
Comment by Andy Fingerhut [ 08/Sep/12 1:19 AM ]

I think that clj-1065-set-map-constructors-allow-duplicates-v1.txt dated Sep 7 2012 updates Clojure for the behavior Rich recommends on the dev Wiki page in the description. I may have missed something, though. Perhaps the most questionable part is the way I've chosen to implement array-map always taking the last key's value. It is no less efficient than the PersistentArrayMap createWithCheck method, i.e. O(n^2).

Comment by Rich Hickey [ 08/Sep/12 7:10 AM ]

Thanks!

array-map is ok. I would prefer that the docs strings say "as if by repeated assoc" (or conj for sets). "eliminates" and "last" are less precise e.g. what if you pass equal things, but with different metadata, to hash-set? "Eliminates dupes" doesn't say.

Comment by Andy Fingerhut [ 08/Sep/12 3:39 PM ]

clj-1065-set-map-constructors-allow-duplicates-v2.txt dated Sep 8 2012 supersedes yesterday's -v1.txt patch, which I will remove.

This one updates doc strings per Rich's suggestion.

Also, his mention of metadata and "as if by assoc" led me to beef up the new test cases to check that metadata is correct, and I thus found that my new array-map constructor was not doing the right thing. This one does.

There is still no implementation of Rich's #3 yet. Just wanted to get this one up there in case someone else builds on it before I do.

Comment by Andy Fingerhut [ 09/Sep/12 3:07 AM ]

Patch clj-1065-do-map-constant-duplicate-key-checks-compile-time-only-v1.txt only makes changes that should eliminate run-time checks for duplicate map keys, if they are compile-time constants.

It is currently separate from the changes to those for the set/map constructor functions, since I am less sure about these changes. I'm not terribly familiar with the compiler internals, and I might be going about this the wrong way. Better to get separate feedback on these changes alone. I'm happy to merge them all into one patch if both parts look good.





[CLJ-1062] CLJ-940 breaks compilation of namespaces that don't have any public functions Created: 05/Sep/12  Updated: 01/Mar/13  Resolved: 21/Sep/12

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

Type: Defect Priority: Critical
Reporter: Michael Klishin Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-CLJ-1062-fix-require-1.patch    
Patch: Code
Approval: Ok

 Description   

CLJ-940 that was recently committed to master break compilation of namespaces that don't have any public functions in them
(for example, like https://github.com/michaelklishin/monger/blob/master/src/clojure/monger/json.clj that only extends
a protocol).

This affects several of clojurewerkz.org projects that no longer can compile with 1.5.0-master-SNAPSHOT.



 Comments   
Comment by Michael Klishin [ 05/Sep/12 8:39 PM ]

To be more correct: it breaks compilation of namespaces that require/load such ns without any public functions.

Comment by Michael Klishin [ 05/Sep/12 8:46 PM ]

An example project that reproduces the issue (see in the README):
https://github.com/michaelklishin/clj1062

Comment by Stuart Sierra [ 21/Sep/12 8:28 AM ]

Patch applied.





[CLJ-1061] when-first double evaluation Created: 04/Sep/12  Updated: 20/Oct/12  Resolved: 20/Oct/12

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

Type: Defect Priority: Minor
Reporter: Steve Miner Assignee: Stuart Sierra
Resolution: Completed Votes: 1
Labels: patch
Environment:

Mac OS X 10.8.1, Java 1.7.0_06


Attachments: Text File 0001-avoid-double-evaluation-in-when-first.patch    
Patch: Code
Approval: Ok
Waiting On: Rich Hickey

 Description   

The when-first macro will evaluate the xs expression twice. Admittedly, it does exactly what the doc string says, but that seems undesirable to me. Even without side effects, there's a potential performance issue if xs is some expensive operation.

Patch attached. The main diff is:

- `(when (seq ~xs)
- (let [~x (first ~xs)]
- ~@body))))
+ `(when-let xs# (seq ~xs)
+ (let ~x (first xs#)
+ ~@body))))



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

Screened.





[CLJ-1052] assoc should throw exception if missing val for last key Created: 29/Aug/12  Updated: 20/Oct/12  Resolved: 20/Oct/12

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

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Completed Votes: 8
Labels: None

Attachments: Text File clj-1052-assoc-should-throw-exc-if-missing-val-patch3.txt    
Patch: Code and Test
Approval: Ok

 Description   

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

Suggested by Ambrose Bonnaire-Sergeant:

I think assoc should throw an error when applied with uneven arguments.

Currently, the "missing" value is just replaced with nil.

(assoc {} :a 1 :b)
;=> {:a 1, :b nil}



 Comments   
Comment by Andy Fingerhut [ 29/Aug/12 4:42 PM ]

Patch clj-1052-assoc-should-throw-exc-if-missing-val-patch1.txt dated Aug 29 2012 makes assoc throw an IllegalArgumentException if more than one key is given, but the last key has no value. It includes a few simple test cases with correct and incorrect arguments to assoc.

Comment by Ambrose Bonnaire-Sergeant [ 29/Aug/12 5:14 PM ]

IMO if the error message included something like "assoc expects even number of arguments after target, found odd number", or some mention of the number of arguments the error would be clearer to me.

Comment by Andy Fingerhut [ 29/Aug/12 6:06 PM ]

clj-1052-assoc-should-throw-exc-if-missing-val-patch2.txt is identical to the now-removed -patch1.txt, except for the text of the exception thrown, updated as per Ambrose's suggestion.

Comment by Stuart Halloway [ 18/Sep/12 6:51 AM ]

The patch appears correct. It does introduce a single extra (next) per iteration into the success path, although that seems unlikely to dominate the work. Wouldn't hurt to add as assessment showing this is no slower for correct programs.

Comment by Andy Fingerhut [ 19/Sep/12 2:03 AM ]

Test platform: Mac OS X 10.6.8 + Oracle/Apple Java 1.6.0_35 Java HotSpot(TM) 64-Bit Server VM

With latest Clojure master as of Sep 19 2012:

Clojure 1.5.0-master-SNAPSHOT
user=> (set! unchecked-math true)
true
(defn count-maps [n]
(let [base {:a 1}]
(loop [i n
sum 0]
(if (zero? i)
sum
(let [m1 (assoc base :b 2 :c 3 :d 4 :e 5 :f 6 :g 7 :h 8 :i 9 :j 10)]
(recur (dec i) (+ sum (count m1))))))))
#'user/count-maps
user=> (time (count-maps 10000000))
"Elapsed time: 48784.077 msecs"
100000000
user=> (time (count-maps 10000000))
"Elapsed time: 49028.52 msecs"
100000000
user=> (time (count-maps 10000000))
"Elapsed time: 50314.729 msecs"
100000000

Same Clojure version, plus the patch that was screened:

user=> (time (count-maps 10000000))
"Elapsed time: 49576.191 msecs"
100000000
user=> (time (count-maps 10000000))
"Elapsed time: 49957.866 msecs"
100000000
user=> (time (count-maps 10000000))
"Elapsed time: 52149.998 msecs"
100000000

(average of 3 times after patch) / (average of 3 times before patch) = 1.0240

So 2.4% slowdown on average for that test case. I should add that I'm not a statistician, but note that this 2.4% difference is less than the variation in run time from one run to the next of the same experiment. Likely any real statistician would recommend collecting a lot more data before asserting there is a change in performance.

Comment by Andy Fingerhut [ 05/Oct/12 7:38 AM ]

clj-1052-assoc-should-throw-exc-if-missing-val-patch3.txt dated Oct 5 2012 is the same as the previous patch clj-1052-assoc-should-throw-exc-if-missing-val-patch2.txt dated Aug 29 2012 except some context lines have been updated so that it applies cleanly using git. The older version will be removed in a minute.





[CLJ-1048] add test.generative to Clojure's tests Created: 21/Aug/12  Updated: 01/Mar/13  Resolved: 07/Sep/12

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

Type: Enhancement Priority: Major
Reporter: Stuart Halloway Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File add-test-generative.patch    
Patch: Code and Test
Approval: Ok

 Description   

The 0.1.5 release of test generative includes data generators and specs based on data generation, plus a runner that can run both c.t and c.t.g. tests under the same umbrella.



 Comments   
Comment by Stuart Halloway [ 21/Aug/12 10:26 PM ]

Note that this patch also removes the cyclic load tests. Those tests relied on leaving broken code on the test classpath, which is very hostile to tooling (in this case the test runner's ability to walk the code in the test directory.)

I believe such tests are an antipattern, and should be implemented differently or e.g. placed in an ancillary test suite.

Comment by Fogus [ 22/Aug/12 10:32 AM ]

I agree with you position on the cyclic load tests. The patch looks sane to me and ran without hitch. A couple points of note:

  • This requires one to re-run antsetup – not a problem, but it might trip a few people
  • The final output of the test run is different than before. In the case of a successful run we now see only the generative success stats. Is the regular clojure.test stats included? It's not clear to me, but I don't think they are. In the fail case we see something like the following. I actually like the map output better, but one advantage to the stack trace output was that it sometimes helped identify code bugs. In any case, it would be nice to see an aggregate score at the end. This is a test.generative feature request I know.
     [java] {:clojure.test/vars (test-equality),
     [java]  :thread/name "main",
     [java]  :pid 8682,
     [java]  :thread 1,
     [java]  :type :assert/fail,
     [java]  :level :warn,
     [java]  :test/actual (not (not true)),
     [java]  :test/expected (not (= nil nil)),
     [java]  :line 28,
     [java]  :tstamp 1345649256308,
     [java]  :file "data_structures.clj"}

In all, I like this! Now we need to start thinking up more specs.

Comment by Stuart Halloway [ 22/Aug/12 11:51 AM ]

The stats reports are aggregate, i.e. they rollup both the generative and clojure.test stats. It would be straightforward to separate these in the report. I will look at doing that in the next drop of c.t.g.

The output already includes stack traces for errors, but not for test failures. I thought this was consistent with clojure.test, but maybe I missed something.

Comment by Fogus [ 22/Aug/12 12:59 PM ]

Thanks for the clarification Stu. You're right that the exception behavior is consistent, the confusion was my own.

Comment by Stuart Sierra [ 07/Sep/12 11:34 AM ]

Patches have been applied as of September 1, 2012.





[CLJ-1041] reduce-kv on sorted maps should stop on seeing a Reduced value Created: 10/Aug/12  Updated: 18/Aug/12  Resolved: 18/Aug/12

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

Type: Defect Priority: Major
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File sorted-map-kvreduce.patch    
Patch: Code and Test
Approval: Ok

 Description   

The current implementation is dereferencing Reduced and returning them up the chain; but that means that parent nodes don't know the reduction has completed, so the reduction might continue along other paths down the tree.

This patch touches the same areas of code as CLJ-1040 so will have conflicts if both patches are merged separately. However, as noted in my comment on CLJ-1040, I don't think the patch there is correct yet, so I've also included a commit (7beb14256) to fix the issue in CLJ-1040. Thus, this patch can be merged independently of 1040's patch, solving both issues without a merge conflict.

This patch also includes tests for both issues, which fail before my commits are applied, and pass afterwards.



 Comments   
Comment by Aaron Bedra [ 14/Aug/12 7:46 PM ]

Patch applies cleanly against 4004d267e124f12b65b0d7fb6522f32a75e3c4fb. Submitter confirmed on the CA list.





[CLJ-1034] "Conflicting data-reader mapping" triggered when the same data_readers.clj is on the classpath twice Created: 26/Jul/12  Updated: 01/Sep/12  Resolved: 01/Sep/12

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

Type: Defect Priority: Minor
Reporter: Phil Hagelberg Assignee: Unassigned
Resolution: Completed Votes: 10
Labels: None

Attachments: File clj_1034_data_readers_fix.diff    
Approval: Ok

 Description   

If you have two data_readers.clj files on the classpath that agree with one another about the mapping of a given literal, Clojure still claims there is a conflict.



 Comments   
Comment by Justin Kramer [ 13/Aug/12 9:59 AM ]

This comes up when using checkout dependencies in Leiningen. If the checked-out project contains data_readers.clj, Clojure will throw an exception.

To reproduce:

user=> (spit "/tmp/data_readers.clj" "{foo/bar foo.core/bar}")
nil
user=> (with-redefs [clojure.core/data-reader-urls (constantly [(java.net.URL. "file:/tmp/data_readers.clj")])] (#'clojure.core/load-data-readers))
{foo/bar #'foo.core/bar}
user=> (with-redefs [clojure.core/data-reader-urls (constantly [(java.net.URL. "file:/tmp/data_readers.clj")])] (#'clojure.core/load-data-readers))
ExceptionInfo Conflicting data-reader mapping clojure.core/ex-info (core.clj:4227)

Comment by Justin Kramer [ 13/Aug/12 10:21 AM ]

Attached clj_1034_data_readers_fix.diff with simple fix: checks that new mappings actually point to different vars before claiming a conflict.





[CLJ-1032] seque leaks threads from the send-off pool Created: 25/Jul/12  Updated: 01/Mar/13  Resolved: 19/Aug/12

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

Type: Defect Priority: Major
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Completed Votes: 5
Labels: None

Attachments: Text File seque.patch    
Patch: Code and Test
Approval: Ok

 Description   

Because a new (fill) action is created every time an item is (drain)ed, the LBQ gets loaded up with piles of EOS markers. If the output seq is consumed faster than the input sequence can produce items, then the original agent action does all of the filling in a single action, while the agent's queue continues to back up with (fill) actions. Eventually the entire sequence is consumed, and each queued action does a blocking .put of an EOS marker; once the queue has filled up, one of these actions blocks forever, since nobody will ever .take from the queue again.

The attached patch does two things:

  • Make sure to put exactly one EOS marker in the stream, by setting the agent's state to nil if and only if a .offer of EOS has been accepted.
  • Replace all occurrences of .put with .offer (and appropriate checking of the return value). This is necessary because if .put ever blocks, it's possible for the system to remain in that state forever (say, because the client stops consuming the queue), thus leading to the same leaked-thread scenario.

The diff is a little bit inconvenient to read because of indentation changes; https://gist.github.com/b7ecd4395a0d3d473de6 is an ignore-whitespace view of the patch for convenience.



 Comments   
Comment by Andy Fingerhut [ 26/Jul/12 3:50 AM ]

Sorry for me just triggering on the function "seque" without a deep understanding, but is this by any chance the same issue as described in CLJ-823? If so, since this one has a patch and that one doesn't, might be nice to mark CLJ-823 as a duplicate of this one.

Comment by Alan Malloy [ 26/Jul/12 4:48 AM ]

No, these are separate issues. CLJ-823 is just a special case of the general problem that seque is not usable from within an agent action. I have an implementation of seque using futures instead of agents so that this isn't a problem, but that has other problems of its own, specifically if you don't fully consume the seque you wind up leaking a future object.

Comment by Alan Malloy [ 19/Aug/12 8:11 PM ]

Marking as resolved since the patch has been applied in master.





[CLJ-1024] Varargs protococol impls can be defined but not called Created: 10/Jul/12  Updated: 14/Feb/13  Resolved: 13/Feb/13

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

Type: Enhancement Priority: Minor
Reporter: Víctor M. Valenzuela Assignee: Stuart Halloway
Resolution: Declined Votes: 1
Labels: patch

Attachments: Text File 0001-CLJ-1024-Check-for-invalid-varags-destrucuring-uses.patch    
Patch: Code

 Description   

The compiler accepts this:

(deftype foo []
clojure.lang.IFn
(invoke [this & xs]))

However calling ((foo.) :bar) will throw an AbstractMethodError. Wouldn't some checking be desirable?



 Comments   
Comment by Tassilo Horn [ 11/Jul/12 1:20 AM ]

First of all, clojure.lang.IFn is no protocol but an interface. And it does not declare a

  public Object invoke(Object... obs)

method. It has an `invoke` method with 20 Object parameters followed by an Object... parameter, but to give an implementation for that, you have to specify every parameter separately, and the last Object... arg is just a normal argument that must be an Object[]. That's because Java-varags Type... parameters are just Java syntax sugar, but in the byte-code its simply a Type-array.

What your example does is provide an `invoke` implementation for the 2-args version, where the first parameter happens to be named `&`, which has no special meaning here. Take that example:

(deftype MyFoo []
  clojure.lang.IFn
  (invoke [this & xs]
    [& xs]))

((MyFoo.) 1 2)
=> [1 2]

But you are right in that `deftype`, `defrecord`, `defprotocol`, and `definferface` probably should error if user's seem to try to use varargs or destructuring.

Comment by Víctor M. Valenzuela [ 11/Jul/12 1:55 AM ]

Cheers for a most clarifying response.

The fact that the meaning of & gets 'subverted' makes the issue only (slightly) worse, in my opinion.

Just for the record, destructuring seems to work, at least for interface impls.

Comment by Tassilo Horn [ 11/Jul/12 2:42 AM ]

> The fact that the meaning of & gets 'subverted' makes the issue only (slightly) worse, in my opinion.

I agree. I'll attach a patch which checks for those invalid usages soon.

> Just for the record, destructuring seems to work, at least for interface impls.

Could you please provide a complete example demonstrating your statement?

I'm rather sure that varargs and destructuring don't work for any of defprotocol, definterface, deftype, defrecord, and reify. But you can use varargs and destructuring when providing dynamic implementations via `extend` (or `extend-protocol`, `extend-type`), because those impls are real functions in contrast to methods.

Comment by Tassilo Horn [ 11/Jul/12 2:43 AM ]

I attached a patch. Here's the commit message:

Check for invalid varags/destrucuring uses.

Protocol, interface method declarations and implementations don't allow for
varags and destructuring support. Currently, for example

(defprotocol FooBar
(foo [this & more]))

compiles just fine, and & is interpreted as a usual argument that happens to be
named & without special meaning. But clearly, the user wanted to specify a
varags parameter here. The same applies to definterface.

Similarly, providing method implementations via defrecord, deftype, and reify
don't allow for destructuring and varags (but dynamic extenions via extend do).

So this patch makes defprotocol, definterface, defrecord, deftype, and reify
throw an IllegalArgumentException if any argument vector contains a
destructuring form or varargs argument.

Comment by Víctor M. Valenzuela [ 12/Jul/12 3:13 AM ]

Glad you've considered my request.

As for destructuring, I was speaking after this example, which may or may not work like it looks like - I don't know.

(deftype foo []
clojure.lang.IFn
(invoke [this [a b] c] (println a b c)))

((foo.) [1 2] 3)

Comment by Tassilo Horn [ 12/Jul/12 8:22 AM ]

Indeed, descructuring seems to work for method implementations. I'll adapt my patch...

Comment by Tassilo Horn [ 12/Jul/12 8:42 AM ]

Revamped patch. Here's the commit message:

Protocol, interface method declarations don't allow for varags and
destructuring support. Currently, for example

  (defprotocol FooBar
    (foo [this & more]))

compiles just fine, and & is interpreted as a usual argument that happens to be
named & without special meaning. But clearly, the user wanted to specify a
varags parameter here. The same applies to definterface.

Similarly, providing method implementations via defrecord, deftype, and reify
don't allow for varags (but dynamic extenions via extend do).

So this patch makes defprotocol and definterface throw an
IllegalArgumentException if a user tries to use varargs and destructuring in
method signatures.

Similarly, defrecord, deftype, and reify throw an IllegalArgumentException if
any method implementation arglist contains a varargs argument.

Comment by Andy Fingerhut [ 12/Jul/12 3:43 PM ]

Tassilo, with your patch 0001-CLJ-1024-Check-for-invalid-varags-destrucuring-uses.patch dated July 12, 2012, I get the following error message while testing, apparently because some metadata is missing on the new functions your patch adds to core:

[java] Testing clojure.test-clojure.metadata
[java]
[java] FAIL in (public-vars-with-docstrings-have-added) (metadata.clj:45)
[java] expected: (= [] (remove (comp :added meta) public-vars-with-docstrin
gs-not-generated))
[java] actual: (not (= [] (#'clojure.core/throw-on-varargs-and-destr #'cl
ojure.core/throw-on-varargs)))

Comment by Tassilo Horn [ 13/Jul/12 2:10 AM ]

Hi Andy, this updated patch declares the two new checking fns private which makes the tests pass again.

Stupid mistake by me: Of course, I've tested the last version, too, but afterwards I decided it wouldn't be bad to add some docstrings, and of course, adding docstrings cannot break anything.

Comment by Aaron Bedra [ 14/Aug/12 7:58 PM ]

Patch applies cleanly against 4004d267e124f12b65b0d7fb6522f32a75e3c4fb. Submitter confirmed as a CA signer.

Comment by Aaron Bedra [ 14/Aug/12 8:02 PM ]

This looks ok to me, but it seems like a fair amount of duplication to accomplish the task. It seems like we should just be able to ask if it is ok to proceed, instead of having to pick which function needs to be called in what case.

Comment by Tassilo Horn [ 15/Aug/12 1:23 AM ]

Aaron, can you please elaborate? I don't get what you mean with duplication and asking if it is ok to proceed.

Comment by Tassilo Horn [ 31/Aug/12 1:40 AM ]

Rebased to apply cleanly on master again.

Comment by Víctor M. Valenzuela [ 31/Aug/12 7:11 AM ]

Pardon the silly contribution, but the added methods' usage of double-negations (when-not ...) seems unnecessary.

Comment by Tassilo Horn [ 31/Aug/12 8:03 AM ]

Hi Victor, this revamped patch removes the double-negation and is more concise and clearer as a result. So your comment wasn't as silly as you thought.

Comment by Tassilo Horn [ 14/Feb/13 1:36 AM ]

Hey Stu, do you mind to explain why you've declined the patch?

Comment by Marek Srank [ 14/Feb/13 8:52 AM ]

@Tassilo: https://groups.google.com/forum/#!topic/clojure-dev/qjkW-cv8nog

Comment by Tassilo Horn [ 14/Feb/13 10:03 AM ]

@Marek, Stu: Thanks, I've left a reply there: https://groups.google.com/d/msg/clojure-dev/qjkW-cv8nog/rMNFqbjNj-EJ





[CLJ-1019] ns-resolve doc has a typo Created: 30/Jun/12  Updated: 18/Aug/12  Resolved: 18/Aug/12

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

Type: Defect Priority: Trivial
Reporter: Gabriel Horner Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: documentation

Attachments: Text File clj-1019-ns-resolve-doc-string-misspelling-patch1.txt     Text File fix_ns-resolve.patch    
Patch: Code
Approval: Ok

 Description   

The doc string for ns-resolve has a typo: environement should be environment.



 Comments   
Comment by Andy Fingerhut [ 12/Jul/12 12:54 PM ]

clj-1019-ns-resolve-doc-string-misspelling-patch1.txt dated July 12, 2012 is identical to fix_ns-resolve.patch of June 30, 2012, except it is in git format. It includes Gabriel's name and email address for proper attribution.

Comment by Aaron Bedra [ 14/Aug/12 8:07 PM ]

Patch applies cleanly against 4004d267e124f12b65b0d7fb6522f32a75e3c4fb. Submitter is a confirmed CA signer.





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

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

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

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

 Description   

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

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

Please see attached code and patch.



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

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

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

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

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

Hi Mike,

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

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

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

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

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

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

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

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

+1 for not allowing 0 step

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

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

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

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

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

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

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

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

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

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

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

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

yes

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

not if they have any sense

  • Has anyone been bitten by this in the past?

not me

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

I doubt it.

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

Not that I know of.

  • Are there performance implications to my patch?

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

  • Am I addressing a symptom rather than the problem?

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

-S

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

Carrying over a message by Michal Marczyk:

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

(range 3 0 -1)

evaluates to

(3 2 1)

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

Agreed on zero step.

Cheers,
Michał

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

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

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

Marking as vetted

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

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

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

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

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

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

Thanks

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

@Tim: I've removed the other attachments.

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





[CLJ-1012] partial function should also accept 1 arg (just f) Created: 12/Jun/12  Updated: 18/Aug/12  Resolved: 18/Aug/12

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

Type: Defect Priority: Minor
Reporter: Joel Martin Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File 0001-Extend-partial-function-to-also-handle-one-argument-.patch     File CLJ-1012-partial-1-arity-test.diff    
Patch: Code and Test
Approval: Ok

 Description   

The partial function should accept just a function. This allows it to be properly used with apply.

E.g. This breaks (but shouldn't) if args are nil: (apply partial f args)



 Comments   
Comment by Michel Alexandre Salim [ 13/Jun/12 5:18 AM ]

Attached patch makes partial just returns f if called with only one argument. Not sure if this will have a performance impact or not; a Clojure/core member would probably be able to better judge if the use cases outweigh any performance hit.

Comment by Fogus [ 15/Aug/12 10:45 AM ]

Attached a patch adding a test for this ticket.

Comment by Fogus [ 15/Aug/12 10:47 AM ]

Original patch is trivial, but did not have a test. I added a test as a separate patch file.





[CLJ-1011] clojure.data/diff should cope with null and false values in maps Created: 12/Jun/12  Updated: 18/Aug/12  Resolved: 18/Aug/12

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

Type: Defect Priority: Minor
Reporter: Philip Aston Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: enhancement, patch

Attachments: Text File 0001-clojure.data-diff-cope-with-falsey-values-in-maps.patch    
Patch: Code and Test
Approval: Ok

 Description   

Current behaviour of clojure.data/diff:

=> (diff {:a false} {:a true})
(nil {:a true} nil)
=> (diff {:a false} {:a nil})
(nil nil nil)

Proposed behaviour:

=> (diff {:a false} {:a true})
({:a false} {:a true} nil)
=> (diff {:a false} {:a nil})
({:a false} {:a nil} nil)


 Comments   
Comment by Philip Aston [ 12/Jun/12 5:04 AM ]
 
From e03a8060214d122ea2ebadf9e8a368f7f593d9f4 Mon Sep 17 00:00:00 2001
From: Philip Aston <philipa@mail.com>
Date: Sun, 10 Jun 2012 13:11:36 +0100
Subject: [PATCH] clojure.data/diff: cope with falsey values in maps

---
 src/clj/clojure/data.clj           |   18 +++++++++++++++++-
 test/clojure/test_clojure/data.clj |    3 ++-
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/src/clj/clojure/data.clj b/src/clj/clojure/data.clj
index 6e8dbcf..345b234 100644
--- a/src/clj/clojure/data.clj
+++ b/src/clj/clojure/data.clj
@@ -30,6 +30,22 @@
      (vec (repeat (apply max (keys m))  nil))
      m)))
 
+(defn- diff-associative-key
+  "Diff associative things a and b, comparing only the key k."
+  [a b k]
+  (let [va (get a k)
+        vb (get b k)
+        [a* b* ab] (diff va vb)
+        in-a (contains? a k)
+        in-b (contains? b k)
+        same (and in-a in-b
+                  (or (not (nil? ab))
+                      (and (nil? va) (nil? vb))))]
+    [(when (and in-a (or (not (nil? a*)) (not same))) {k a*})
+     (when (and in-b (or (not (nil? b*)) (not same))) {k b*})
+     (when same {k ab})
+     ]))
+
 (defn- diff-associative
   "Diff associative things a and b, comparing only keys in ks."
   [a b ks]
@@ -38,7 +54,7 @@
      (doall (map merge diff1 diff2)))
    [nil nil nil]
    (map
-    (fn [k] (map #(when % {k %}) (diff (get a k) (get b k))))
+    (partial diff-associative-key a b)
     ks)))
 
 (defn- diff-sequential
diff --git a/test/clojure/test_clojure/data.clj b/test/clojure/test_clojure/data.clj
index 9bab766..5a241e0 100644
--- a/test/clojure/test_clojure/data.clj
+++ b/test/clojure/test_clojure/data.clj
@@ -27,5 +27,6 @@
        [#{1} #{3} #{2}] (HashSet. [1 2]) (HashSet. [2 3])
        [nil nil [1 2]] [1 2] (into-array [1 2])
        [nil nil [1 2]] (into-array [1 2]) [1 2]
-       [{:a {:c [1]}} {:a {:c [0]}} {:a {:c [nil 2] :b 1}}] {:a {:b 1 :c [1 2]}} {:a {:b 1 :c [0 2]}}))
+       [{:a {:c [1]}} {:a {:c [0]}} {:a {:c [nil 2] :b 1}}] {:a {:b 1 :c [1 2]}} {:a {:b 1 :c [0 2]}}
+       [{:a nil} {:a false} {:b nil :c false}] {:a nil :b nil :c false} {:a false :b nil :c false}))
 
-- 
1.7.9.5
Comment by Andy Fingerhut [ 12/Jun/12 12:43 PM ]

Philip, thanks for writing that patch. Could you please attach it as a file to this ticket, instead of copying and pasting it into a comment? Instructions are under the heading "Adding patches" on this page:

http://dev.clojure.org/display/design/JIRA+workflow

Rich Hickey's policy is only to include code in Clojure that is written by those who have signed a contributor agreement. You can find out more at http://clojure.org/contributing

Comment by Philip Aston [ 12/Jun/12 2:51 PM ]

Thanks Andy. Patch attached:

0001-clojure.data-diff-cope-with-falsey-values-in-maps.patch

I'll send in a CA.

Comment by Philip Aston [ 16/Jun/12 5:27 AM ]

CA snail-mailed yesterday, I guess it will take a week to arrive.

Comment by Philip Aston [ 23/Jun/12 5:00 AM ]

I now have a CA in place.

Comment by Stuart Halloway [ 20/Jul/12 4:51 PM ]

Yeah, this should be fixed.

Comment by Aaron Bedra [ 14/Aug/12 9:42 PM ]

Patch applies cleanly against 4004d267e124f12b65b0d7fb6522f32a75e3c4fb. Submitter is a confirmed CA signer.





[CLJ-1009] make print-table org mode compatible Created: 08/Jun/12  Updated: 18/Aug/12  Resolved: 18/Aug/12

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

Type: Enhancement Priority: Minor
Reporter: Stuart Halloway Assignee: Stuart Halloway
Resolution: Completed Votes: 2
Labels: None

Attachments: Text File clj-1009-print-table-org-mode-patch3.txt     Text File print-table-org-mode.txt    
Patch: Code
Approval: Ok

 Comments   
Comment by Andy Fingerhut [ 08/Jun/12 10:39 PM ]

Patch clj-1009-print-table-org-mode-patch2.txt dated June 8, 2012 is the same as Stuart Halloway's print-table-org-mode.txt of the same date, except a couple of tests have been added. His code changes look correct to me.

Comment by Aaron Bedra [ 14/Aug/12 8:22 PM ]

This is not quite correct. the header separator in org-mode is

|--+--+--|
not
+--+--+

Comment by Andy Fingerhut [ 15/Aug/12 10:29 AM ]

clj-1009-print-table-org-mode-patch3.txt dated Aug 15, 2012 is same as the previous -patch2.txt version of Jun 8, 2012 (now deleted as obsolete), except it corrects the header separator as identified by Aaron Bedra.

Comment by Andy Fingerhut [ 15/Aug/12 10:31 AM ]

Presumptuously changing approval from Incomplete back to Vetted, as it was before Aaron Bedra described a problem with the patch, and I updated the patch to address his comment. Feel free to apply the smackdown if necessary.

Comment by Aaron Bedra [ 15/Aug/12 10:44 AM ]

Looks ok now. Three cheers for org-mode awesomeness!!!





[CLJ-1000] Performance drop in PersistentHashMap.valAt(...) in v.1.4 -- Util.hasheq(...) ? Created: 21/May/12  Updated: 01/Mar/13  Resolved: 11/Dec/12

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

Type: Enhancement Priority: Major
Reporter: Oleksandr Shyshko Assignee: Timothy Baldridge
Resolution: Completed Votes: 1
Labels: performance
Environment:

Java(TM) SE Runtime Environment (build 1.7.0_04-b21)
Java HotSpot(TM) 64-Bit Server VM (build 23.0-b21, mixed mode)


Attachments: File caching-hasheq-v3.diff     PNG File clj_13.png     PNG File clj_14.png    
Patch: Code
Approval: Ok

 Description   

It seems there is a 30-40% performance degradation of PersistentHashMap.valAt(...) in Clojure 1.4.
Possibly due to references to new CPU-hungry implementation of Util.hasheq(...).

I have created a demo project with more details and some profiling information here:
https://github.com/oshyshko/clj-perf



 Comments   
Comment by Christophe Grand [ 27/Nov/12 8:30 AM ]

I added a patch consisting of three commits:

  • one which adds caching to seqs, sets, maps, vectors and queues
  • one that aligns the shape of Util/hasheq on the one Util/equiv (to have a consistent behavior from the JIT compiler: without that deoptimization was more penalizing for hasheq than for equiv)
  • one that fix hasheq on records (which was non consistent with its equiv impl.) – and this commit relies on a static method introduced in the "caching hasheq" commit
Comment by Timothy Baldridge [ 30/Nov/12 12:10 PM ]

In the process of screening this, I'm not seeing much of a performance difference after applying the patch.

before patch:
user=> (-main)

Version: 1.5.0-master-SNAPSHOT
"Elapsed time: 6373.752 msecs"
"Elapsed time: 6578.037 msecs"
"Elapsed time: 6476.399 msecs"

after patch:
user=> (-main)

Version: 1.5.0-master-SNAPSHOT
"Elapsed time: 6182.699 msecs"
"Elapsed time: 6548.086 msecs"
"Elapsed time: 6496.711 msecs"

clojure 1.4:
user=> (-main)

Version: 1.4.0
"Elapsed time: 6484.234 msecs"
"Elapsed time: 6243.672 msecs"
"Elapsed time: 6248.898 msecs"

clojure 1.3
user=> (-main)

Version: 1.3.0
"Elapsed time: 3584.966 msecs"
"Elapsed time: 3618.189 msecs"
"Elapsed time: 3372.979 msecs"

I blew away my local clojure repo and re-applied the patch just to make sure, but the results are the same. Does this fix not optimize the case given in the original test project?

For reference I'm running this code:

(defn -main
[& args]

(println)
(println "Version: " (clojure-version))

(def mm 10000)

(def str-keys (map str (range mm)))
(def m (zipmap str-keys (range mm)))
(time (dotimes [i mm] (doseq [k str-keys] (m k))))

(def kw-keys (map #(keyword (str %)) (range mm)))
(def m (zipmap kw-keys (range mm)))
(time (dotimes [i mm] (doseq [k kw-keys] (m k))))

(def sym-keys (map #(symbol (str %)) (range mm)))
(def m (zipmap sym-keys (range mm)))
(time (dotimes [i mm] (doseq [k sym-keys] (m k))))

(println))

Comment by Christophe Grand [ 30/Nov/12 2:10 PM ]

Sorry, I was too quick to react on the ML (someone said it was related to hasheq caching and since I had the patch almost ready: on a project I noticed too much time spent computing hasheq on vectors).
So no, my patch doesn't improve anything with kws, syms or strs as keys. However when keys are collections, it fares better.

In 1.4, for a "regular" object, it must fails two instanceof tests before calling .hashCode().
If we make keywords and symbols implement IHashEq and reverse the test order (first IHashEq, then Number) it should improve the performance of the above benchmark – except for Strings.

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

Marking as incomplete, should we also delete the patch as it seems like it should be in a different ticket?

Comment by Christophe Grand [ 03/Dec/12 10:00 AM ]

In 1.3, #'hash was going through Object.hashCode and thus was simple and fast. Plus collections hashes were cached.
In 1.4 and master, #'hash goes now through two instanceof test (Number and IHasheq in this order) before trying Object.hashCode in last resort. Plus collections hashes are not cached.
As such I'm not sure Util.hasheq inherent slowness (compared to Util.hash) and hasheq caching should be separated in two issues.

The caching-hasheq-v2.diff patchset reintroduces hashes caching for collections/hasheq and reorders the instanceof tests (to test for IHashEq before Number) and makes Keyword and Symbol implement IHashEq to branch fast in Util.hasheq.

I recommend adding a collection test to the current benchmark:

(defn -main
[& args]

(println)
(println "Version: " (clojure-version))

(def mm 10000)

(def str-keys (map str (range mm)))
(def m (zipmap str-keys (range mm)))
(time (dotimes [i mm] (doseq [k str-keys] (m k))))

(def kw-keys (map #(keyword (str %)) (range mm)))
(def m (zipmap kw-keys (range mm)))
(time (dotimes [i mm] (doseq [k kw-keys] (m k))))

(def sym-keys (map #(symbol (str %)) (range mm)))
(def m (zipmap sym-keys (range mm)))
(time (dotimes [i mm] (doseq [k sym-keys] (m k))))

(def vec-keys (map (comp (juxt keyword symbol identity) str) (range mm)))
(def m (zipmap vec-keys (range mm)))
(time (dotimes [i mm] (doseq [k vec-keys] (m k))))

(println))
Comment by Timothy Baldridge [ 03/Dec/12 10:38 AM ]

For some reason I can't get v2 to build against master. It applies cleanly, but fails to build.

Comment by Christophe Grand [ 03/Dec/12 11:30 AM ]

Timothy: I inadvertently deleted a "public" modifier before commiting... fixed in caching-hasheq-v3.diff

Comment by Timothy Baldridge [ 10/Dec/12 11:00 AM ]

I now get the following results:

Version: 1.4.0
"Elapsed time: 6281.345 msecs"
"Elapsed time: 6344.321 msecs"
"Elapsed time: 6108.55 msecs"
"Elapsed time: 36172.135 msecs"

Version: 1.5.0-master-SNAPSHOT (pre-patch)
"Elapsed time: 6126.337 msecs"
"Elapsed time: 6320.857 msecs"
"Elapsed time: 6237.251 msecs"
"Elapsed time: 18167.05 msecs"

Version: 1.5.0-master-SNAPSHOT (post-patch)
"Elapsed time: 6501.929 msecs"
"Elapsed time: 3861.987 msecs"
"Elapsed time: 3871.557 msecs"
"Elapsed time: 5049.067 msecs"

Marking as screened

Comment by Oleksandr Shyshko [ 10/Dec/12 3:53 PM ]

Please, could you add as a comment the bench result using 1.3 vs 1.5-master-post-patch?

Comment by Oleksandr Shyshko [ 11/Dec/12 2:13 PM ]

The performance with 1.5-master is now very close to 1.3 for 3/4 of the benchmark.

However, this code is still showing 43% performance drop (3411 ms VS 6030 ms – 1.3 VS 1.5-master):

(def str-keys (map str (range mm)))
(def m (zipmap str-keys (range mm)))
(time (dotimes [i mm] (doseq [k str-keys] (m k))))

Version: 1.3.0
"Elapsed time: 3411.353 msecs" <---
"Elapsed time: 3459.992 msecs"
"Elapsed time: 3365.182 msecs"
"Elapsed time: 3813.637 msecs"

Version: 1.4.0
"Elapsed time: 5710.073 msecs" <---
"Elapsed time: 5817.356 msecs"
"Elapsed time: 5774.856 msecs"
"Elapsed time: 18754.482 msecs"

Version: 1.5.0-master-SNAPSHOT
"Elapsed time: 6030.247 msecs" <---
"Elapsed time: 3372.637 msecs"
"Elapsed time: 3267.481 msecs"
"Elapsed time: 3852.927 msecs"

To reproduce:
$ git clone https://github.com/clojure/clojure.git
$ cd clojure
$ mvn install -Dmaven.test.skip=true

$ cd ..

$ git clone https://github.com/oshyshko/clj-perf.git
$ cd clj-perf
$ lein run-all





[CLJ-990] Implement clojure.core.reducers/mapcat and some initial reducers tests Created: 10/May/12  Updated: 01/Mar/13  Resolved: 10/May/12

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

Type: Enhancement Priority: Minor
Reporter: Tassilo Horn Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: patch, test

Attachments: Text File 0001-Implement-clojure.core.reducers-mapcat.patch     Text File 0002-Add-initial-reducers-tests.patch    
Patch: Code and Test

 Description   

The first patch implements a reducers equivalent of clojure.core/mapcat, and the second patch adds a new reducers.clj to the clojure tests that checks that map, mapcat, filter, and reduce are equivalent in clojure.core and clojure.core.reducers.



 Comments   
Comment by Rich Hickey [ 10/May/12 7:44 PM ]

applied - thanks!





[CLJ-988] the locking in MultiFn.java (synchronized methods) can cause lots of contention in multithreaded programs Created: 08/May/12  Updated: 21/Sep/12  Resolved: 21/Sep/12

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

Type: Enhancement Priority: Major
Reporter: Kevin Downey Assignee: Stuart Sierra
Resolution: Completed Votes: 10
Labels: bug, performance

Attachments: Text File clj-988-tests-only-patch-v1.txt     File issue988-lockless-multifn+tests-120817.diff     File issue988-lockless-multifn+tests.diff    
Patch: Code and Test
Approval: Ok

 Description   

if you call a single multimethod a lot in multithreaded code you get lots of contention for the lock on the multimethod. this contention slows things down a lot.

this is due to getMethod being synchronized. it would be great if there was some fast non-locking path through the multimethod.



 Comments   
Comment by Kevin Downey [ 08/May/12 11:30 AM ]

http://groups.google.com/group/clojure-dev/browse_thread/thread/6a8219ae3d4cd0ae?hl=en

Comment by David Santiago [ 11/May/12 6:38 AM ]

Here's a stab in the dark attempt at rewriting MultiFn to use atoms to swap between immutable copies of its otherwise mutable state.

The four pieces of the MultiFn's state that are mutable and protected by locks are now contained in the MultiFn.State class, which is immutable and contains convenience functions for creating a new one with one field changed. An instance of this class is now held in an atom in the MultiFn called state. Changes to any of these four members are now done with an atomic swap of these State objects.

The getMethod/findAndCacheBestMethod complex was rewritten to better enable the atomic logic. findAndCacheBestMethod was replaced with findBestMethod, which merely looks up the method; the caching logic was moved to getMethod so that it can be retried easily as part of the work that method does.

As it was findAndCacheBestMethod seemingly had the potential to cause a stack overflow in a perfect storm of heavy concurrent modification, since it calls itself recursively if it finds that the hierarchy has changed while it has done its work. This logic is now done in the CAS looping of getMethod, so hopefully that is not even an unlikely possibility anymore.

There is still seemingly a small race here, since the check is done of a regular variable against the value in a ref. Now as before, the ref could be updated just after you do the check, but before the MultiFn's state is updated. Of course, only the method lookup part of a MultiFn call was synchronized before; it could already change after the lookup but before the method itself executed, having a stale method finish seemingly after the method had been changed. Things are no different now in general, with the atom-based approach, so perhaps this race is not a big deal, as a stale value can't persist for long.

The patch passes the tests and Clojure and multimethods seems to work.

Comment by Kevin Downey [ 12/May/12 8:59 PM ]

this patch gets rid of the ugly lock contention issues. I have not been able to benchmark it vs. the old multimethod implementation, but looking at it, I would not be surprised if it is faster when the system is in a steady state.

Comment by Stuart Halloway [ 08/Jun/12 12:11 PM ]

This looks straightforward, except for getMethod. Can somebody with context add more discussion of how method caching works?

Also, it would be great to have tests with this one.

Comment by David Santiago [ 15/Jun/12 4:44 AM ]

Obviously I didn't write the original code, so I'm not the ideal
person to explain this stuff. But I did work with it a bit recently,
so in the hopes that I can be helpful, I'm writing down my
understanding of the code as I worked with it. Since I came to the
code and sort of reverse engineered my way to this understanding,
hopefully laying this all out will make any mistakes or
misunderstandings I may have made easier to catch and correct. To
ensure some stability, I'll talk about the original MultiFn code as it
stands at this commit:
https://github.com/clojure/clojure/blob/8fda34e4c77cac079b711da59d5fe49b74605553/src/jvm/clojure/lang/MultiFn.java

There are four pieces of state that change as the multimethod is either
populated with methods or called.

  • methodTable: A persistent map from a dispatch value (Object) to
    a function (IFn). This is the obvious thing you think it is,
    determining which dispatch values call which function.
  • preferTable: A persistent map from a dispatch value (Object) to
    another value (Object), where the key "is preferred" to the value.
  • methodCache: A persistent map from a dispatch value (Object) to
    function (IFn). By default, the methodCache is assigned the same
    map value as the methodTable. If values are calculated out of the
    hierarchy during a dispatch, the originating value and the
    ultimate method it resolves to are inserted as additional items in
    the methodCache so that subsequent calls can jump right to the
    method without recursing down the hierarchy and preference table.
  • cachedHierarchy: An Object that refers to the hierarchy that is
    reflected in the latest cached values. It is used to check if the
    hierarchy has been updated since we last updated the cache. If it
    has been updated, then the cache is flushed.

I think reset(), addMethod(), removeMethod(), preferMethod(),
prefers(), isA(), dominates(), and resetCache() are extremely
straightforward in both the original code and the patch. In the
original code, the first four of those are synchronized, and the other
four are only called from methods that are synchronized (or from
methods only called from methods that are synchronized).

Invoking a multimethod through its invoke() function will call
getFn(). getFn() will call the getMethod() function, which is
synchronized. This means any call of the multimethod will wait for and
take a lock as part of method invocation. The goal of the patch in
this issue is to remove this lock on calls into the multimethod. It in
fact removes the locks on all operations, and instead keeps its
internal mutable state by atomically swapping a private immutable
State object held in an Atom called state.

The biggest change in the patch is to the
getFn()>getMethod()>findAndCacheBestMethod() complex from the
original code. I'll describe how that code works first.

In the original code, getFn() does nothing but bounce through
getMethod(). getMethod() tries three times to find a method to call,
after checking that the cache is up to date and flushing it if it
isn't:

1. It checks if there's a method for the dispatch value in the
methodCache.

2. If not, it calls findAndCacheBestMethod() on the
dispatch value. findAndCacheBestMethod() does the following:

1. It iterates through every map entry in the method table,
keeping at each step the best entry it has found so far
(according to the hierarchy and preference tables).

2. If it did not find a suitable entry, it returns null.

3. Otherwise, it checks if the hierarchy has been changed since the cache
was last updated. If it has not changed, it inserts the method
into the cache and returns it. If it has been changed, it
resets the cache and calls itself recursively to repeat the process.

3. Failing all else, it will return the method for the default
dispatch value.

Again, remember everything in the list above happens in a call to a
synchronized function. Also note that as it is currently written,
findAndCacheBestMethod() uses recursion for iteration in a way that
grows the stack. This seems unlikely to cause a stack overflow unless
the multimethod is getting its hierarchy updated very rapidly for a
sustained period while someone else tries to call it. Nonetheless, the
hierarchy is held in an IRef that is updated independently of the
locking of the MultiFn. Finally, note that the multimethod is locked
only while the method is being found. Once it is found, the lock is
released and the method actually gets called afterwards without any
synchronization, meaning that by the time the method actually
executes, the multimethod may have already been modified in a way that
suggests a different method should have been called. Presumably this
is intended, understood, and not a big deal.

Moving on now to the patch in this issue. As mentioned, the main
change is updating this entire apparatus to work with a single atomic
swap to control concurrency. This means that all updates to the
multimethod's state have to happen at one instant in time. Where the
original code could make multiple changes to the state at different
times, knowing it was safely protected by an exclusive lock, rewriting
for atom swaps requires us to reorganize the code so that all updates
to the state happen at the same time with a single CAS.

To implement this change, I pulled the implicit looping logic from
findAndCacheBestMethod() up into getMethod() itself, and broke the
"findBestMethod" part into its own function, findBestMethod(), which
makes no update to any state while implementing the same
logic. getMethod() now has an explicit loop to avoid stack-consuming
recursion on retries. This infinite for loop covers all of the logic
in getMethod() and retries until a method is successfully found and a
CAS operation succeeds, or we determine that the method cannot be
found and we return the default dispatch value's implementation.

I'll now describe the operation of the logic in the for loop. The
first two steps in the loop correspond to things getMethod() does
"before" its looping construct in the original code, but we have to do
in the loop to get the latest values.

1. First we dereference our state, and assign this value to both
oldState and newState. We also set a variable called needWrite to
false; this is so we can avoid doing a CAS (they're not free) when
we have not actually updated the state.

2. Check if the cache is stale, and flush it if so. If the cache
gets flushed, set needWrite to true, as the state has changed.

3. Check if the methodCache has an entry for this dispatch
value. If so, we are "finished" in the sense that we found the
value we wanted. However, we may need to update the state. So,

  • If needWrite is false, we can return without a CAS, so just
    break out of the loop and return the method.
  • Otherwise, we need to update the state object with a CAS. If
    the CAS is successful, break out of the loop and return the
    target function. Otherwise, continue on the next iteration
    of the loop, skipping any other attempts to fetch the method
    later in the loop (useless work, at this point).

4. The value was not in the methodCache, so call the function
findBestMethod() to attempt to find a suitable method based on the
hierarchy and preferences. If it does find us a suitable method,
we now need to cache it ourselves. We create a new state object
with the new method cache and attempt to update the state atom
with a CAS (we definitely need a write here, so no need to check
needWrite at this point).

The one thing that is possibly questionable is the check at this
point to make sure the hierarchy has not been updated since the
beginning of this method. I inserted this here to match the
identical check at the corresponding point in
findAndCacheBestMethod() in the original code. That is also a
second check, since the cache is originally checked for freshness
at the very beginning of getMethod() in the original code. That
initial check happens at the beginning of the loop in the
patch. Given that there is no synchronization with the method
hierarchy, it is not clear to me that this second check is needed,
since we are already proceeding with a snapshot from the beginning
of the loop. Nonetheless, it can't hurt as far as I can tell, it
is how the original code worked, and I assume there was some
reason for that, so I kept the second check.

5. Finally, if findBestMethod() failed to find us a method for the
dispatch value, find the method for the default dispatch value and
return that by breaking out of the loop.

So the organization of getMethod() in the patch is complicated by two
factors: (1) making the retry looping explicit and stackless, (2)
skipping the CAS when we don't need to update state, and (3) skipping
needless work later in the retry loop if we find a value but are
unable to succeed in our attempt to CAS. Invoking a multimethod that
has a stable hierarchy and a populated cache should not even have a
CAS operation (or memory allocation) on this code path, just a cache
lookup after the dispatch value is calculated.

Comment by David Santiago [ 15/Jun/12 4:45 AM ]

I've updated this patch (removing the old version, which is entirely superseded by this one). The actual changes to MultiFn.java are identical (modulo any thing that came through in the rebase), but this newer patch has tests of basic multimethod usage, including defmulti, defmethod, remove-method, prefer-method and usage of these in a hierarchy that updates in a way interleaved with calls.

Comment by David Santiago [ 15/Jun/12 6:38 AM ]

I created a really, really simple benchmark to make sure this was an improvement. The following tests were on a quad-core hyper-threaded 2012 MBP.

With two threads contending for a simple multimethod:
The lock-based multifns run at an average of 606ms, with about 12% user, 15% system CPU at around 150%.
The lockless multifns run at an average of 159ms, with about 25% user, 3% system CPU at around 195%.

With four threads contending for a simple multimethod:
The lock-based multifns run at an average of 1.2s, with about 12% user, 15% system, CPU at around 150%.
The lockless multifns run at an average of 219ms, with about 50% user, 4% system, CPU at around 330%.

You can get the code at https://github.com/davidsantiago/multifn-perf

Comment by David Santiago [ 14/Aug/12 10:02 PM ]

It's been a couple of months, and so I just wanted to check in and see if there was anything else needed to move this along.

Also, Alan Malloy pointed out to me that my benchmarks above did not mention single-threaded performance. I actually wrote this into the tests above, but I neglected to report them at the time. Here are the results on the same machine as above (multithreaded versions are basically the same as the previous comment).

With a single thread performing the same work:
The lock-based multifns run at an average of 142ms.
The lockless multifns run at an average of 115ms.

So the lockless multimethods are still faster even in a single-threaded case, although the speedup is more modest compared to the speedups in the multithreaded cases above. This is not surprising, but it is good to know.

Comment by Stuart Sierra [ 17/Aug/12 2:58 PM ]

Screened. The approach is sound.

I can confirm similar performance measurements using David Santiago's benchmark, compared with Clojure 1.5 master as of commit f5f4faf.

Mean runtime (ms) of a multimethod when called repeatedly from N threads:

|            | N=1 | N=2 | N=4 |
|------------+-----+-----+-----|
| 1.5 master |  80 | 302 | 765 |
| lockless   |  63 |  88 | 125 |

My only concern is that the extra allocations of the State class will create more garbage, but this is probably not significant if we are already using persistent maps. It would be interesting to compare this implementation with one using thread-safe mutable data structures (e.g. ConcurrentHashMap) for the cache.

Comment by David Santiago [ 17/Aug/12 7:05 PM ]

I think your assessment that it's not significant compared to the current implementation using persistent maps is correct. Regarding the extra garbage, note that the new State is only created when the hierarchy has changed or there's a cache miss (related, obviously)... situations where you're already screwed. Then it won't have to happen again for the same method (until another change to the multimethod). So for most code, it won't happen very often.

ConcurrentHashMap might be faster, it'd be interesting to see. My instinct is to keep it as close to being "made out of Clojure" as possible. In fact, it's hard to see why this couldn't be rewritten in Clojure itself some day, as I believe Chas Emerick has suggested. Also, I would point out that two of the three maps are used from the Clojure side in core.clj. I assume they would be happier if they were persistent maps.

Funny story: I was going to point out the parts of the code that were called from the clojure side just now, and alarmingly cannot find two of the functions. I think I must have misplaced them while rewriting the state into an immutable object. Going to attach a new patch with the fix and some tests for it in a few minutes.

Comment by David Santiago [ 17/Aug/12 7:44 PM ]

Latest patch for this issue. Supersedes issue988-lockless-multifn+tests.diff as of 08/17/2012.

Comment by David Santiago [ 17/Aug/12 7:49 PM ]

As promised, I reimplemented those two functions. I also added more multimethod tests to the test suite. The new tests should definitely prevent a similar mistake. While I was at it, I went through core.clj and found all the multimethod API functions I could and ensured that there were at least some basic functionality tests for all of them. The ones I found were: defmulti, defmethod, remove-all-methods, remove-method, prefer-method, methods, get-method, prefers (Some of those already had tests from the earlier version of the patch).

Really sorry about catching this right after you vetted the patch. 12771 test assertions were apparently not affected by prefers and methods ceasing to function, but now there are 12780 to help prevent a similar error. Since you just went through it, I'm leaving the older version of the patch up so you can easily see the difference to what I've added.

Comment by Rich Hickey [ 15/Sep/12 9:05 AM ]

https://github.com/clojure/clojure/commit/83ebf814d5d6663c49c1b2d0d076b57638bff673 should fix these issues. The patch here was too much of a change to properly vet.

If you could though, I'd appreciate a patch with just the multimethod tests.

Comment by Andy Fingerhut [ 15/Sep/12 10:59 AM ]

Patch clj-988-tests-only-patch-v1.txt dated Sep 15 2012 is a subset of David Santiago's
patch issue988-lockless-multifn+tests-120817.diff dated Aug 17 2012. It includes only the tests from that patch. Applies cleanly and passes tests with latest master after Rich's read/write lock change for multimethods was committed.

Comment by Rich Hickey [ 17/Sep/12 9:20 AM ]

tests-only patch ok





[CLJ-987] pprint doesn't flush the underlying stream Created: 07/May/12  Updated: 01/Mar/13  Resolved: 14/Nov/12

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

Type: Defect Priority: Major
Reporter: David Powell Assignee: David Powell
Resolution: Completed Votes: 1
Labels: patch

Attachments: Text File 0002-pprint-now-flushes-the-underlying-stream-similarly-t.patch    
Patch: Code and Test
Approval: Ok

 Description   

Unlike prn, pprint doesn't flush the underlying stream.

pretty_writer currently overrides .flush with behaviour that pushes its own data, but does not flush the underlying stream.

pprint should behave similarly to prn with respect to flushing the underlying stream.



 Comments   
Comment by David Powell [ 07/May/12 9:43 AM ]

Test included.
Patch should ensure that flushing happens after pprint, without introducing any additional unnecessary flushes.

Comment by Stuart Halloway [ 08/Jun/12 3:11 PM ]

I am confused by the tests – they all seem to call prn, though some claim to be calling pprint.

Comment by David Powell [ 09/Jun/12 6:16 AM ]

Ah, sorry. I'd tried to remove duplicate tests before committing them, but removed the wrong ones.
Replacement patch attached:

0002-pprint-now-flushes-the-underlying-stream-similarly-t.patch

Comment by Andy Fingerhut [ 21/Jun/12 5:27 PM ]

Tempting fate once again by changing Approval from Incomplete to None after the reason it was marked as incomplete seems to have been addressed.

Comment by Stuart Sierra [ 09/Nov/12 4:07 PM ]

Screened.

Comment by Stuart Sierra [ 14/Nov/12 9:23 AM ]

Pushed in commit 1588ff3f70e864d9817bc565bd2c30b08189bc6e





[CLJ-985] make jsr166 available at build time Created: 06/May/12  Updated: 01/Mar/13  Resolved: 18/May/12

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

Type: Enhancement Priority: Major
Reporter: Stuart Halloway Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File build-with-jsr166-antsetup.patch     Text File build-with-jsr166.patch    
Patch: Code
Approval: Ok

 Description   

but no runtime requirement.



 Comments   
Comment by Kevin Downey [ 07/May/12 12:46 AM ]

antsetup.sh seems to be missing?

Comment by Stuart Halloway [ 07/May/12 8:01 AM ]

and now, a patch with antsetup included

Comment by Neale Swinnerton [ 10/May/12 2:05 PM ]

You say you don't want a runtime requirement, but presumably you'll want to, at some point, run tests as part of the build so some element of runtime is required?

Comment by Stuart Halloway [ 18/May/12 8:15 PM ]

Neale: the maven "provided" scope allows use during build without runtime requirement.





[CLJ-981] clojure.set/rename-keys deletes keys when there's a collision Created: 04/May/12  Updated: 15/Jun/12  Resolved: 15/Jun/12

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

Type: Defect Priority: Minor
Reporter: Ed Bowler Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File rename-keys-fix.patch    
Patch: Code and Test
Approval: Ok

 Description   

(set/rename-keys {:a "one" :b "two" :c "three"} {:a :b :b :a}) returns {:b "one" :c "three"}
should be {:a "two" :b "one" :c "three"}

I have created a pull request for a fix, here: https://github.com/clojure/clojure/pull/23



 Comments   
Comment by Ed Bowler [ 04/May/12 1:31 PM ]

added fix





[CLJ-977] (int \a) returns a value, (long \a) throws an exception Created: 28/Apr/12  Updated: 18/Aug/12  Resolved: 18/Aug/12

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

Type: Defect Priority: Minor
Reporter: Zach Tellman Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-CLJ-977-int-a-returns-a-value-long-a-throws-an-excep.patch    
Patch: Code
Approval: Ok

 Description   

As described in the summary, calling (long ...) on a character throws a ClassCastException. I know the semantics surrounding numbers has changed a bit, but I think it would be nice for this to be consistent.



 Comments   
Comment by Scott Lowe [ 12/May/12 10:45 PM ]

Attached a patch. 0001-CLJ-977-int-a-returns-a-value-long-a-throws-an-excep.patch 13/May/12

Comment by Aaron Bedra [ 14/Aug/12 8:11 PM ]

Patch applies cleanly against 4004d267e124f12b65b0d7fb6522f32a75e3c4fb. Submitter is a confirmed CA signer.





[CLJ-967] java.io/do-copy can still garble multibyte characters on IBM JDK 1.5 and 1.6 Created: 07/Apr/12  Updated: 01/Mar/13  Resolved: 01/Mar/13

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

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

At least Linux + IBM JDK 1.5, and Linux + IBM JDK 1.6


Attachments: Text File clj-886-improved-fix-for-ibm-jdks-patch2.txt     Text File clj-967-disable-failing-io-copy-tests-on-ibm-jdk-16-patch1.txt    
Patch: Code and Test

 Description   

Same issue as CLJ-886, but while the patch applied for that issue fixes the problem for many OS/JDK combos, there appear to be differences in how surrogate pair characters are handled in some OS/JDK combos. In particular, at least Linux + IBM JDK 1.5 and Linux + IBM JDK 1.6 still fail the tests checked in for do-copy.



 Comments   
Comment by Andy Fingerhut [ 07/Apr/12 9:32 AM ]

clj-886-improved-fix-for-ibm-jdks-patch2.txt dated Apr 7 2012 makes the tests pass with Linux + IBM JDK 1.5, as well as these other combos tested:

Linux + Oracle JDK 1.7
Linux + IBM JDK 1.5
Mac OS X 10.6.8 + Oracle/Apple JDK 1.6

There are still failing tests for Linux + IBM JDK 1.6. This patch currently skips the failing tests whenever the java.vendor is "IBM Corporation" and java.version is "1.6.0", so that "ant" succeeds. It is easy enough to modify the patch so that the failing tests are kept as a bright shining reminder. Let me know if that would be preferred – it just involves removing the function ibm-jdk16, and simplifying where it is called after replacing it with false.

Comment by Stuart Halloway [ 18/May/12 9:30 AM ]

Not sure if we should be in the business of building JDK-specific workarounds into our IO library, but marking this as screened.

Option B is to leave the code as-is and only disable the tests. Rich?

Comment by Rich Hickey [ 18/May/12 2:42 PM ]

Please disable the tests. We shouldn't be doing such workarounds.

Comment by Andy Fingerhut [ 19/May/12 2:50 AM ]

clj-967-disable-failing-io-copy-tests-on-ibm-jdk-16-patch1.txt dated May 19, 2012 disables tests of clojure.java.io/copy that fail with IBM JDK 1.6.0. It makes minor changes to the clojure.java.io code file, but these are only to eliminate uses of reflection, and to make a few of the versions of do-copy share more of their implementation code.

Tested that all results are good on Mac OS X 10.6.8 + Oracle/Apple JDK 1.6.0 and Ubuntu 11.10 + Oracle JDK 1.7.0, including that the number of tests run are identical to before this patch. Only 2 tests are disabled on IBM JDK 1.6.0, and all of the rest pass before and after these changes.

Comment by Andy Fingerhut [ 19/May/12 3:22 AM ]

Hoping that I'm not out of line changing approval from Incomplete to None after attaching a patch that should address the reason it was marked incomplete. The only other way to get a ticket out of Incomplete state is for a core team member to do it for me, which seems like busy-work.

Comment by Andy Fingerhut [ 30/Aug/12 2:13 PM ]

Any comments from Rich or other screeners on the status of this one? Just curious, since it seemed to have been screened, and then quietly tossed back into the unscreened pile. Is it considered undesirable to disable selected tests for a particular JDK as the patch clj-967-disable-failing-io-copy-tests-on-ibm-jdk-16-patch1.txt does? My motivation was to selectively disable only the tests that fail, and only on the JDK where they fail, leaving all passing tests to continue to run wherever possible.





[CLJ-966] Add support for marker protocols Created: 05/Apr/12  Updated: 18/Aug/12  Resolved: 18/Aug/12

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

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: None

Attachments: File CLJ-966-additional-marker-tests-APPLY-AFTER.diff     File marker-protocols.diff    
Patch: Code and Test
Approval: Ok

 Description   

The attached patch adds support to marker protocols, for example

(defprotocol Sequential
"marker protocol indicating a sequential type")



 Comments   
Comment by Jonas Enlund [ 07/Apr/12 1:20 PM ]

Marker protocols are supported and used in ClojureScript.

Comment by Kevin Downey [ 15/Aug/12 2:23 PM ]

what are the uses for marker protocols? I know there are marker interfaces in java, but is it a pattern we want to carry forward?

Comment by Fogus [ 15/Aug/12 2:40 PM ]

Nice and simple. Tested with:

(defprotocol P (hi [_]))
(defprotocol M)
(deftype T [a] M P (hi [_] "hi there"))
(satisfies? P (T. 1))
(satisfies? M (T. 1))
(hi (T. 1))
(defprotocol M2 "marker for 2")
(extend-type T M2)
(satisfies? M2 (T. 1))

Similar tests are included in the additional patch file as test cases. This additional patch file should be applied after the feature's main patch file.





[CLJ-964] test-clojure/rt.clj has undeclared dependency on clojure.set Created: 31/Mar/12  Updated: 18/Aug/12  Resolved: 18/Aug/12

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

Type: Defect Priority: Trivial
Reporter: David Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: test

Attachments: Text File clj-964-add-require-of-clojure-set-patch1.txt    
Patch: Code
Approval: Ok

 Description   

In test-clojure/rt.clj, the test last-var-wins-for-core evaluates #'clojure.set/subset?. This fails unless clojure.set has been loaded. In the normal run of the test suite, this dependency is satisfied by test-clojure/core-set being loaded first.



 Comments   
Comment by Andy Fingerhut [ 31/Mar/12 8:47 PM ]

clj-964-add-require-of-clojure-set-patch1.txt dated March 31, 2012 applies cleanly as of latest master. It simply adds a require of clojure.set to test_clojure/rt.clj. I verified that if that test was the only one, it does fail without the require, and passes with it. I also verified that every other test file succeeds on its own without any further changes.

Comment by Aaron Bedra [ 14/Aug/12 8:30 PM ]

Patch applies cleanly against 4004d267e124f12b65b0d7fb6522f32a75e3c4fb. Submitter is a confirmed CA signer.





[CLJ-963] Support pretty printing namespace declarations under code-dispatch Created: 29/Mar/12  Updated: 01/Sep/12  Resolved: 01/Sep/12

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

Type: Defect Priority: Minor
Reporter: Tom Faulhaber Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: patch,
Environment:

all


Attachments: File pprint-ns-patch.diff    
Patch: Code and Test
Approval: Ok
Waiting On: Rich Hickey

 Description   

Currently when you print code with an (ns ) declaration in it, the ns declaration comes out kind of messy. This patch makes that beautiful.



 Comments   
Comment by Stuart Sierra [ 24/Aug/12 8:30 AM ]

Screened.

It's not perfect, but an improvement.

Before the patch:

user=> (with-pprint-dispatch code-dispatch (pprint '(ns com.some-example.my-app (:require clojure.string [clojure.set :as set] [clojure.java.io :refer (file reader)]))))
(ns
  com.some-example.my-app
  (:require
    clojure.string
    [clojure.set :as set]
    [clojure.java.io :refer (file reader)]))
nil

After the patch:

user=> (with-pprint-dispatch code-dispatch (pprint '(ns com.some-example.my-app (:require clojure.string [clojure.set :as set] [clojure.java.io :refer (file reader)]))))
(ns com.some-example.my-app
  (:require clojure.string [clojure.set :as set]
            [clojure.java.io :refer (file reader)]))
nil




[CLJ-962] Vectors returned by subvec allow access at negative indices Created: 29/Mar/12  Updated: 18/May/12  Resolved: 18/May/12

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

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

Attachments: Text File clj-962-subvec-nth-throws-on-negative-index-patch2.txt    
Patch: Code and Test
Approval: Ok

 Description   

Google group thread: https://mail.google.com/mail/?shva=1#label/clojure/1365e058eaf0d5fa

Vectors returned by subvec correctly disallow access to elements after their end, but not before their beginning.

Clojure 1.3.0
user=> (def v1 (vec (range 100)))
#'user/v1
user=> (def v2 (subvec v1 50 52))
#'user/v2
user=> (v2 3)
IndexOutOfBoundsException clojure.lang.APersistentVector$SubVector.nth (APersistentVector.java:526)
user=> (v2 -48)
2



 Comments   
Comment by Andy Fingerhut [ 29/Mar/12 10:12 AM ]

One-line simple fix. clj-962-subvec-nth-throws-on-negative-index-patch1.txt dated March 29, 2012 applies, builds, and tests cleanly on latest master. Includes a few new tests that exhibit the problem. One author has signed CA.

Comment by Alan Dipert [ 20/Apr/12 1:52 PM ]

I checked this out and it looks good to me, thank you.

Comment by Andy Fingerhut [ 10/May/12 6:29 PM ]

clj-962-subvec-nth-throws-on-negative-index-patch2.txt dated May 10, 2012 is identical to previous patch clj-962-subvec-nth-throws-on-negative-index-patch1.txt dated Mar 29, 2012, except previous one failed to apply cleanly to latest master because of some lines of context changing in the source code.





[CLJ-960] Capture :column metadata (needed for ClojureScript source maps) Created: 27/Mar/12  Updated: 20/Oct/12  Resolved: 20/Oct/12

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

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: patch

Attachments: File CLJ-960-tests.diff     Text File columns-1.patch     Text File columns-1-v2.patch    
Patch: Code and Test
Approval: Ok

 Description   

I've begun working on implementing SourceMaps for ClojureScript. For an overview of SourceMaps, see: http://www.html5rocks.com/en/tutorials/developertools/sourcemaps/ For discussion of the feature in ClojureScript, see: https://groups.google.com/d/topic/clojure-dev/zgmXO2iM1JQ/discussion

In order to produce accurate source maps, I need column information in addition to line information from the Clojure reader.

I've made the necessary enhancement to LispReader, etc. but have some cleanup and testing left to do. I'd also like a sanity check from the core team before attaching a formal patch. You can find my work in progress here: https://github.com/brandonbloom/clojure/compare/columns



 Comments   
Comment by David Nolen [ 13/Apr/12 8:29 AM ]

You need to attach a patch so this can be assessed. Thanks!

Comment by Brandon Bloom [ 31/May/12 6:09 PM ]

Sorry David! I added a patch a while ago but forgot to add the patch label(s?) as well as leave a comment.

The patch may be out of date now, but I haven't checked. Hopefully the prescreening process will pick it up automatically run the tests.

Comment by Andy Fingerhut [ 31/May/12 6:32 PM ]

Yes, Brandon, your patch has been on the list of prescreened patches since April 15. It has always applied and built cleanly during that time. That patch label is nice to have for certain JIRA report filters, but isn't necessary for the prescreening process to pick it up.

Comment by John Szakmeister [ 27/Jul/12 5:32 AM ]

v2 now applies against the current master (191b05f1). The original patch seemed to be broken from a whitespace perspective, which was making it difficult to apply--even in a 3-way merge. The only real conflict was in Compiler.java where a "final int line" was added to CompilerException. All the tests passed.

Comment by Brandon Bloom [ 27/Jul/12 7:33 PM ]

It looks like the line field was added to CompilerException in commit 89245c68, but that commit doesn't use it for anything. Maybe a later commit uses it? Also, if we want to keep that field, can we also add a column field for patch v3?

Comment by David Nolen [ 27/Jul/12 7:36 PM ]

This patch is an enhancement. In order for this one to make any movement I believe it will need a design page outlining the problem, what this solves / alternatives.

Comment by Brandon Bloom [ 25/Aug/12 9:38 PM ]

http://dev.clojure.org/display/design/Compiler+tracking+of+columns

I added a design page, but it seems like overkill. This is a straightforward enhancement...

Comment by Rich Hickey [ 04/Oct/12 8:51 AM ]

I've applied the v2 patch (thanks!), but before we close the ticket can we please get a patch comprising some tests?

Comment by Chas Emerick [ 19/Oct/12 11:45 AM ]

Attached CLJ-960-tests.diff to verify :line and :column metadata as now provided by LineNumberingPushbackReader.

Comment by Stuart Halloway [ 19/Oct/12 3:02 PM ]

The tests are a little fragile (exact map comparison) and so will break if the reader ever does more things. In library and app projects I write tests like this using core.match, but that isn't available as a build dependency here.

Marking screened anyway.





[CLJ-952] bigdec does not properly convert a clojure.lang.BigInt Created: 12/Mar/12  Updated: 18/May/12  Resolved: 18/May/12

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

Type: Defect Priority: Major
Reporter: Paul Stadig Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: None

Attachments: Text File clj-952-make-bigdec-work-on-bigints-patch1.txt    
Patch: Code and Test
Approval: Ok

 Description   

bigdec handles java.math.BigInteger when converting to java.math.BigDecimal, but it does not handle clojure.lang.BigInt. Instead it treats a clojure.lang.BigInt as a Number, by casting it to long. This causes the following error:

Clojure 1.4.0-beta3
user=> (bigdec (inc (bigint Long/MAX_VALUE)))
IllegalArgumentException Value out of range for long: 9223372036854775808 clojure.lang.RT.longCast (RT.java:1123)



 Comments   
Comment by Andy Fingerhut [ 21/Mar/12 10:55 AM ]

Add a case to bigdec to handle BigInts. Also eliminate a reflection warning in the ratio case while we are in there. Paul's failing case has been added to tests, fails before the fix, and passes after. Attempted to make it as run-time efficient as possible by creating a new BigInt/toBigDecimal, patterned after the existing BigInt/toBigInteger.

Comment by Paul Stadig [ 21/Mar/12 11:51 AM ]

I was originally thinking of something like (BigDecimal. (.toBigInteger ^clojure.lang.BigInt x)). Adding a toBigDecimal method to clojure.lang.BigInt saves some object allocations and such. Probably more of a micro optimization, but it works.

Clojure 1.4.0-master-SNAPSHOT
user=> (bigdec (inc (bigint Long/MAX_VALUE)))
9223372036854775808M

Thanks, Andy!

+1

Comment by Alan Dipert [ 20/Apr/12 1:35 PM ]

Looks good to me, thanks.





[CLJ-948] It would be very useful to be able to annotate the constructors of classes created with gen-class Created: 06/Mar/12  Updated: 20/Oct/12  Resolved: 20/Oct/12

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

Type: Enhancement Priority: Major
Reporter: Kevin Downey Assignee: Chouser
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File clj-948-annotate-gen-class-constructors-patch3.txt     Text File CLJ-948.patch    
Patch: Code and Test
Approval: Ok

 Description   

gen-class currently provides a way to annotate methods, but not constructors.

when interoperating with java code that uses google juice heavily the ability to annotate constructors is required.



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

clj-948-annotate-gen-class-constructors-patch2.txt same as Kevin's CLJ-948.patch, but with slight update so it applies cleanly to latest master as of March 9, 2012. ant builds and tests with no errors or warnings. One author Kevin Downey has signed CA.

Comment by Kevin Downey [ 09/Mar/12 9:27 AM ]

http://www.s2ki.com/s2000/uploads/gallery/1288955707/gallery_99130_32179_14062581294cd627112ab1f.gif

Comment by Andy Fingerhut [ 26/Mar/12 5:52 PM ]

clj-948-annotate-gen-class-constructors-patch3.txt on Mar 26, 2012 is no different from previous patches, except that it applies cleanly to latest master as of that date. One author Kevin Downey has signed a CA. Kevin, you don't need to thank me again. The last time gave me eye damage (only joking).

Comment by Chouser [ 18/Sep/12 12:40 AM ]

Patch is small and is a nice improvement. Tests look good and pass.





[CLJ-945] clojure.string/capitalize can give wrong result if first char is supplementary Created: 05/Mar/12  Updated: 01/Mar/13  Resolved: 01/Mar/13

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

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

all


Attachments: Text File capitalize-for-supplementary-chars-patch.txt    
Patch: Code and Test

 Description   

When the first unicode code point of a string is supplementary (i.e. requires two 16-bit Java chars to represent in UTF-16), and that first code point is changed by converting it to upper case, clojure.string/capitalize gives the wrong answer.



 Comments   
Comment by Rich Hickey [ 20/Jul/12 7:43 AM ]

Isn't this a Java bug?

Comment by Andy Fingerhut [ 20/Jul/12 12:36 PM ]

If using UTF-16 to encode Unicode strings, and making every UTF-16 code unit (i.e. Java char) individually indexable as a separate entity in strings, is such a bad design choice that you consider it a bug, then yes, this is a Java bug (and a bug in all the other systems that use UTF-16 in this way).

clojure.string/capitalize isn't using some Java capitalization method that has a bug, though. By calling (.toUpperCase (subs s 0 1)) it is not giving enough information to .toUpperCase for any implementation, Java or otherwise, to do the job correctly. It is analogous to calling toupper on the least significant 4 bits of the ASCII encoding of a letter and expecting it to return the correct answer.





[CLJ-943] When load-lib fails, a namespace is still created Created: 01/Mar/12  Updated: 15/Jun/12  Resolved: 15/Jun/12

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

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

Attachments: Text File 0001-Remove-namespace-if-load-lib-fails-and-namespace-did.patch    
Patch: Code and Test
Approval: Ok

 Description   

When requiring a namespace that doesn't compile, a namespace is still created. The attached patch removes the namespace on failure if the namespace wasn't already present on entry to load-lib. See the test case in the patch for repro instructions.

This is obviously a subset of having atomic loads. Would a step further in this direction, e.g. using a swapable state object within clojure.lang.Namespace be of interest?



 Comments   
Comment by Stuart Halloway [ 08/Jun/12 3:17 PM ]

Why catch Exception, not Throwable?

Comment by Stuart Halloway [ 08/Jun/12 3:23 PM ]

This patch accomplishes its purpose, although (in the absence of fully atomic load) I would imagine it creates a race condition if one thread tried to load a broken namespace while another thread tried simply to create a namespace.

Marked screened anyway...





[CLJ-940] Passing a non-sequence to refer :only results in uninformative exception Created: 24/Feb/12  Updated: 01/Sep/12  Resolved: 01/Sep/12

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

Type: Defect Priority: Major
Reporter: Hugo Duncan Assignee: Stuart Sierra
Resolution: Completed Votes: 0
Labels: None

Attachments: File 0001-add-exception-for-non-sequence-in-refer-only.diff     Text File 0003-CLJ-940-check-for-sequential.patch     Text File clj-940-add-exception-for-non-sequence-in-refer-only-patch.txt    
Patch: Code
Approval: Ok
Waiting On: Rich Hickey

 Description   

Compiling the following code results in a Don't know how to create ISeq from: clojure.lang.Symbol exception

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



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

Hugo, your patch doesn't apply cleanly to latest master due to some changed lines of context around it that are from before Nov 2011, which confuses me given that your patch was created recently. I could mechanically update it, but if you could take a look and create an updated patch yourself it would be safer.

Comment by Andy Fingerhut [ 26/Apr/12 7:36 PM ]

Patch clj-940-add-exception-for-non-sequence-in-refer-only-patch.txt dated Apr 26 2012 is same as Hugo Duncan's, except it applies cleanly to latest master as of today.

Comment by Stuart Sierra [ 17/Aug/12 9:14 AM ]

Previous patch does not accurately reflect new :refer syntax in ns:

(:require [... :refer ...])

I offer a new patch, 0003-CLJ-940-check-for-sequential.patch, as an alternative.

This patch also checks for the more specific clojure.lang.Sequential instead of IPersistentCollection (which includes sets and maps).

If I had my druthers, I'd check for IPersistentList, but I can't face the screaming that would result.

Neither patch provides file/line information in the error, but there isn't much affordance for that in core.clj right now.

Comment by Aaron Bedra [ 21/Aug/12 10:57 AM ]

Applies cleanly against d4170e65d001c8c2976f1bd7159484056b9a9d6d. This looks good to me. We should at some point talk more about the implications of checking IPersistenList, but I think there is enough value here to push it forward.





[CLJ-936] Improve docs about argument destructuring at clojure.org Created: 21/Feb/12  Updated: 01/Mar/13  Resolved: 27/Nov/12

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

Type: Enhancement Priority: Minor
Reporter: Jakub Holy Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: documentation, web

Approval: Accepted

 Description   

What:
Make the description of argument destructuring more visible at http://clojure.org/special_forms, namely:

  • add a heading such as "<h4>Binding Forms (Argument Destructuring)</h4>" to the section describing it
  • make all occurences of "binding-form" a link to that heading, especially under the let and fn headings
  • for (fn ..) add a new second paragraph like "Regarding binding forms, also known as argument destructuring, <a href=#thea-new-heading>read more in the binding forms section</a> under the let special form."

Why:
It was always surprisingly difficult for me to google out the explanation of destructuring in Clojure and only today have I discovered that it is described pretty well under the let special form. Thus it should be made more visible to the readers (and preferably also search engines). (Even though Google has returned the page as one of the first matches, I had problems seeing it there - partly due to usually mistyping destructuring e.g. as deconstruction).

Thanks a lot!

PS: I haven't found a better way to report this, such as a commenting capability on the page itself.



 Comments   
Comment by Andy Fingerhut [ 12/Mar/12 6:03 PM ]

Jakub, I don't think I have authorization to edit those web pages in the way you suggest. I did want to comment that the most recent version of the Clojure cheatsheet at http://clojure.org/cheatsheet now has a link called "examples" in the "Special Forms" section that links to the 'let' section of the special forms documentation page.





[CLJ-934] disj! throws exception when attempting to remove multiple items in one call Created: 21/Feb/12  Updated: 15/Jun/12  Resolved: 15/Jun/12

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

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

Attachments: Text File clj-934-transient-disj-patch-3.txt    
Patch: Code and Test
Approval: Ok

 Description   

disj! fails whenever called with multiple items to remove:

user=> (-> #{5 10 15 20} transient (disj! 10 15) persistent!)
ClassCastException clojure.lang.PersistentHashSet$TransientHashSet cannot be cast to clojure.lang.IPersistentSet clojure.core/disj (core.clj:1419)

It is a simple one line fix to disj! Clojure source code to correct this.



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

clj-934-transient-disj-patch2.txt fixes a problem with my previous one where it had a warning during testing because of a poorly named test that conflicted with a name in clojure.core. This one is clean.

Comment by Stuart Halloway [ 08/Jun/12 10:37 AM ]

Patch 3 is same as patch 2, but nonreflective invocation.

Comment by Andy Fingerhut [ 08/Jun/12 10:42 AM ]

Removed older patch in favor of Stuart's improved one clj-934-transient-disj-patch-3.txt dated June 8, 2012.





[CLJ-932] contains? should throw exception on non-keyed collections Created: 17/Feb/12  Updated: 18/Aug/12  Resolved: 18/Aug/12

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

Type: Enhancement Priority: Minor
Reporter: Stuart Sierra Assignee: Stuart Sierra
Resolution: Completed Votes: 1
Labels: None

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

 Description   

The contains? function, given a collection which is not an associative (Map, Set, String, array), returns false instead of throwing an exception.

This is a subject of confusion when people call contains? on sequential collections like lists, and on associative collections which do not implement the Associative interface.

Other predicates, such as even?, throw an exception when passed arguments of an invalid type.



 Comments   
Comment by Stuart Sierra [ 17/Feb/12 3:30 PM ]

Patch adds fix, adds some tests, and removes tests reflecting the old behavior.

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

This seems to be working properly, but what about vectors?

(contains? [1 2 3] 3)
;-> false
Comment by Andy Fingerhut [ 15/Aug/12 12:40 PM ]

The doc string for contains? covers the vector and Java array case explicitly. I'm not saying that this behavior shouldn't change, but at least it is well documented what it currently does in these cases.

Comment by Aaron Bedra [ 15/Aug/12 2:03 PM ]

Agreed. I just want to make sure that we are still ok with this functionality given that things are changing. Are there others (Stuart) that want to chime in here and make the intentions clear? If this is good then I would consider this screened and ready.

Comment by Stuart Sierra [ 15/Aug/12 2:40 PM ]

Vector is Associative, so supporting contains? is valid even if it does not do what people might expect:

user=> (contains? [:a :b :c] 2)
true
user=> (contains? [:a :b :c] 7)
false

All I'm trying to change here is have contains? throw an exception if the argument is not Associative. The current behavior (returning false) was hiding a bug in my code.

I do not consider this a breaking change. I believe the docstring of contains? leaves room for this interpretation, but Rich will have the final say.

Comment by Aaron Bedra [ 15/Aug/12 2:42 PM ]

Perfect. I just wanted to make sure that this was intended.





[CLJ-927] default tagged literal reader Created: 06/Feb/12  Updated: 01/Mar/13  Resolved: 09/Nov/12

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

Type: Enhancement Priority: Major
Reporter: Fogus Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: clojure, reader

Attachments: Text File CLJ-927-default-data-reader-fn-for-unknown-tags.patch    
Patch: Code and Test
Approval: Ok

 Description   

With data reader support, it's impossible to write a program to read an arbitrary stream of Clojure forms. For example, the following code will fail with the current 1.4.0-beta1 tagged literal support:

#point [0 2]

It might be enough to require that the read side define a reader for point, but what if we do not wish to require that both sides of the conversation know about the #point tag beforehand? Using the identity function as a default allows the reader to handle the literal form as is even when it doesn't recognize a tag (or even care to translate it).

The change from the current behavior is that missing tagged literal parsers are no longer error conditions.



 Comments   
Comment by Steve Miner [ 12/Feb/12 10:30 AM ]

I'd like to preserve the unknown literal tag as well as the value. That would allow a program to recreate the printed representation. A map would work as the value. You'd get something like: {:unknown-literal point :value [0 2]}. If you needed to pass that on to some other process, you could easily write it in the original literal form. Perhaps the key for literal tag should be namespace qualified to avoid possible confusion with user data. Another benefit of returning a map for an unknown literal tag is that equality tests still seem reasonable: (not= #foo "abc" #bar "abc").

Comment by Steve Miner [ 18/Sep/12 11:01 AM ]

It would be convenient if I could handle unknown tags using some sort of catch-all key in *data-readers* (say 'default). The associated function should take two arguments: the tag and the literal value. If there is no 'default key in *data-readers*, then an error would be thrown (same as Clojure 1.4).

I think it's a simple way to allow the programmer to take control without having to add new API or data types. It's just one distinguished key ('default, :default something like that) and one additional line of doc.

I can provide a patch.

Comment by Rich Hickey [ 21/Sep/12 9:17 AM ]

This needs to be addressed. We should follow the advice given in the edn docs:

If a reader encounters a tag for which no handler is registered, the implementation can either report an error, call a designated 'unknown element' handler, or create a well-known generic representation that contains both the tag and the tagged element, as it sees fit. Note that the non-error strategies allow for readers which are capable of reading any and all edn, in spite of being unaware of the details of any extensions present.

We can get both of the latter by having a preinstalled default unknown element handler that returns the generic representation. "identity" is out since it loses the tag. Using a map with namespaced keys is somewhat subtle. An TaggedElement record type is also possible.

Issues are - what happens when more than one library tries to supply a default? If the system supplies one, perhaps it's best to only allow dynamic rebinding and not static installation. Or error on conflicting default handlers, or first/last one wins (but first/'last' might be difficult to predict).

Comment by Steve Miner [ 17/Oct/12 12:49 PM ]

Everyone agrees that identity is not an appropriate default so I changed the Summary field.

Comment by Steve Miner [ 18/Oct/12 8:54 PM ]

Minimal patch that adds support for a default data reader in *data-readers*. If an unknown tag is read, we look up the 'default reader in *data-readers and call it with two arguments: the tag and the value. By default, there is no default reader and you get an exception as in Clojure 1.4.

Comment by Steve Miner [ 18/Oct/12 8:57 PM ]

An alternative patch that establishes a default data reader to handle the case of an unknown tag. The default reader returns a map with metadata to define the :unknown-tag. This preserves the information for the unknown data literal, but keeps a simple and convenient format.

Comment by Stuart Halloway [ 19/Oct/12 5:27 AM ]

Steve,

I am guessing that you consider these two patches alternatives, not cumulative. I am marking as screened the "default-in-data-readers" patch, which allows users to specify a 'default handler.

The other patch "unknown-data-reader", which specifies a built-in Clojure handler for unknown tags, is not screened. It specifies a default handler that returns a metadata-annotated map. If there is to be a default handler, I think it would need to return something disjoint from data, e.g. a tagged interface of some kind (or at least a namespaced name in the map.)

It would be a great help to have a little discussion along with the patches.

Comment by Steve Miner [ 19/Oct/12 8:01 AM ]

Yes, the two patches are separate alternatives. The "default-in-data-readers" patch just adds the special treatment for the 'default key in *data-readers* without providing a system default. This allows the application programmer to provide a catch-all handler for unknown data literal tags. Libraries should be discouraged from setting a 'default handler. Conflicts with the 'default key in data_readers.clj are handled just like other keys so it would be a bad thing for multiple libraries to try to take over the 'default data reader. Libraries can instead provide implementations and let the application programmer do the actual setting of the 'default key.

The second patch ("unknown-data-reader") implements 'default similarly, but also provides for a 'default reader in default-data-readers. My unknown-data-reader returns a map. I found it safer and simpler to deal with keywords instead of symbols for unknown tag – Clojure doesn't like symbols for unknown packages and you never know what you might get in data literal tags. After experimenting with with my original idea of having a map with two keys (essentially :tag and :value), I decided that I preferred the single entry map with the key derived from the tag and the value preserved as read. Adding the metadata to define the :unknown-tag provides enough information for the application programmer to deal with the maps unambiguously. I think the single entry maps are easier to read.

The alternative that I did not pursue would be to use a new record type as the default data type. My second patch could be used as a basis for that approach. We just need to replace my unknown-data-reader function with one that creates a record (TaggedElement).

Comment by Rich Hickey [ 19/Oct/12 5:43 PM ]

I don't like the 'default entry, especially if it is usually wrong for a library to set it.

Having no bindable var default makes editing data_readers.clj an outstanding chore for everyone.

It is unlikely a single special override in data_readers.clj is suitable for every reading situation, even in the same app.

If there is a known TaggedElement type, then people need only be able to opt out of that.
So if there were a default-tagged-reader function of (tag read-element) that built a TaggedElement (or, alternatively, threw, if people prefer), people could just rebind that in a particular reading context.

There's no perfect default, but in most situations getting unknown data is an error. I personally would default to that, and provide a read-tagged-element fn people could bind in when trying to implement read-anything.

Comment by Steve Miner [ 20/Oct/12 10:03 AM ]

old patches deleted. This revised patch introduces a var *default-data-reader-fn* which can be used to handle unknown tags. By default it is nil and an exception is thrown for the unknown tag as in Clojure 1.4. If it's non-nil, the function is called with the tag and value. I chose the name so that it contained 'data-reader', which makes it search friendly. I wanted to commit this separately from any attempt to provide a built-in catch-all reader and new record type as that might be more contentious.

Comment by Steve Miner [ 02/Nov/12 9:42 AM ]

The patch for *default-data-reader-fn* was committed for Clojure 1.5 beta1. The programmer can now specify a catch-all reader, which solves the main issue. However, there is no built-in default reader so unknown tags still cause exceptions to be thrown as in Clojure 1.4.

I think this bug can be closed. Default reader implementations could be provided by a contrib library. Or someone can open a new bug if you want the language to provide a built-in default reader.

Comment by Nicola Mometto [ 02/Nov/12 1:24 PM ]

what about adding default-tagged-reader-fn to clojure.main/with-bindings to make it set!able?

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

Good point about making it set!-able. I filed that issue as a separate bug: CLJ-1101.

Comment by Stuart Sierra [ 09/Nov/12 8:26 AM ]

Resolved.





[CLJ-923] Reading ratios prefixed by + is not working Created: 03/Feb/12  Updated: 18/Aug/12  Resolved: 18/Aug/12

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

Type: Defect Priority: Major
Reporter: Cosmin Stejerean Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-923-reading-ratios-prefixed-by-plus-patch2.txt    
Patch: Code and Test
Approval: Ok

 Description   

In general Clojure's number types can be read prefixed with either a +
or - and this seems to work correctly for reading integers and floats.
In the case of ratios however things break down when ratios are
prefixed with a +.

The ratio pattern in LispReader.java does match on ratios starting
with both + and - but matchNumber fails on ratios prefixed with +
because it ends up calling "new BigInteger(m.group(1))" and it turns
out the constructor for BigInteger has no problems with negative
numbers but it doesn't like numbers prefixed by a +.



 Comments   
Comment by Cosmin Stejerean [ 03/Feb/12 6:26 PM ]

added patch with fix and tests

Comment by Kevin Downey [ 24/Feb/12 4:02 PM ]

changes to the reader tests on master cause 0001-added-tests-for-reading-ratios-and-fixed-reading-of-.patch to no longer apply cleanly

Comment by Andy Fingerhut [ 24/Feb/12 4:21 PM ]

clj-923-reading-ratios-prefixed-by-plus-patch.txt applies cleanly to latest as of Feb 24, 2012 (2:20 PM PST

Comment by Andy Fingerhut [ 23/Mar/12 7:55 PM ]

clj-923-reading-ratios-prefixed-by-plus-patch2.txt still semantically same as Cosmin's original patch, except it applies, builds, and tests cleanly on latest master as of Mar 23, 2012. Context lines around patch must have changed recently.

Comment by Cosmin Stejerean [ 23/Mar/12 8:07 PM ]

Thanks for updating the patch. I've removed the original to make it clear which one we need.

Comment by Aaron Bedra [ 14/Aug/12 9:33 PM ]

Patch applies cleanly against 4004d267e124f12b65b0d7fb6522f32a75e3c4fb. Submitter is a confirmed CA signer.





[CLJ-917] clojure.core/definterface is not included in the API docs Created: 23/Jan/12  Updated: 20/Oct/12  Resolved: 20/Oct/12

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

Type: Defect Priority: Minor
Reporter: Tassilo Horn Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: documentation

Attachments: Text File 0001-Add-docstring-and-added-metadata-to-definterface.patch     Text File CLJ-917-definterface.patch    
Patch: Code
Approval: Ok

 Description   

Is definterface meant to be a public API? If yes, then it needs a docstring.



 Comments   
Comment by Tassilo Horn [ 25/Jan/12 2:28 PM ]

This patch obsoletes the previous one. The only addition is the insertion of :added metadata which is needed to make the tests pass.

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

Updated patch.

Comment by Stuart Sierra [ 23/Mar/12 9:04 AM ]

Screened, pending question for Rich: "Is definterface meant to be a public API?"

Comment by Tassilo Horn [ 31/May/12 1:45 AM ]

Rebased patch on current master.

Comment by Stuart Sierra [ 24/Aug/12 1:12 PM ]

Screened again. Still applies as of commit 1c8eb16a14ce5daefef1df68d2f6b1f143003140

Comment by Stuart Halloway [ 19/Oct/12 10:32 AM ]

The CLJ-971 patch I just added is the same as the original with grammar corrections.

Comment by Tassilo Horn [ 20/Oct/12 5:48 AM ]

Patch including Stu's grammar/typo fixes.

Sorry, contributors like to show up in the commit history, so I couldn't resist stealing my patch back from you.





[CLJ-916] into loses metadata Created: 21/Jan/12  Updated: 18/Aug/12  Resolved: 18/Aug/12

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

Type: Defect Priority: Major
Reporter: Mark Swanson Assignee: Unassigned
Resolution: Completed Votes: 4
Labels: into, metadata, walk

Attachments: Text File clj-916-make-into-and-others-preserve-metadata-patch2.txt     Text File clj-916-make-into-preserve-metadata-patch1.txt    
Patch: Code and Test
Approval: Ok

 Description   

The into fn loses metadata.

I'm seeing some team members (myself included) rely on metadata. Metadata has been incredibly useful in some cases, however the silent destruction of metadata by core clojure fns (into, walk, etc) have been a source of increasing complexity and confusion.

A team member could start relying on a 3rd party library that used 'into'. Actually, the into fn could essentially be added to the code base at any time in many ways. Wrt metadata the potential for incidental complexity increases exponentially as more developers and libraries enter the mix.

One of the reasons Clojure has worked for us so well is because it has greatly reduced incidental complexity.
IMHO the 'into metadata bug' seriously limits the usefulness of metadata and would love to see the into fn fixed in 1.4.



 Comments   
Comment by Andy Fingerhut [ 01/Mar/12 4:42 AM ]

Someone please correct this if it is wrong, but it seems that into loses metadata because it is implemented using transients, and transients lose metadata. If true, then one way to fix this ticket is to implement metadata for transients.

Are there any fundamental reasons why it would be difficult to add metadata to transients, as long as the metadata itself was immutable?

Comment by Andy Fingerhut [ 16/Mar/12 4:57 AM ]

First rough cut of a patch that makes not only into, but also select-keys, clojure.set/project, and clojure.set/rename preserve metadata. Added tests for these and many other functions. Still some open questions about other functions not tested in comments. This patch does not attempt to make transients preserve metadata.

Comment by Andy Fingerhut [ 23/Mar/12 8:03 PM ]

clj-916-make-into-and-others-preserve-metadata-patch2.txt is semantically same patch and comments as March 16, 2012. Merely touched up to apply cleanly to latest master as of Mar 23, 2012.

Comment by Aaron Brooks [ 26/May/12 10:58 PM ]

I just ran into this issue myself. Is there anything that I could do to help get this fixed? Test writing? Patch checks? I'm happy to help in any way I can.

Comment by Andy Fingerhut [ 27/May/12 1:11 AM ]

You could examine the existing patch, including its tests, and see if it would have done what you were hoping it would do. Add a comment here regarding whether the changes look good to you. The attached patch is already on my weekly list of prescreened patches, but it is only one among many.

Comment by Fogus [ 15/Aug/12 1:14 PM ]

The code is clean and tests test what they should test. Regarding the questions in the test file:

  • I recommend (royal) we create another ticket for the remaining clojure.set functions and discuss them there. Once resolved the questions in the metadata.clj test file should be removed.
  • remove returns a LazySeq, but the intent of the question is clear. I vote that remove preserve metadata as well, but that should be the topic of yet another ticket.

One final point. I'd prefer that tickets be addressed in isolation and not contain extra fixes. This is a personal preference, but one that I'd like to take up on the clojure-dev list.

Comment by Andy Fingerhut [ 16/Aug/12 12:38 AM ]

clj-916-make-into-preserve-metadata-patch1.txt dated Aug 15 2012 only changes the behavior of into so that it preserves metadata. One or more other tickets will be created for some other functions that currently do not preserve metadata, but perhaps should.





[CLJ-910] [Patch] Allow for type-hinting the method receiver in memfn Created: 13/Jan/12  Updated: 01/Sep/12  Resolved: 01/Sep/12

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

Type: Enhancement Priority: Minor
Reporter: Tassilo Horn Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: performance, reflection

Attachments: Text File 0001-Make-memfn-allow-for-type-hinting-the-method-receive.patch    
Patch: Code
Approval: Ok

 Description   

The attached patch copies metadata given to the method name symbol of memfn to the method receiver in the expansion. That way, memfn is able to be used even for type-hinted calls resulting in a big performance win.

user> (time (dotimes [i 1000000] ((memfn intValue) 1)))
Reflection warning, NO_SOURCE_FILE:1 - call to intValue can't be resolved.
"Elapsed time: 2579.229115 msecs"
nil
user> (time (dotimes [i 1000000] ((memfn ^Number intValue) 1)))
"Elapsed time: 12.015235 msecs"
nil


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

Updated patch.

Comment by Aaron Bedra [ 21/Aug/12 6:06 PM ]

Applies properly against d4170e65d001c8c2976f1bd7159484056b9a9d6d. Things look good to me.





[CLJ-909] Make LineNumberingPushbackReader's buffer size configurable Created: 11/Jan/12  Updated: 18/Aug/12  Resolved: 18/Aug/12

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

Type: Enhancement Priority: Minor
Reporter: Colin Jones Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-Allow-custom-buffer-size-in-LineNumberingPushbackRea.patch    
Patch: Code
Approval: Ok

 Description   

In some situations, it's necessary to configure the buffer size of LineNumberingPushbackReader's wrapped java.io.LineNumberReader, that gets created in the constructor. A concrete problem case is where you want to avoid doing reads from the underlying Reader whenever possible, so using a buffer size of 1 makes it a bit lazier. I can also imagine cases where you'd want to increase the buffer from java.io.BufferedReader's 8192-char default, but I haven't dealt with that one directly.

There's no good way to do this by subclassing LineNumberingPushbackReader, since all the action happens in the constructors: those of java.io.LineNumberReader and the superclass of LineNumberingPushbackReader, which is java.io.PushbackReader. So my current workaround is to copy the entirety of LineNumberingPushbackReader, change the name, and add a constructor. Having LineNumberingPushbackReader support this directly would be great.

Both java.io.LineNumberReader and java.io.PushbackReader have constructors that accept the buffer size as the second argument, so it seems very reasonable to me to add a similar constructor for LineNumberingPushbackReader.



 Comments   
Comment by Colin Jones [ 22/Jan/12 6:56 PM ]

This patch still works great.

Turns out my current workaround to get around the lack of this ability has a problem: LispReader depends on the concrete LineNumberingPushbackReader class to be able to call .getLineNumber (via instanceof / casting). So the similar (read: nearly-copied) class I'm trying to use can't store line numbers, which makes the stack trace less nice (fwiw, this is in REPL-y: https://github.com/trptcolin/reply).

It would be nice to have an interface (ILineNumberingPushbackReader?) that declares getLineNumber, readLine, and atLineStart, and have things check instanceof on that instead of the concrete class. That way, anyone else adhering to the LineNumberingPushbackReader contract (in order to bind that to *in* as the docstring for `clojure.main/repl` prescribes) can do it and have line numbering play nicely with the Clojure reader.

If that sounds desirable, I can replace this patch. The existing patch will also work fine if we want to keep things concrete, or if that feels enough like solving a different problem to require another ticket.

Comment by Fogus [ 15/Aug/12 1:49 PM ]

The patch looks fine.

Regarding the larger question of an interface I'd say another ticket is in order. The existence of LNPBR is an implementation detail, but I too have found it useful. Maybe there is a wider discussion on the usefulness of LNPBR and a sane way to expose it for tool, compiler, etc. devs?





[CLJ-902] doc macro broken for namespaces Created: 28/Dec/11  Updated: 01/Mar/13  Resolved: 12/Aug/12

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

Type: Defect Priority: Minor
Reporter: Greg Chapman Assignee: Stuart Sierra
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File clj-902-doc-on-namespaces-patch.txt     File repl.diff    
Patch: Code and Test
Approval: Ok

 Description   

Example:

Clojure 1.3.0
user=> (doc clojure.pprint)
ClassNotFoundException clojure.pprint java.net.URLClassLoader$1.run (URLClassLoader.java:366)

It appears it is not safe to call resolve on a symbol representing a namespace; you get the error above. FWIW, I seem to have resolved the problem (see attached diff) by moving the find-ns clause above the resolve clause (in the cond); also the reference to namespace-doc needs to be var-quoted since namespace-doc is private.



 Comments   
Comment by Andy Fingerhut [ 17/Feb/12 12:43 AM ]

Verified there is a bug, and that this change fixes it. New patch is in proper format, and includes a new unit test that would have caught the bug.

Comment by Stuart Sierra [ 08/Aug/12 9:52 AM ]

Screened. Ready for approval.

Comment by Stuart Sierra [ 12/Aug/12 3:48 PM ]

Patch applied.





[CLJ-897] deftype error message is misleading not useful Created: 14/Dec/11  Updated: 15/Jun/12  Resolved: 15/Jun/12

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

Type: Enhancement Priority: Minor
Reporter: Myles Megyesi Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

OS X


Attachments: Text File 0001-Fix-CLJ-897-Don-t-use-descructuring-in-defrecord-def.patch     Text File clj-897-deftype-error-message-is-misleading-patch.txt    
Patch: Code
Approval: Ok

 Description   

If you forget to include the argument vector for deftype, i.e.

(deftype second-request-handler
HttpServer.RequestHandler
(canRespond [this request] true)
(getResponse [this request] nil))

which should be...

(deftype second-request-handler []
HttpServer.RequestHandler
(canRespond [this request] true)
(getResponse [this request] nil))

I get the error message..

Don't know how to create ISeq from: clojure.lang.Symbol (main.clj:1)

When in fact the error is coming from (second-request-handler.clj:3). Can this error message be improved and give the actually location of the error?



 Comments   
Comment by Tassilo Horn [ 23/Dec/11 4:28 AM ]

The problem is that deftype and defrecord use destructuring in the argument vector, so the error pops up before the macro is actually running.

The attached patch removes the destructuring form (but keeps the nice syntax in :arglists) and adds a vector? check to validate-fields.

All tests pass, and the example above now errors with "AssertionError No fields vector given."

Comment by Andy Fingerhut [ 23/Mar/12 2:40 AM ]

Changing Patch attribute to "Code".

Comment by Andy Fingerhut [ 26/Apr/12 7:29 PM ]

clj-897-deftype-error-message-is-misleading-patch.txt dated Apr 26 2012 is the same as Tassilo Horn's patch, except it is updated to apply cleanly to latest master as of today.





[CLJ-893] crafted `vec' call allows created vector to be mutated Created: 08/Dec/11  Updated: 18/May/12  Resolved: 18/May/12

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

Type: Defect Priority: Major
Reporter: Stephen Compall Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None
Environment:

Found in 1.3.0; tested in 1.4.0-alpha2 as well.


Attachments: Text File clj-893-doc-vec-aliases-java-array-patch3.txt    
Patch: Code
Approval: Ok

 Description   
user> (let [a (to-array (repeat 31 0)), v (vec a)]
        [(v 2) (do (aset a 2 'no) (v 2))])
[0 no]
user> (let [a (to-array (repeat 33 0)), v (vec a)]
        [(v 2) (do (aset a 2 'no) (v 2))])
[0 0]

While a relaxation of when different changes to the environment are made may impact the resulting value of the vector, as it is with lazy seqs, it oughtn't be possible to get two different results for the same access, just as with lazy seqs.



 Comments   
Comment by Stuart Halloway [ 09/Dec/11 9:34 AM ]

The code path followed here lets vec assume (without copying) a passed-in array. to-array just returns the array, and createOwning doesn't copy either.

Seems like we need a defensive copy somewhere

Comment by Rich Hickey [ 22/Jan/12 12:26 PM ]

Just document that, when passed an array, vec aliases it, and it should not be changed. Then only people that can't ensure that must copy it first.

Comment by Andy Fingerhut [ 24/Feb/12 8:33 PM ]

Proposed patch to document the existing behavior, and briefly suggest safe ways to use vec with mutable collections.

Comment by Stuart Sierra [ 23/Mar/12 9:00 AM ]

I think the docstring in the 24/Feb/12 patch is too long. Also not entirely correct: calling vec on a mutable java.util.List, for example, will create an immutable vector from the contents of the List.

Instead just say that Java arrays may be aliased by vec and should not be modified.

Comment by Andy Fingerhut [ 26/Mar/12 5:16 PM ]

clj-893-doc-vec-aliases-java-array-patch2.txt of Mar 26, 2012 is shorter than the previous attempt, and avoids the error Stuart Sierra found.

Comment by Rich Hickey [ 13/Apr/12 8:09 AM ]

I'd prefer it said will alias and "should not be modified" vs "should be copied".

Comment by Andy Fingerhut [ 14/Apr/12 4:22 PM ]

Would someone better at writing Clojure style doc strings take a crack at this? I'm too verbose. I have added a (verbose) note about this behavior on clojuredocs.org.

Comment by Brenton Ashworth [ 21/Apr/12 4:12 PM ]

Added patch

clj-893-doc-vec-aliases-java-array-patch3.txt

With this patch the doc string would be:

Creates a new vector containing the contents of coll. Java arrays
will be aliased and should not be modified.
Comment by Andy Fingerhut [ 24/Apr/12 7:42 PM ]

Removed non-preferred patch clj-893-doc-vec-aliases-java-array-patch2.txt of Mar 26, 2012.





[CLJ-892] sort changes its argument, if a Java array Created: 08/Dec/11  Updated: 26/Jul/13  Resolved: 18/May/12

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

Type: Enhancement Priority: Minor
Reporter: Stephen Compall Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

java version "1.6.0_22"
OpenJDK Runtime Environment (IcedTea6 1.10.4) (6b22-1.10.4-0ubuntu1~11.04.1)
(clojure-version)
"1.4.0-alpha2"


Attachments: Text File clj-892-clarify-sort-sort-by-doc-strings-patch1.txt    
Patch: Code
Approval: Ok

 Description   

user> (let [a (to-array [32 11])]
(prn (seq a))
(sort a)
(prn (seq a)))
(32 11)
(11 32)
nil

Where the second line printed ought to be the same as the first.



 Comments   
Comment by Stuart Halloway [ 09/Dec/11 9:37 AM ]

This is an enhancement request, since the docs for sort make no promise one way or the other.

For performance, I prefer the current behavior, so another possibility is a clarifying doc string.

Comment by Andy Fingerhut [ 26/Mar/12 5:32 PM ]

clj-892-clarify-sort-sort-by-doc-strings-patch1.txt of Mar 26, 2012 applies cleanly to latest master on that date. Only doc string changes, to make it clear that by sort and sort-by will modify Java arrays given as arguments.





[CLJ-881] Problem with the "cl-format" function from the clojure.pprint Created: 20/Nov/11  Updated: 18/May/12  Resolved: 18/May/12

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

Type: Defect Priority: Trivial
Reporter: Vyacheslav Dimitrov Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None
Environment:

Linux 2.6.31-22-generic #61-Ubuntu SMP Wed Jul 28 01:57:06 UTC 2010 i686 GNU/Linux


Attachments: Text File clj-881-cl-format-exception-patch2.txt     File patchfile    
Patch: Code and Test
Approval: Ok

 Description   

Let's see the following scenario:

vdim@home:~/clojure$ git log -1
commit ba930d95fc3a4a78c5bd6756ea483c9dac681618
Author: Rich Hickey <richhickey@gmail.com>
Date: Sun Oct 30 10:44:55 2011 -0400

inline equiv in variadic =
vdim@home:~/clojure$ rlwrap java -cp clojure-1.4.0-master-SNAPSHOT.jar clojure.main
Clojure 1.4.0-master-SNAPSHOT
user=> (use 'clojure.pprint)
nil
user=> (cl-format nil "~12,10F" 1.00000000074)
"1.0000000007"
user=> (cl-format nil "~12,10F" 1.00000000076)
NumberFormatException For input string: "10000000007" java.lang.NumberFormatException.forInputString (NumberFormatException.java:65)
user=>

The exception is caused from round-str function (cl-format.clj) where
my number (100000000076) is coerced to an Integer (see line with Integer/valueOf code
into this function).

Is this normal behaviour?

See patch with tests and my suggestion for solving this problem.



 Comments   
Comment by Vyacheslav Dimitrov [ 20/Nov/11 2:49 AM ]

Also I can create pull request if any.

Comment by Andy Fingerhut [ 12/Feb/12 2:29 AM ]

I've modified Vyacheslav's patch so that it is in the proper format. I also changed his implementation of function inc-s so that it should allocate a bit less memory, and removed an addition of a redundant test case that is in his patch. There is a bug he has found here, and I've verified that this patch fixes it.

Comment by Andy Fingerhut [ 26/Mar/12 5:04 PM ]

clj-881-cl-format-exception-patch2.txt Mar 26, 2012 applies cleanly to latest master, and fixes the problem in the same way as my Feb 12, 2012 patch (since deleted to avoid confusion). I am the author, and have signed a CA.

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

This patch looks good to me. I recommend we apply it.





[CLJ-870] clojure.string/replace behaves unexpectedly when \ or $ are part of the result string Created: 04/Nov/11  Updated: 18/Aug/12  Resolved: 18/Aug/12

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

Type: Defect Priority: Minor
Reporter: Heinz N. Gies Assignee: Stuart Sierra
Resolution: Completed Votes: 2
Labels: None
Environment:

HW/OS/SW indipendant - issue is part of java interface


Attachments: Text File 0001-Add-tests-for-870.patch     Text File 0002-Fix-870-by-using-quoteReplacement.patch     Text File CLJ-753-870-905-combined-fix3.patch     Text File CLJ-753-870-905-combined-fix3.readme.txt     Text File clj-753-870-905-combined-fix4.patch    
Patch: Code and Test
Approval: Ok
Waiting On: Rich Hickey

 Description   

clojure.string/replace uses javas replace function to do it's work, the replace function has the tricky habit of 'double evaluating' the replacement string (third argument). This means that every \ in the string (so i.e. "
") is evaluated by the java code behind replace.

Since this behavior isn't documented it can lead to confusing errors, for example (made up semi real world example to explain the issue):

(clojure.string/replace "c:/windows/" #"/" "\\")

This should replace all unix folder separators with windows separators, this crashes with a index out of bound exemption since java tries to evaluate "
" as a semi regexp.

or

(println (str "\"" (clojure.string/replace "my string with \\ and \" in it" #"[\"\\]" (fn [x] (str "\\" x))) "\""))

The expected result would be:

"my string with \\ and \" in it"

the actual result is:

"my string with \ and " in it"

This should return an 'escaped' string, it does not since the 'double evaluation' of the return string results in \\\\ being reduced to
again.



 Comments   
Comment by Heinz N. Gies [ 04/Nov/11 5:31 AM ]

A working code for the replace example is:

(println (str "\"" (clojure.string/replace "my string with \\ and \" in it" #"[\"\\]" (fn [x] (str  "\\\\"  (if (= x "\\") "\\") x))) "\""))

which is horribly ugly since you need to hand escape the backslash or it crashes.

Comment by Stuart Sierra [ 09/Dec/11 3:31 PM ]

Vetted on 1.4.0 master. Needs patch.

Comment by Alan Malloy [ 05/Jan/12 8:30 PM ]

No hand-escaping is necessary, just use #(java.util.regex.Matcher/quoteReplacement %).

Edit: I see, we're talking about when you use a function as a replacement, not a replacement string like "
x" - the function's output should not be interpreted as regex replacement, I agree. Looks like it just needs a small patch in clojure.string/replace-by.

Comment by Alan Malloy [ 05/Jan/12 8:53 PM ]

Patch and test for this issue.

Comment by Andy Fingerhut [ 06/Jan/12 12:25 AM ]

CLJ-870-also-fix-replace-first.patch combines Alan Malloy's two patches, and adds the same change for replace-first. I like this change, too, and think replace and replace-first should be consistent with each other in this behavior.

Comment by Andy Fingerhut [ 06/Jan/12 12:32 AM ]

If there is any interest in a combined patch for this issue, CLJ-753 and CLJ-905 all in one, I'd be happy to merge them.

Comment by Andy Fingerhut [ 12/Jan/12 12:05 AM ]

Proposed combined patch for CLJ-753, CLJ-870, and CLJ-905, since they all affect the behavior of clojure.string/replace and replace-first.

Comment by Stuart Sierra [ 03/Feb/12 9:15 AM ]

The 1/12/2012 combined patch is not in correct format. Please recreate, see http://clojure.org/patches for correct format.

Comment by Andy Fingerhut [ 04/Feb/12 10:51 AM ]

CLJ-753-870-905-combined-fix2.patch should have proper patch format.

Comment by Andy Fingerhut [ 28/Feb/12 12:38 PM ]

Attaching slightly improved patch CLJ-753-870-905-combined-fix3.patch, along with a README to explain in gory detail the behavior and performance changes to clojure.string/replace and clojure.string/replace-first, in case it would help anyone review the changes. Deleting my older patches.

Comment by Aaron Bedra [ 14/Aug/12 8:44 PM ]

This is a small nit pick, but re-qr is a pretty non-descriptive name. Perhaps we could change it to re-quote-replacement?

Comment by Andy Fingerhut [ 15/Aug/12 11:19 AM ]

Patches clj-753-870-905-combined-fix4.patch dated Aug 15 2012 and the patch that Aaron reviewed, CLJ-753-870-905-combined-fix3.patch dated Feb 28 2012, are almost identical.

fix3 uses the name re-qr for a function, where fix4 uses the name re-quote-replacement. I have no strong preference for either of them.

Comment by Andy Fingerhut [ 15/Aug/12 11:23 AM ]

Added patch addressing what I think is Aaron's reason for changing state to Incomplete, so as a result I am changing state back to its former Vetted state. If there is something else a non-screener should be doing to bring Incomplete tickets to the attention of screeners, please let me know, and I will document it here: http://dev.clojure.org/display/design/JIRA+workflow

Comment by Stuart Sierra [ 15/Aug/12 2:42 PM ]

I will be looking at this.

Comment by Stuart Sierra [ 17/Aug/12 7:55 AM ]

Screened. This is an adequate fix for this particular problem, given the current API.

However, the long and complicated docstring proves that replace and replace-first should each be four separate functions:

code
replace-char [input char char]
replace-string [input string string]
replace-re [input pattern string]
replace-re-by [input pattern fn]
code

This is how these functions were originally written back in the days of clojure.contrib.str-utils. This would be a minor breaking change.





[CLJ-867] Records of different types with the same data have the same hashcodes, even though they are not considered to be equal Created: 30/Oct/11  Updated: 18/May/12  Resolved: 18/May/12

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

Type: Defect Priority: Minor
Reporter: David Powell Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-CLJ-867-Incorporate-the-record-name-into-the-hash-co.patch     Text File clj-867-incorporate-record-name-into-hash-code.txt    
Patch: Code
Approval: Ok

 Description   

Records which contain the same field values are not considered to be equal, but the record type is not currently included in the hash. This means that if an heterogenous map contains records of different types, where the fields aren't necessarily distinct, there is a high likelyhood of hash collisions.

I propose that the record type should be included in the hash code algorithm for records. There should be no expectation that unequal records of different types should have the same hash-code.

user=> (hash (->A 1))
1013911913
user=> (hash (->B 1))
1013911913
user=> (= (>A 1) (>B 1))
false

This issue also affects ClojureScript. But see CLJS-97, as ClojureScript also has an issue with record equality.



 Comments   
Comment by David Powell [ 30/Oct/11 7:27 AM ]

Potential patch to xor the hash of the record symbol name with the current hash obtained from the data

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

You can't change hashCode w/o breaking Map semantics. But you can implement IHashEq and do this in hasheq()

Comment by Brenton Ashworth [ 21/Apr/12 5:23 PM ]

Added patch

clj-867-incorporate-record-name-into-hash-code.txt

which implements IHashEq for records, incorporating the record name into the hash code in the hasheq method.

With this patch you will now see the following behavior:

user=> (defrecord A [x])
user.A
user=> (defrecord B [x])
user.B
user=> (= (->B 1) (->A 1))
false
user=> (= (hash (->B 1)) (hash (->A 1)))
false




[CLJ-844] NPE calling keyword on map from bean Created: 27/Sep/11  Updated: 15/Jun/12  Resolved: 15/Jun/12

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

Type: Defect Priority: Major
Reporter: Phil Hagelberg Assignee: Steve Miner
Resolution: Completed Votes: 2
Labels: None

Attachments: Text File 0001-CLJ-844-NPE-calling-keyword-on-map-from-bean.patch    
Patch: Code and Test
Approval: Ok

 Description   

Calling a keyword on a map returned from clojure.core/bean causes a null pointer exception if the keyword is not a key in the map:

user=> (:a (bean {}))
java.lang.NullPointerException (NO_SOURCE_FILE:0)
user=> (.printStackTrace *e)
Caused by: java.lang.NullPointerException
at clojure.core$bean$v__4765.invoke(core_proxy.clj:385)
at clojure.core$bean$fn__4786.invoke(core_proxy.clj:394)
at clojure.core.proxy$clojure.lang.APersistentMap$0.valAt(Unknown Source)
at clojure.lang.KeywordLookupSite.fault(KeywordLookupSite.java:33)
at user$eval1062.invoke(NO_SOURCE_FILE:7)
at clojure.lang.Compiler.eval(Compiler.java:5424)
... 9 more

The object returned by bean claims to be an APersistentMap FWIW:

user=> (class (bean {}))
clojure.core.proxy$clojure.lang.APersistentMap$0



 Comments   
Comment by Stuart Halloway [ 07/Oct/11 9:41 AM ]

Bean needs a full overhaul. In my ideal world it would be deprecated out of core into some other namespace.

Comment by Luc Préfontaine [ 07/Oct/11 11:59 PM ]

If you can sketch a bit what you have in mind, I might give it a try.
I have some time available and went through the pending issues, this one seems within
my reach.

Comment by Steve Miner [ 20/Apr/12 1:09 PM ]

The current work-around is to use a default arg like this:

(:a my-bean nil)

It works, but it's inconvenient and adds complexity to the handling of bean maps.

Normal maps of course already return nil as the default value for a missing key. The current bean proxy does the contains? check on the key only if there's a default value. The patch uses the same contains? check for the single-arg version of valAt.





[CLJ-828] clojure.core/bases returns a cons when passed a class and a Java array when passed an interface Created: 15/Aug/11  Updated: 18/Aug/12  Resolved: 18/Aug/12

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

Type: Enhancement Priority: Minor
Reporter: Alf Kristian Støyle Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-bases-should-return-a-seq-not-a-Java-array.patch     Text File 0001-Make-sure-the-clojure.core-bases-function-always-ret.patch    
Patch: Code and Test
Approval: Ok

 Description   

clojure.core/bases returns a clojure.lang.Cons when called with a class as parameter, but a Java array ( [Ljava.lang.Class; ) when called with an interface. Both should return values of the same type, which I guess should be a seq.

Showing the problem at the REPL:
user=> (bases java.util.List)
#<Class[] [Ljava.lang.Class;@315b0333>
user=> (bases java.util.ArrayList)
(java.util.AbstractList java.util.List java.util.RandomAccess
java.lang.Cloneable java.io.Serializable)

I have attached a patch which calls the seq function on the else part of clojure.core/bases. Also updated the clojure.test-clojure.java-interop/test-bases test. However these test do not currently seem to run as part of the maven build. I have however run the test manually and verified that it works.



 Comments   
Comment by Andy Fingerhut [ 24/Feb/12 7:29 PM ]

Behavior where bases returns Java array in some cases still exists on latest master, and this patch still applies cleanly and changes that behavior as described.

The patch writer Alf is not on the list of people who have signed a CA. If he isn't in the process of doing so, what should be done with the patch? If I described the behavior to a committer without showing them the patch, they would likely come up with a similar one-line fix.

Comment by Alf Kristian Støyle [ 25/Feb/12 1:09 AM ]

I was not aware that contributors had to sign the CA to get a patch in when I wrote this, so my apologies for adding the patch. I just wanted to make the contribution an "easy add". I guess you guys would have figured out a similar fix really easy.

I am not in the process of signing the CA. I would not have any problems signing it, but then I guess for such a small contribution it would be too much of a hassle. And besides, I don't think this small contribution makes me a worthy contributor.

I would certainly not mind you guys fixing this in anyway you can, with or without my patch.

Cheers,
Alf

Comment by Andy Fingerhut [ 25/Feb/12 11:58 AM ]

No apologies necessary for trying to improve the code, Alf. We appreciate it.

I agree it might be a bit of a hassle to sign a CA just for this fix. Legally, a Clojure contributor is one whether anyone considers them "worthy" or not.

We'll figure something out.

Comment by Steve Miner [ 18/May/12 3:38 PM ]

Independently written patch that fixes bases so that it always returns a seq. Added additional tests to verify that it returns nil (rather than an empty seq) when there are no bases to maintain compatibility with the original code.

Comment by Fogus [ 16/Aug/12 2:27 PM ]

Patch 0001-bases-should-return-a-seq-not-a-Java-array.patch from Steve Miner works as expected and is of acceptable quality.

Comment by Steve Miner [ 18/Aug/12 8:54 AM ]

I think commit 73f94cb8c7f60a131759b4488a3fefd6599d0328 took the wrong patch. Stoyle said he did not have a CA. I wrote an independent patch which Fogus vetted.

Comment by Stuart Halloway [ 18/Aug/12 9:19 AM ]

Reverted wrong patch, applied correct patch. Thanks for watching my back, Steve.





[CLJ-820] int coercion doesn't work in clojure 1.3 Created: 14/Jul/11  Updated: 20/Jul/12  Resolved: 20/Jul/12

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

Type: Defect Priority: Major
Reporter: Tassilo Horn Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

Gentoo GNU/Linux



 Description   

Using the clojure git head as of 2011-07-14 (commit f704853751d02faf72bd53be599ee0be6c1da63e), int coercion doesn't work:

user> (class (int 1))
java.lang.Long

byte, short, double, and float coercion work fine, though:

user> (class (byte 1))
java.lang.Byte
user> (class (short 1))
java.lang.Short
user> (class (double 1))
java.lang.Double
user> (class (float 1))
java.lang.Float

Also creating integers directly works fine:

user> (class (Integer. "100"))
java.lang.Integer
user> (class (Integer/valueOf 1))
java.lang.Integer
user> (class (Integer. 100))
java.lang.Integer

This is probably related to CLJ-439.



 Comments   
Comment by Sebastián Bernardo Galkin [ 15/Jul/11 2:33 PM ]

Related documentation here: http://dev.clojure.org/display/doc/Enhanced+Primitive+Support

all long-or-smaller integers are boxed as Long
You can't use primitive coercions to force specific box types
Your code was broken if you were relying on that

So, apparently (int) has the correct behavior. It's byte and short which should be also boxed into Longs

Comment by Sebastián Bernardo Galkin [ 15/Jul/11 2:34 PM ]

See also this topic in clojure-dev: https://groups.google.com/d/topic/clojure-dev/fXQAYv8s0tQ/discussion

Comment by Tassilo Horn [ 15/Jul/11 3:00 PM ]

So to call some Java method that requires an int you have to use Integer/valueOf?

Comment by Sebastián Bernardo Galkin [ 15/Jul/11 3:46 PM ]

Yes, I think so

Comment by Tassilo Horn [ 15/Jul/11 4:16 PM ]

But for what are the coercion functions now good for? I mean, http://clojure.org/java_interop#Java Interop-Coercions states:

"At times it is necessary to have a value of a particular primitive type. These coercion functions yield a value of the indicated type as long as such a coercion is possible: bigdec bigint boolean byte char double float int long num short"

That's exactly my use case which is not supported anymore.

Comment by Sebastián Bernardo Galkin [ 15/Jul/11 11:09 PM ]

(int) (char) etc. are not intended to return boxed types, they always return primitive types. If you want boxed types you can use (num). When you do (class (int 42)) you are casting the long 42 to a int and then boxing it into a Long to get its class.

For short and byte, I don't know, I think there is a bug, (class (short 42)) should be Short.

Comment by Tassilo Horn [ 16/Jul/11 7:13 AM ]

(int) (char) etc. are not intended to return boxed types, they always return primitive types.

Thank you, Sebastián, that made it clear and unveiled the problem at my side. When I did

(set-value! foo :position (int 17))

I got the exception "java.lang.Long cannot be cast to java.lang.Integer". set-value! is a protocol function extended on the java types of the java library I'm interacting with. Eventually, it'll call (doto foo (.setAttribute "position" (mutable <providedVal>)), where mutable is yet another protocol function that converts clojure collections to the libraries counterparts and does nothing for primitive types.

Now, although (int 17) returns an unboxed int, these two indirections box it into a Long again and the java setter is called with that leading to the error.

(set-value! foo :position (Integer/intValue 17))

circumvents the issue, because here you directly get an int boxed as Integer.

If you want boxed types you can use (num).

Well, I want a boxed value of a certain type, like Integer.

For short and byte, I don't know, I think there is a bug, (class (short 42)) should be Short.

Should or should not? Currently, for all primitive types <primType>, the call (class (<primType> <val>)) returns the wrapper class corresponding to <primType> with the exception of int where you get a Long instead of an Integer. That's totally surprising and caused me to file this bug report.

Comment by Michel Alexandre Salim [ 26/May/12 12:55 PM ]

This is fixed in 1.4.0 – could one of the screeners confirm this, and maybe close the issue?





[CLJ-788] Add source and line members and getters to CompilerException Created: 03/May/11  Updated: 02/Dec/12  Resolved: 20/Jul/12

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

Type: Enhancement Priority: Minor
Reporter: Hugo Duncan Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File 0001-add-line-member-and-getter-to-CompilerException.diff     Text File clj-788-add-line-member-and-getter-to-CompilerException-patch.txt    
Patch: Code
Approval: Ok

 Description   

To improve the reliability and ease of using CompilerException in tooling, add source path and line getters to CompilerException.

When handling compiler exceptions programmatically, the source path and line currently have to be parsed out of the compiler exception.



 Comments   
Comment by Hugo Duncan [ 18/May/11 5:13 PM ]

OK, I'm blind - the source is already there - the line number would be nice though.

Comment by Hugo Duncan [ 24/Feb/12 10:28 AM ]

On clojure-1.3.0

Comment by Andy Fingerhut [ 24/Feb/12 11:03 AM ]

clj-788-add-line-member-and-getter-to-CompilerException-patch.txt is identical to Hugo's, but for some reason that isn't obvious to me his did not apply cleanly to latest master when I tried it out, but this one does.

Compiles without warnings and passes all tests.

Comment by Stuart Halloway [ 08/Jun/12 3:03 PM ]

Would it make sense to have CompilerException inherit from ExceptionInfo, now that we have the latter?

Comment by Rich Hickey [ 15/Jun/12 9:32 AM ]

what patch am I supposed to be looking at?

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

They are identical except for context lines. clj-788-add-line-member-and-getter-to-CompilerException-patch.txt is the only one that applies cleanly to latest master as of June 15, 2012.

Comment by Brandon Bloom [ 01/Dec/12 7:02 PM ]

What about getColumn ? http://dev.clojure.org/jira/browse/CLJ-960

Comment by Andy Fingerhut [ 02/Dec/12 5:41 PM ]

Brandon, just a warning that while anyone who looks at this ticket will see your comment, not many people examine closed tickets looking for things to do with them. If by your comment you mean that you think some additional change should be made to Clojure that wasn't made with the patch on this ticket (which I think was committed), nor by the patch committed for CLJ-960, bringing it up in the clojure-dev group, or opening another ticket, might be more effective.





[CLJ-779] Document clojure.org differences between 1.2 and 1.3 Created: 26/Apr/11  Updated: 01/Mar/13  Resolved: 09/Nov/12

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

Type: Task Priority: Minor
Reporter: Christopher Redinger Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Approval: Vetted

 Description   

Documentation at http://clojure.org should always represent the latest release version. When we ship Release.next, we need to update the documentation there to reflect the latest changes.



 Comments   
Comment by Stuart Sierra [ 09/Nov/12 8:35 AM ]

This work has been done.





[CLJ-768] cl-format bug in ~f formatting Created: 05/Apr/11  Updated: 15/Jun/12  Resolved: 15/Jun/12

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

Type: Defect Priority: Minor
Reporter: Tom Faulhaber Assignee: Andy Fingerhut
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File clj-768-patch-for-after-clj-881-fixed-patch2.txt    
Patch: Code and Test
Approval: Ok

 Description   

Carlos Ungil reports a bug in cl-format as follows (http://groups.google.com/group/clojure/browse_thread/thread/bb235ff4464aed78#):

Hello,

I don't know if there is a better way to file bug reports (if there is one,
it's not easy to find).

(use 'clojure.pprint)

(let [x 111.11111]
(doseq [fmt ["~3f~%" "~4f~%" "~5f~%" "~6f~%"]]
(cl-format true fmt x)))

;; 111.11111
;; 111.11111
;; 111.1
;; 111.11

There is clearly a problem in the first two cases, too many digits are being
printed.

For reference, this is what common lisp does:

(let ((x 111.11111))
(loop for fmt in '("3f%" "4f%" "5f%" "6f%")
do (format t fmt x)))

;; 111.
;; 111.
;; 111.1
;; 111.11

Cheers,

Carlos



 Comments   
Comment by Andy Fingerhut [ 13/Feb/12 9:49 PM ]

Attached patch assumes that CLJ-881 patch has been applied. It might apply cleanly without CLJ-881 patch being applied first – I haven't checked. Fixes the issue originally reported, and several other problems found with cl-format's ~f directive.

Comment by Tom Faulhaber [ 17/Feb/12 9:26 AM ]

I've reviewed the patch and it looks good to me. Thanks, Andy!

To clojure/core: please go ahead and accept this patch.

Comment by Andy Fingerhut [ 17/Feb/12 1:33 PM ]

Thanks for the review, Tom. If you have the time, CLJ-881 is shorter, and fixes a different bug. I do know that if you apply the CLJ-881 recommended patch, and then this one, everything works fine. If it is helpful, I can try out this one independently, but they are both bugs in the ~f format of cl-format, the other one causing Clojure to throw an exception for some combinations of ~f directives and numerical values.

Comment by Andy Fingerhut [ 23/Feb/12 3:30 AM ]

clj-768-without-clj-881-patched-first.txt is the same as the one that Tom reviewed, except it applies cleanly to master as of Feb 23, 2012 without having to first apply the patch for CLJ-881. This bug and the patch are really independent of CLJ-881.

Comment by Andy Fingerhut [ 19/May/12 1:03 AM ]

clj-768-patch-for-after-clj-881-fixed-patch2.txt dated May 18, 2012 is the most up to date one as of the latest updates to Clojure master. Only context line changes from previous patches so that it applies cleanly, not substantive changes.





[CLJ-763] Do not use hash-map constructor in destructuring to avoid multiple key check. Created: 21/Mar/11  Updated: 18/May/12  Resolved: 18/May/12

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

Type: Enhancement Priority: Trivial
Reporter: Meikel Brandmeyer Assignee: Meikel Brandmeyer
Resolution: Completed Votes: 0
Labels: None
Environment:

Clojure versions 1.2 and above


Attachments: Text File 0001-Do-not-check-for-duplicates-in-destructuring-map-cre.patch    
Patch: Code and Test
Approval: Ok

 Description   

What I did:

(defn number-to-string
  [& {fmt :format locale :locale :or {locale (Locale/getDefault)}}]
  (fn [v]
    (String/format locale fmt (to-array [v]))))

(defn double-to-string
  [& options]
  (apply number-to-string :format "%f" options))

(def us-number (double-to-string :locale Locale/US))
(def legacy-number (double-to-string :format "% 3.2f"))

(us-number 3.14159)
(legacy-number 3.14159)

What I expected:

"3.14159"
"  3,14"

What I got:

java.lang.IllegalArgumentException: Duplicate key: :format

Using into or a combination of reduce and assoc instead of hash-map would allow this without breaking things.

If this is a desired modification, I can provide a patch. (CA is filed.)



 Comments   
Comment by Meikel Brandmeyer [ 21/Mar/11 1:43 PM ]

Build map in destructuring step by step instead of calling hash-map.

Comment by Andy Fingerhut [ 26/Mar/12 6:43 PM ]

clj-763-destructuring-allow-dup-keys-patch2.txt dated Mar 26, 2012 applies cleanly to latest master and passes all tests, including a new one added based upon Meikel's report that fails without his fix. Supersedes his earlier patch 0001-Avoid-duplicate-key-check-in-destructuring.patch of Mar 21, 2011. Meikel and I have both signed CAs.

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

I'm happy to see this improved, but it has to be fast. into + partition is not fast enough down here. Probably should instead call non-checking map ctor

Comment by Meikel Brandmeyer [ 23/Apr/12 6:08 PM ]

Map destructuring without duplicate check with test case

Comment by Meikel Brandmeyer [ 23/Apr/12 6:10 PM ]

Uploaded 0001-Do-not-check-for-duplicates-in-destructuring-map-cre.patch with a faster implementation accessing the unchecked constructor directly plus a simplified test case. Supersedes the two previous patches. (I can delete only mine, but not Andy's.)

Comment by Andy Fingerhut [ 24/Apr/12 7:39 PM ]

Removing patch clj-763-destructuring-allow-dup-keys-patch2.txt dated Mar 26 2012 in favor of Meikel's Apr 23 2012 patch, which fixes the problem in a faster way, and its test is more straightforward.

Comment by Meikel Brandmeyer [ 25/Apr/12 2:30 PM ]

Deleted obsolete patch.





[CLJ-757] Empty transient maps/sets return wrong value for .contains Created: 14/Mar/11  Updated: 18/Aug/12  Resolved: 18/Aug/12

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

Type: Defect Priority: Major
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-Fix-behavior-of-empty-transient-maps.patch     Text File clj-757-fix-behavior-of-empty-transient-maps-patch3.txt    
Patch: Code and Test
Approval: Ok

 Description   

As explained in http://groups.google.com/group/clojure/browse_thread/thread/496ffc7fb9058699/6e9d8152b6fd6365?show_docid=6e9d8152b6fd6365, (.contains (transient #{}) :some-key) returns true instead of false. (contains? #{} :some-key) works properly, but the plain .contains method does not.

The issue is in TransientHashMap.doValAt(key, notFound), and the attached patch is a simple fix with test.



 Comments   
Comment by Stuart Halloway [ 05/Apr/11 8:59 PM ]

IIRC there are broader issues around lookup from transients. Should we look at this patch alone, or step back and review?

Comment by Rich Hickey [ 29/Jul/11 7:44 AM ]

take the patch if it fixes the problem

Comment by Andy Fingerhut [ 23/Feb/12 2:55 AM ]

clj-757-fix-behavior-of-empty-transient-maps-patch2.txt is an update of Alan's original patch, with no changes other than those required to apply cleanly to latest master as of Feb 22, 2012. Compiles fine and runs without test errors, and does fix the behavior of the one new unit test included.

Comment by Andy Fingerhut [ 23/Mar/12 6:30 PM ]

Approval was changed from Test to Incomplete when it was waiting on Rich's feedback. He did give his feedback, but Approval never changed back to Test. Doing that now in hopes it makes the ticket more accurately categorized by filters. Feel free to give me a stern warning for mucking with the Approval field if I'm out of line here.

Comment by Andy Fingerhut [ 18/Jun/12 2:42 PM ]

clj-757-fix-behavior-of-empty-transient-maps-patch3.txt dated June 18, 2012 is identical to clj-757-fix-behavior-of-empty-transient-maps-patch2.txt, except it has some updated context lines so that it applies cleanly with latest Clojure master. I will remove the -patch2.txt patch since it no longer applies cleanly.

Comment by Fogus [ 17/Aug/12 9:56 AM ]

The patch in clj-757-fix-behavior-of-empty-transient-maps-patch3.txt is straight-forward and so is its test. This fixes only the behavior listed in the ticket and does not address the the general transient lookup behavior.





[CLJ-745] gen-class should allow exposes-methods to expose protected final methods Created: 25/Feb/11  Updated: 18/Aug/12  Resolved: 18/Aug/12

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

Type: Enhancement Priority: Major
Reporter: Daniel Solano Gómez Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: None

Attachments: File expose-protected-final-gen-class-clojure1.4.diff     Text File expose-protected-final-gen-class.diff    
Patch: Code and Test
Approval: Ok

 Description   

Currently, there is no way in Clojure to call a protected final method of a superclass. While this may be an acceptable limitation for proxy, gen-class should provide that ability. Otherwise, one is now forced to create a dummy subclass in Java for the sole purpose of widening the visibility of such methods.

My patch makes it so that protected final methods may be exposed via the :exposes-methods mechanism. It is still impossible to override them.



 Comments   
Comment by Chouser [ 19/Nov/11 4:13 PM ]

Updated patch to apply cleanly to Clojure 1.4 SNAPSHOT. Included tests look good and pass.

Comment by Chouser [ 16/Aug/12 10:26 PM ]

Patch still applies, tests still pass.





[CLJ-730] Create test suite for functional fns (e.g. juxt, comp, partial, etc.) Created: 26/Jan/11  Updated: 26/Jul/13  Resolved: 18/May/12

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

Type: Task Priority: Minor
Reporter: Fogus Assignee: Fogus
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-add-tests-for-complement-constantly-juxt-partial.patch    
Approval: Ok

 Description   

The tests in other_functions.clj for the combinator functions are tenuous in some cases (comp) and missing in others (juxt, partial, complement, and constantly). It would be nice to have a full suite of combinator tests moving forward.

I would be happy to write the needed tests.



 Comments   
Comment by Stuart Halloway [ 28/Jan/11 9:00 AM ]

Go for it, and mark them waiting for me when they are ready.

Comment by Federico Brubacher [ 22/Mar/11 11:07 AM ]

Michael, Stuart, Can I barge in with a patch? I would love to get feedback on this. I have CA signed already. Thanks
Federico





[CLJ-667] Allow loops fully nested in catch/finally Created: 31/Oct/10  Updated: 15/Jun/12  Resolved: 15/Jun/12

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

Type: Defect Priority: Minor
Reporter: Juha Arpiainen Assignee: Unassigned
Resolution: Completed Votes: 4
Labels: None

Attachments: Text File 0001-Allow-loop-recur-nested-in-catch-and-finally.patch     Text File clj-667-allow-loop-recur-nested-in-catch-and-finally-patch2.txt    
Patch: Code and Test
Approval: Ok

 Description   

~/clj-git/clojure $ java -cp clojure.jar clojure.main
Clojure 1.3.0-alpha2-SNAPSHOT
user=> (try (catch Exception e (loop [x 0] (recur x))))
CompilerException java.lang.UnsupportedOperationException: Cannot recur from catch/finally, compiling:(NO_SOURCE_PATH:1)
user=> (try (finally (loop [x 0] (if false (recur x)))))
CompilerException java.lang.UnsupportedOperationException: Cannot recur from catch/finally, compiling:(NO_SOURCE_PATH:2)

With attached patch (should also fix CLJ-31):

~/clj-head/clojure $ java -cp clojure.jar clojure.main
Clojure 1.3.0-alpha2-SNAPSHOT
user=> (try (catch Exception e (loop [x 0] (recur x))))
nil
user=> (try (finally (loop [x 0] (if false (recur x)))))
nil
user=> (loop [x 0] (try (if false (recur x))))
CompilerException java.lang.UnsupportedOperationException: Can only recur from tail position, compiling:(NO_SOURCE_PATH:3)
user=> (loop [x 0] (try (catch Exception e (recur x))))
CompilerException java.lang.UnsupportedOperationException: Can only recur from tail position, compiling:(NO_SOURCE_PATH:4)
user=> (loop [x 0] (try (finally (recur x))))
CompilerException java.lang.UnsupportedOperationException: Can only recur from tail position, compiling:(NO_SOURCE_PATH:5)



 Comments   
Comment by Andy Fingerhut [ 24/Feb/12 6:47 PM ]

clj-667-allow-loop-recur-nested-in-catch-and-finally-patch2.txt applies cleanly to latest master as of Feb 24, 2012.

I cannot vouch for the changes in Compiler.java – I simply modified what was in Juha Arpiainen's patch so that they applied cleanly. Some of the context changed noticeably due to a patch for CLJ-31 by Kevin Downey that was applied.

The test parts of the patch I can vouch for. Some of the previously existing tests seem not to test what they should. I can make a separate patch with just the updated passing tests if that is desired.





[CLJ-157] Need better error report when omitting parameter list from (fn) or (defn) Created: 20/Jul/09  Updated: 01/Mar/13  Resolved: 24/Aug/12

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

Type: Enhancement Priority: Minor
Reporter: Ambrose Bonnaire-Sergeant Assignee: Stuart Sierra
Resolution: Completed Votes: 7
Labels: None

Attachments: Text File 0001-Better-error-messages-for-syntax-errors-w-defn-and-f.patch     Text File 0002-Fix-redundant-conditional.patch     Text File clj-157-better-err-msgs-for-defn-fn-syntax-errors-patch2.txt    
Patch: Code and Test
Approval: Ok

 Description   

Somehow I just forgot to define parameters for my zero argument function and it took me a long time to notice it, mostly because I had been working with a lot of macros but especially because I didn't get an error that said "You must provide a list of parameter names", but instead: "Don't know how to create ISeq from: Symbol".



 Comments   
Comment by Assembla Importer [ 24/Aug/10 6:58 AM ]

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

Comment by Alex Miller [ 15/Dec/10 9:40 AM ]

I have hit this several times. Would be very nice (esp for newbs) to have a guard-rail here that detected this as a possible issue.

Comment by Kevin Sookocheff [ 30/Jun/11 9:32 AM ]

I've came across this as well and would welcome a more user friendly error message.

Comment by Stuart Sierra [ 05/Sep/11 1:38 AM ]

This is an easy mistake to make - I do it myself quite often. Better error messages for this common case would be good for new users.

CLJ-157 could probably be subsumed into a more general patch improving error messages about the expected arguments to fn.

defn is a macro which expands to fn, so a patch to fn should fix both.

Comment by Ambrose Bonnaire-Sergeant [ 05/Sep/11 11:43 PM ]

I'm working a patch for this.

Comment by Ambrose Bonnaire-Sergeant [ 07/Sep/11 7:31 AM ]

Is it ok to break the behavior of these two cases?

(fn a)
(fn)

So far I am throwing an error "Parameter list missing" instead of silently returning a function with AFAICT undefined behavior.

Comment by Ambrose Bonnaire-Sergeant [ 09/Sep/11 5:23 AM ]

I've attached a patch. There are some breaking changes for undefined behavior, I have detailed them in the commit message.

Comment by Stuart Halloway [ 10/Oct/11 7:10 AM ]

The trickiest thing with this kind of error reporting is guessing what the programmer was trying to do:

The greater the probability a random string is a valid program, the harder it is to report errors well.

http://www.paulgraham.com/redund.html
An example:

(defn add (a b) (+ a b))
IllegalArgumentException Parameter declaration a should be a vector  clojure.core/assert-valid-fdecl

Here the error message is based on a guess that I was trying to use the multi-arity syntax, but I was probably using the single-arity syntax with the wrong brackets. You might split these cases out by counting the top-level forms first.

I think I am cool with the change to (fn) and (fn a), but I can imagine a code generator relying on this behavior as a sentinel to avoid explicit coding for a corner case. What do others think?

One small thing in the code. Why the redundant boolean work here?

(if (instance? clojure.lang.Symbol name) false true)
Comment by Ambrose Bonnaire-Sergeant [ 10/Oct/11 7:48 AM ]
(if (instance? clojure.lang.Symbol name) false true)

It's so early in the bootstrap that both `not` and `symbol?` are not defined. That line is in the definition of `defn`. I added a comment documenting this.

Comment by Colin Jones [ 10/Oct/11 9:19 AM ]

I run into this all the time too - thanks for doing this, Ambrose!

Stuart's example is an interesting one - that could have meant at least one other thing in addition to multi-arity syntax and single-arity syntax with accidentally-list params: single-arity syntax with missing param list, and two successive body forms (where a and b are free variables). I think the error message here would still be an improvement over "Don't know how to create ISeq from: Symbol", but maybe it tries to be too specific in talking about "a". I'd tend toward being more generic and just saying "Parameter declaration should be a vector", which I think would be helpful and correct in all the intended cases we've imagined for this particular example.

I think breaking (fn) / (fn a) makes sense - if there's a valid use for those I'd be interested in seeing it. Relying on ArityException in code generation seems fairly error-prone to me, but I could easily be missing something here.

To the boolean point, I tend to agree with Stuart and would prefer reversing the if/else clauses to avoid it:

(if (instance? clojure.lang.Symbol name)
  nil      
  (throw (IllegalArgumentException. "First argument to defn must be a symbol")))
Comment by Ambrose Bonnaire-Sergeant [ 10/Oct/11 9:45 AM ]

Doh, of course Colin's suggestion is preferable.

Comment by Ambrose Bonnaire-Sergeant [ 08/Nov/11 3:29 PM ]

Added a second patch to fix redundant conditional

Comment by Ambrose Bonnaire-Sergeant [ 08/Nov/11 6:32 PM ]

Another breaking change to undefined behavior I didn't realize. (defn a) now throws an exception, instead of silently returning a function.

Comment by Carin Meier [ 27/Nov/11 2:54 PM ]

I checked out the changes and tried it out successfully. I think it is a great improvement. My only comment is regarding the text of the error message for

user=> (defn [x])
IllegalArgumentException First argument to defn must be a symbol  clojure.core/defn (core.clj:273)

It might be helpful to newcomers to mention that the parameter is the name. Something like, "Name argument to defn must be a symbol".

Comment by Stuart Sierra [ 28/Nov/11 4:28 PM ]

Vetted. Moving to "Approved Backlog" because it's an enhancement we would like to have in the next release.

Comment by Stuart Sierra [ 28/Nov/11 4:28 PM ]

Changing priority to "Minor"

Comment by Andy Fingerhut [ 07/Mar/12 7:49 PM ]

clj-157-better-err-msgs-for-defn-fn-syntax-errors-patch2.txt has no substantive changes from Ambrose's two patches. It does combine them into one commit, and unlike the older ones applies cleanly to latest master as of March 7, 2012.

Comment by Stuart Sierra [ 17/Aug/12 3:12 PM ]

Screened.

Comment by Stuart Sierra [ 24/Aug/12 8:34 AM ]

Patch applied.





[CLJ-103] GC Issue 99: Incorrect error with if-let Created: 17/Jun/09  Updated: 18/May/12  Resolved: 18/May/12

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

Type: Enhancement Priority: Minor
Reporter: Jeff Rose Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-Fix-CLJ-103-Incorrect-error-with-if-let.patch     Text File clj-103-improved-if-let-error-message-patch.txt    
Patch: Code
Approval: Ok

 Description   
Reported by rosejn, Mar 19, 2009

Example:
user=> (if-let [foo (+ 1 2)] foo + 1 foo)
java.lang.IllegalArgumentException: if-let requires a vector for its
binding (NO_SOURCE_FILE:2)

The error reports that if-let requires a vector for its binding, but the
problem here has nothing to do with the binding being a vector.  A paren
was forgotten before the '+', so the number of arguments is incorrect for
the if form.


 Comments   
Comment by Assembla Importer [ 24/Aug/10 6:45 AM ]

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

Comment by Assembla Importer [ 24/Aug/10 6: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 John Szakmeister [ 27/Oct/10 9:15 PM ]

I have a patch that gives a better error message. It will now say: "IllegalArgumentException if-let requires 1 or 2 forms to be passed". I'm not sure if that's a better error message or not, but I'm happy to tweak it.

Comment by John Szakmeister [ 27/Oct/10 9:16 PM ]

First iteration of the patch.

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

clj-103-incorrect-if-let-error-patch2.txt is same as John's except updated to apply cleanly to latest master as of Feb 23, 2012. No compiler errors or warnings, no test failures. No new tests for this one – I could write one, but the only way to do so is to check the contents of the error message in the exception thrown, which seems a bit too specific. The author John Szakmeister is on the contributor list.

Comment by Brenton Ashworth [ 20/Apr/12 5:19 PM ]

The provided patch from John and updated by Andy

clj-103-incorrect-if-let-error-patch2.txt

applies cleanly and doesn't cause any problems. I don't like the wording of the error message: "requires 1 or 2 forms to be passed" so I added another patch

clj-103-improved-if-let-error-message-patch.txt

which is the same change but with the wording "requires 1 or 2 forms after binding vector" which I think is a little more clear.

With this change, a user would now get the following error messages:

> (if-let foo)
ArityException 
Wrong number of args (1) passed to: core$if-let 
clojure.lang.Compiler.macroexpand1 (Compiler.java:6371)

> (if-let foo (+ 1 3))
IllegalArgumentException 
clojure.core/if-let requires a vector for its binding in clojure.core: 
clojure.core/if-let (NO_SOURCE_FILE:124)

> (if-let [foo (+ 1 3)])
ArityException
Wrong number of args (1) passed to: core$if-let 
clojure.lang.Compiler.macroexpand1 (Compiler.java:6371)

> (if-let [foo (+ 1 3)] foo 7 5)
IllegalArgumentException
if-let requires 1 or 2 forms after binding vector in clojure.core:142
clojure.core/if-let (NO_SOURCE_FILE:124)

> (if-let [foo (+ 1 3) bar 7] foo 7)
IllegalArgumentException
if-let requires exactly 2 forms in binding vector in clojure.core:143
clojure.core/if-let (NO_SOURCE_FILE:124)
Comment by John Szakmeister [ 20/Apr/12 6:40 PM ]

I like the improved wording. I struggled with that myself. Thanks Brenton!

Comment by Andy Fingerhut [ 24/Apr/12 7:28 PM ]

Removed obsolete patch from Feb 23 2012. Preference should be given to patch clj-103-improved-if-let-error-message-patch.txt dated Apr 20 2012 instead.





Generated at Thu Sep 18 19:00:52 CDT 2014 using JIRA 4.4#649-r158309.