<< Back to previous view

[CLJ-953] drop-while doc string wrong, nil instead of logical false Created: 15/Mar/12  Updated: 30/Mar/12  Resolved: 30/Mar/12

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

Type: Enhancement Priority: Trivial
Reporter: Eric Schoonover Assignee: Alan Dipert
Resolution: Completed Votes: 0
Labels: documentation, patch,

Attachments: File take-while_doc_string_CLJ-953.diff    
Patch: Code
Approval: Ok

 Description   
"Returns a lazy sequence of the items in coll starting from the first
-  item for which (pred item) returns nil."
+  item for which (pred item) returns logical false."





[CLJ-931] Syntactically broken clojure.test/are tests succeed Created: 16/Feb/12  Updated: 23/Mar/12  Resolved: 23/Mar/12

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

Type: Defect Priority: Minor
Reporter: Tassilo Horn Assignee: Stuart Sierra
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File 0001-Fix-CLJ-931-Syntactically-broken-clojure.test-are-te.patch     Text File clj-931-error-on-bad-are-syntax-patch2.txt    
Patch: Code
Approval: Ok
Waiting On: Rich Hickey

 Description   

While clojure.test/are is a very useful macro, it has one major flaw. If the assertion is syntactically incorrect, the test succeeds. Take this testcase:

(deftest broken-test
  (are [a b c] (= a b c)
       1 1))

See the error? The are form takes three values, but I have provided only two. The test simply passes.

Latest patch checks the number of arguments to are and throws an exception if they don't match.



 Comments   
Comment by Andy Fingerhut [ 16/Feb/12 11:36 AM ]

Is this the same issue as CLJ-806? If so, it might be nice to close one with a comment that it is a duplicate of the other. Given CLJ-806 has no patch, and this one does, perhaps preferable to close CLJ-806?

Comment by Tassilo Horn [ 16/Feb/12 11:46 AM ]

John, you are right. I didn't find it searching for "test are" earlier today; now I do.

I did as you've suggested: closed CLJ-806 in favour of this one.

Comment by Andy Fingerhut [ 17/Feb/12 2:27 AM ]

I've tested this patch by hand, and it seems to work fine, and the code looks correct. I have tried several ways to add a unit test to verify this fix, but haven't found the right way to do it, if there is one.

Comment by Tassilo Horn [ 17/Feb/12 2:58 AM ]

I'm working on it...

Comment by Tassilo Horn [ 17/Feb/12 4:10 AM ]

This patch obsoletes the former on (I'm gonna delete the old one). It includes:

  • an extended fix that catches also the zero-arg given case
  • a fix for a testcase in test/clojure/test_clojure/java_interop.clj which only passed because of the bug this issue is all about
  • a currently #_-disabled test case for testing `are'. It is disabled, because it fails when run with "ant test". However, it works just fine when run inside some REPL. I presume that's because ant runs inside its own JVM instance and thus makes catching exceptions from macroexpand/eval forms impossible?!?
Comment by Tassilo Horn [ 17/Feb/12 4:36 AM ]

Another minor update. Now (are [] true) is allowed (but meaningless). In the previous patch, you got a "divide by zero" in this case.

Comment by Andy Fingerhut [ 09/Mar/12 9:29 AM ]

Patch 0001-Fix-CLJ-931-Syntactically-broken-clojure.test-are-te.patch applies, builds, and tests cleanly as of March 9, 2012 latest master. One author Tassilo has signed CA.

Comment by Stuart Sierra [ 09/Mar/12 10:11 AM ]

I can't make the test work either. The patch looks good. Just remove the broken test instead of leaving it commented out.

Comment by Andy Fingerhut [ 09/Mar/12 10:24 AM ]

clj-931-error-on-bad-are-syntax-patch2.txt same as previous one, except removes commented out test. Applies, builds, and tests cleanly to latest master.

Comment by Stuart Sierra [ 09/Mar/12 10:37 AM ]

Screened.





[CLJ-930] cljs.core// does not work in ClojureScript Created: 08/Feb/12  Updated: 01/Mar/13  Resolved: 29/Feb/12

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

Type: Defect Priority: Major
Reporter: David Nolen Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None

Attachments: Text File cljs_slash.patch    
Patch: Code

 Description   

clojure.core// is hard coded into the reader as a special case. cljs.core// should similarly be hard coded



 Comments   
Comment by Andy Fingerhut [ 24/Feb/12 8:02 PM ]

David, the patch for CLJ-873 seems to be more general than this, i.e. it allows the symbol / to be used in any namespace, not only in clojure.core and cljs.core.

Comment by Andy Fingerhut [ 29/Feb/12 1:25 PM ]

I checked with David Nolen, and he agrees that the patch for CLJ-873 fixes not only this issue, but also enables the symbol / to be used in any namespace.





[CLJ-924] Error reporting of invalid digit in unicode character literal Created: 05/Feb/12  Updated: 23/Mar/12  Resolved: 23/Mar/12

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

Type: Defect Priority: Minor
Reporter: Cosmin Stejerean Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File CLJ-924-unicode-invalid-digit.patch     Text File clj-924-unicode-invalid-digit-patch2.txt    
Patch: Code
Approval: Ok

 Description   

I ran across the following code when reading unicode character literals in readUnicodeChar(String token, int offset, int length, int base)

 
int d = Character.digit(token.charAt(i), base);
if(d == -1)
    throw new IllegalArgumentException("Invalid digit: " + (char) d);

Casting -1 to a char doesn't seem to produce anything useful. I'm guessing the appropriate code might look like

 
int d = Character.digit(token.charAt(i), base);
if(d == -1)
    throw new IllegalArgumentException("Invalid digit: " + token.charAt(i));

For comparison, the code in the other readUnicodeCharacter used when reading strings handles this correctly with:

int d = Character.digit(ch, base);
if(d == -1)
    throw new IllegalArgumentException("Invalid digit: " + (char) ch); 


 Comments   
Comment by Cosmin Stejerean [ 05/Feb/12 1:09 AM ]

Attached a patch with the minor code fix.

Comment by Stuart Sierra [ 24/Feb/12 3:00 PM ]

Screened. I can't demonstrate this code being called by the reader, but it seems to be correct.

Comment by Andy Fingerhut [ 27/Feb/12 10:08 PM ]

Sorry to disturb things after this has been screened, but clj-924-unicode-invalid-digit-patch2.txt has some added tests that cause the problem to occur, as well as one other similar one nearby in LispReader.java, which is also fixed, in addition to Cosmin's fix.





[CLJ-914] Add support for UUID literals using tagged literal capability. Created: 20/Jan/12  Updated: 01/Mar/13  Resolved: 03/Feb/12

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

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

Attachments: File CLJ-914-uuid-literals.diff     Text File CLJ-914-uuid-literals-update.patch    
Patch: Code and Test
Waiting On: Stuart Sierra

 Description   

Using the tagged literal facility added in Clojure 1.4-alpha4, add support for a UUID literal of the form:

#uuid "550e8400-e29b-41d4-a716-446655440000"

;=> #<UUID 550e8400-e29b-41d4-a716-446655440000>

The default object built with this literal would be a java.util.UUID. A print-method for this type would also be required for this enhancement.



 Comments   
Comment by Fogus [ 27/Jan/12 12:02 PM ]

Added updated patch based on fixes to CLJ-871. This patch should be applied AFTER the patch for the latest patch for CLJ-871.





[CLJ-898] Agent sends consume heap Created: 16/Dec/11  Updated: 01/Mar/13  Resolved: 27/Jan/12

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

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

Attachments: Text File 0001-Only-capture-a-shallow-copy-of-the-current-Frame-in-.patch     Text File 0002-Fix-the-already-pushed-fix-for-CLJ-898.patch    
Patch: Code
Approval: Ok

 Description   

Simple demonstration:

(defn update [state]
  (send *agent* update)
  (inc state))

(def a (agent 1))

(send a update)

On Clojure 1.2.1, this runs forever with no problem. On 1.3.0, it throws "java.lang.OutOfMemoryError: Java heap space."

The problem appears to be clojure.core/binding-conveyor-fn: each send creates a new Var binding frame, and nested send creates an infinite stack of frames.

Also discussed at https://groups.google.com/d/topic/clojure/1qUNPZv3OYA/discussion



 Comments   
Comment by Tassilo Horn [ 16/Dec/11 11:46 AM ]

I've just checked the code: send uses binding (which calls push-thread-bindings, creating a new Frame with non-null prev) inside which the binding-conveyor-fn captures the current frame. When the binding-conveyor-fn runs, the old Frame is restored, and since the wrapped function sends again, a new thread-binding is pushed on top of that.

Hm, one way to get rid of the issue was not to capture the "real" current Frame in the binding-conveyor-fn but only a shallow copy, i.e., a Frame with the same bindings but no prev. I've tried that out, all tests still pass, and the example above won't grow heap without bounds. Patch attached.

Comment by Stuart Sierra [ 16/Dec/11 2:30 PM ]

Screened.

Comment by Tassilo Horn [ 25/Jan/12 2:24 PM ]

My fix had some small issue as David Miller pointed out correctly in

https://github.com/clojure/clojure/commit/12f07da889819bc5613546ec223e97ac27c86dbf#commitcomment-867608

Attached is a patch that fixes the quirk.

Comment by Tassilo Horn [ 25/Jan/12 2:25 PM ]

Here's the fix for the fix.





[CLJ-886] java.io/do-copy can garble multibyte characters Created: 28/Nov/11  Updated: 23/Mar/12  Resolved: 23/Mar/12

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

Type: Defect Priority: Major
Reporter: Jeff Palmucci Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: patch,
Environment:

all


Attachments: File clj-886.diff     Text File CLJ-886-fix2.patch    
Patch: Code and Test
Approval: Ok
Waiting On: Rich Hickey

 Description   

See comments in fix:

(defmethod do-copy [InputStream Writer] [#^InputStream input #^Writer output opts]
;; WRONG! if the buffer boundry falls in the middle of a multibyte character, we will get garbled results.
#_
(let [#^"[B" buffer (make-array Byte/TYPE (buffer-size opts))]
(loop []
(let [size (.read input buffer)]
(when (pos? size)
(let [chars (.toCharArray (String. buffer 0 size (encoding opts)))]
(do (.write output chars)
(recur)))))))
;; here we decode the characters before stuffing them into the buffer
(let [#^"[C" buffer (make-array Character/TYPE (buffer-size opts))
in (InputStreamReader. input (encoding opts))]
(loop []
(let [size (.read in buffer 0 (alength buffer))]
(if (pos? size)
(do (.write output buffer 0 size)
(recur)))))))



 Comments   
Comment by Jeff Palmucci [ 05/Dec/11 11:23 AM ]

Make patch file per http://clojure.org/patches. Also sent in my CA.

Comment by Jeff Palmucci [ 19/Dec/11 8:53 AM ]

Any reason why this hasn't been applied yet? It's a pretty serious error. Just wondering if I've submitted anything incorrectly. Thanks.

Comment by Andy Fingerhut [ 09/Feb/12 8:13 PM ]

CLJ-886-fix2.patch includes Jeff Palmucci's fix, plus another one found while enhancing clojure.java.io's unit tests to fail with the original code. Tested on Mac OS X 10.6.8 with java 1.6.0_29 and Ubuntu Linux 11.04 with JVM 1.7.0_02.

Comment by Jeff Palmucci [ 13/Feb/12 7:29 AM ]

Andy's fix is good. Please use the fix2 patch instead of the original.

Comment by Stuart Sierra [ 17/Feb/12 2:55 PM ]

Screened. Patch applies successfully and the tests seem to be complete.





[CLJ-885] clojure.java.io/Coercions doesn't handle URL-escaping Created: 27/Nov/11  Updated: 09/Dec/11  Resolved: 09/Dec/11

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

Type: Defect Priority: Minor
Reporter: Colin Jones Assignee: Unassigned
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File 0001-Handle-URL-escaping-in-both-directions-in-java.io-Co.patch     Text File 0002-Handle-URL-escaping-in-both-directions-in-java.io-Co.patch    
Patch: Code and Test
Approval: Ok

 Description   

clojure.java.io/Coercions implementations for URL -> File and File -> URL don't take URL escaping into account, and I think they should.

This breaks things like slurping a resource file that has spaces in the path.

Example behavior here: https://gist.github.com/1398972

I will post a link to the clojure-dev list discussion in the comments once I have the link for this ticket to post there.



 Comments   
Comment by Colin Jones [ 27/Nov/11 10:37 PM ]

clojure-dev posting with more details (and presumably forthcoming discussion): https://groups.google.com/d/topic/clojure-dev/bTNX4pt_b4w/discussion

Comment by Colin Jones [ 30/Nov/11 12:31 PM ]

Updated patch to fix the issue David Powell brought up on the dev list, so that "+" in a URL does not get translated to " " in a File.

Comment by Stuart Sierra [ 30/Nov/11 2:04 PM ]

Vetted & assigned to "Approved Backlog" in preparation for next release.





[CLJ-876] #^:dynamic vars declared in a nested form are not immediately dynamic Created: 14/Nov/11  Updated: 09/Dec/11  Resolved: 09/Dec/11

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

Type: Enhancement Priority: Minor
Reporter: Micah Martin Assignee: Micah Martin
Resolution: Completed Votes: 0
Labels: None
Environment:

All


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

 Description   

http://groups.google.com/group/clojure/browse_thread/thread/890202da5db0fe1/e34819de71c6ea24?hl=en&lnk=gst&q=micah#e34819de71c6ea24

The following snippet throws an error stating that p is unbound.

(list
(declare ^:dynamic p)
(defn q [] @p))
(binding [p (atom 10)]
(q))



 Comments   
Comment by Micah Martin [ 14/Nov/11 10:10 AM ]

Rich fixed: https://github.com/clojure/clojure/commit/535907eb2be47eaee0c385ae16436f39d52ffa96

Comment by Stuart Halloway [ 02/Dec/11 2:01 PM ]

test for fix already made





[CLJ-872] Add support for property lookup Created: 04/Nov/11  Updated: 01/Mar/13  Resolved: 16/Dec/11

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

Type: Enhancement Priority: Major
Reporter: Alan Dipert Assignee: Fogus
Resolution: Completed Votes: 1
Labels: None

Attachments: Text File 872-add-prop-lookup.patch     Text File CLJ-872-prop-lookup-tests.patch    
Patch: Code and Test
Approval: Ok
Waiting On: Stuart Halloway

 Description   

Add support for property lookups to match functionality introduced in ClojureScript with CLJS-89.

This is a breaking change for Clojure with record/type fields that start with "-".



 Comments   
Comment by Alan Dipert [ 04/Nov/11 4:17 PM ]

Attached 872-add-prop-lookup.patch. It's a first cut that Fogus and I worked on. It needs tests.

Comment by Fogus [ 09/Nov/11 12:03 PM ]

Tests for prop lookup.

Comment by Fogus [ 16/Nov/11 9:10 AM ]

Ready for screening.

Comment by Stuart Halloway [ 09/Dec/11 10:29 AM ]

Can you explain the compiler change? It appears to implement the desired functionality, but also to disable a code path that allows keywords for field lookup (the block at line 892 can no longer be reached by a keyword).

Comment by Fogus [ 09/Dec/11 12:28 PM ]

Clojure has for a while secretly allowed kw lookup for field access. However, in the discussions with Rich and Clojure/dev at http://dev.clojure.org/display/design/Unified+ClojureScript+and+Clojure+field+access+syntax it was decided to use -prop across CLJ and CLJS thus obviating the need for kw access.





[CLJ-871] instant literal Created: 04/Nov/11  Updated: 01/Mar/13  Resolved: 03/Feb/12

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

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

Attachments: Text File 0871-instant-reader.patch     Text File CLJ-871-data-readers-1.patch     File CLJ-871-jre-only.diff     Text File CLJ-871-with-defaults.patch     File CLJ-915-instant-literals.diff    
Patch: Code and Test
Waiting On: Stuart Sierra

 Description   

See http://dev.clojure.org/pages/viewpage.action?pageId=950382 for design discussion.



 Comments   
Comment by Stuart Halloway [ 04/Nov/11 12:59 PM ]

I have tested patch locally with no Joda, 1.6.2 Joda, and 2.0 Joda. All work as intended in simple invocations.

Seeking feedback on

  • UTC at the door. Goal is to stay out of localization business. Is that a good goal, and is that the right way to achieve it?
  • idiom for dynamically loading code based on whether Joda on classpath
  • idiom for conveying maven classpath into the ant part of the build. People using ant will have to configure path locally to include Joda.
Comment by Kevin Downey [ 04/Nov/11 1:37 PM ]

using a bindable var for a read time setting is kind of a drag stuff like:

(binding [*instant-reader* some-other-instant-reader]
#@2011-11-04T14:42Z)

will not have the desired effect, so then what are the use cases for the bindable var? should the repl bind it? should the compiler?

Comment by Kevin Downey [ 04/Nov/11 6:47 PM ]

dates should always be read and printed as UTC

Comment by Aaron Brooks [ 05/Nov/11 9:55 AM ]

It seems like it would be nice to have time-delta literals as well. My usage cases deal more often with time-deltas applied as intervals or offsets in time. I presume the current scheme doesn't allow for that but could be adjusted. Is that outside of the scope of this discussion?

Comment by Rich Hickey [ 06/Nov/11 10:31 AM ]

> UTC at the door.

No. Offsets are not localization, just arithmetic.

>idiom for dynamically loading code based on whether Joda on classpath

We'll need more explicit mechanisms for determining the programmatic representation. Dynamic Joda is just to avoid Joda as hard implementation dep.

> People using ant will have to configure path locally to include Joda.

People using ant = me.

Comment by Ben Smith-Mannschott [ 12/Nov/11 3:10 PM ]

This patch implements instant literals of the form #@yyyy-mm-ddThh:mm:ss.fff+hh:mm using only classes available in the JRE.

clojure.instant provides instant-readers producing instances of three different JDK classes. These functions accept a string representing a timestamp See (doc clojure.instant/parse-timestamp) for details.

  • read-instant-date (java.util.Date)
  • read-instant-calendar (java.util.Calendar)
  • read-instant-timestamp (java.sql.Timestamp)

By default *instant-reader* is bound to read-instant-date.

print-method and print-dup are provided for all three types.

Rough bits include:

I'm not yet certain about the exact public interface of clojure.instant. It's clear that read-instant-* need to be visible. It also seems likely that parse-timestamp and validated could usefully support alternate implementations for *instant-reader*.

fixup-offset and fixup-nanos are ugly warts necessitated by Java's pathetic built-in support for dates and times (possibly exacerbated by my own misunderstandings of the same).

Unit tests are very basic. For example, I'm not testing validated except in the good case where everything is valid.

See also https://github.com/bpsm/clojure/commit/753f991151847df53d624f7c09b7113cd2321793


I've made a few trivial fixes to doc strings, visible on my github branch. Those changes will be included when I re-roll the patch it to incorporate any feedback.

Comment by Stuart Sierra [ 13/Nov/11 6:53 PM ]

CLJ-871-data-readers-1.patch

An alternative approach. The dynamic Var *data-readers* maps keywords to reader functions, triggered by reader syntax #:keyword. Default #:instant reader takes a vector like [year month day hrs min sec ms], all components optional.

Pros: No string parsing. Extensible.

Cons: Clojure 1.2 used the #: syntax to print records.

Comment by Fogus [ 21/Jan/12 9:11 AM ]

Oddly, I made another case for this. My bad. Attached is the updated patch based off of Ben's work. This patch is merged into the tagged literal feature as implemented in v1.4-alpha4. I also fixed a fence-post error and very minor doc problems. The parser currently defaults to constructing j.u.Date instances of the following form:

date-year   = 4DIGIT
date-month      = 2DIGIT  ; 01-12
date-mday       = 2DIGIT  ; 01-28, 01-29, 01-30, 01-31 based on
                         ; month/year
time-hour       = 2DIGIT  ; 00-23
time-minute     = 2DIGIT  ; 00-59
time-second     = 2DIGIT  ; 00-58, 00-59, 00-60 based on leap second
                         ; rules
time-secfrac    = '.' 1*DIGIT
time-numoffset  = ('+' / '-') time-hour ':' time-minute
time-offset     = 'Z' / time-numoffset

time-part            = time-hour [ ':' time-minute [ ':' time-second [time-secfrac] [time-offset] ] ]

timestamp       = date-year [ '-' date-month [ '-' date-mday [ 'T' time-part ] ] ]

Or in other words, RFC 3339 .

Comment by Stuart Sierra [ 27/Jan/12 8:46 AM ]

2 problems:

1. You can't specify an alternate inst reader in data_readers.clj. Doing so causes an opaque error when Clojure starts. This is a flaw in CLJ-890

2. *data-readers* is intended to be a map of symbols to Vars, not symbols to functions. This doesn't really matter.

Comment by Stuart Sierra [ 27/Jan/12 9:24 AM ]

New patch CLJ-871-with-defaults.patch.

Fixes problems described in previous comment by adding default-data-readers which can be overridden by *data-readers*. Also adds documentation for *data-readers*.

Comment by Steve Miner [ 01/Feb/12 2:25 PM ]

divisible? and indivisible? should be normal functions, not macros. They're used only in leap-year? – it would be pretty simple to use zero? and mod? directly there.





[CLJ-868] core min and max should behave predictably when args include NaN Created: 01/Nov/11  Updated: 09/Dec/11  Resolved: 09/Dec/11

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

Type: Defect Priority: Minor
Reporter: Ben Smith-Mannschott Assignee: Ben Smith-Mannschott
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJ-868-contagious-NaN.patch     Text File CLJ-868-ignore-NaN.patch     Text File CLJ-868-throw-exception-on-NaN.patch    
Patch: Code and Test
Approval: Ok

 Description   

The min and max functions in clojure.core behave unpredictably when one or more of their arguments is Float/NaN or Double/NaN. This is because the current implementation assumes that > provides a total ordering, but this is not the case when NaN is added to the mix. This is an unfortunate fact of life when dealing with IEEE floating point numbers.

See also the recent mailing list thread "clojure.core/max and NaN".

May be related to issue CLJ-738.



 Comments   
Comment by Ben Smith-Mannschott [ 01/Nov/11 10:05 AM ]

It seems to me that there are four approaches one might take to address this.

  1. Document the current undefined behavior of min and max in the presence of NaN.
  2. Define that min and max will return NaN if any argument is NaN.
  3. Define that min and max will ignore any NaN arguments.
  4. Define that min and max will throw an exception if any argument is NaN.

1 Document current behavior as undefined

This requires no changes in the implementation, but it doesn't strike me as a desirable resolution. Why unnecessarily codify clearly confusing behavior?

2 Make NaN contagious

Define min and max to return NaN if and only if at least one of their arguments is NaN. This seems most in keeping with the (admittedly perverse) behavior of NaN as specified.

See JLS 4.2.4 Floating Point Operations:

An operation that overflows produces a signed infinity, an operation that underflows produces a denormalized value or a signed zero, and an operation that has no mathematically definite result produces NaN. All numeric operations with NaN as an operand produce NaN as a result. As has already been described, NaN is unordered, so a numeric comparison operation involving one or two NaNs returns false and any != comparison involving NaN returns true, including x!=x when x is NaN.

3 Let min and max ignore NaN arguments

This means that (min a NaN b) would be exactly equivalent to (min a b). It would further imply that (min NaN) would be equivalent to (min), which currently throws an exception.

4 Let NaN cause min and max to throw an exception

Currently min and max throw an exception if given arguments that are not Numeric. One might plausibly argue that NaN is not numeric.

Comment by Ben Smith-Mannschott [ 01/Nov/11 11:05 AM ]

CLJ-868-contagious-NaN.patch

(Implementation corresponds to variant 2 from my earlier comment.)

Comment by Ben Smith-Mannschott [ 01/Nov/11 1:12 PM ]

This implements the third variant described above with one difference:

When when all arguments are NaN we don't throw a exception, but return NaN instead.

(min) and (max) still throw an exception, however.

This fell out of the implementation naturally and I couldn't see a good reason to prefer one behavior to another.

Comment by Stuart Halloway [ 02/Dec/11 12:38 PM ]

The contagious version of the patch seems to me the right approach, assuming fidelity to Java (e.g. Math/max and Math/min) is the standard.

Comment by Rich Hickey [ 09/Dec/11 9:28 AM ]

contagious ok





[CLJ-861] PersistentHashMap uses a hashing function that is incongruent with the equality function it uses Created: 23/Oct/11  Updated: 01/Mar/13  Resolved: 23/Oct/11

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

Type: Defect Priority: Major
Reporter: Paul Stadig Assignee: Rich Hickey
Resolution: Completed Votes: 0
Labels: None

Approval: Ok

 Description   

PersistentHashMap hashes its keys with Util.hash which simply calls Object.hashCode, but it compares its keys for equality using Util.equiv. This means:

user=> (clojure.lang.Util/equiv (Integer. -1) (Long. -1))
true
user=> (= (clojure.lang.Util/hash (Integer. -1)) (clojure.lang.Util/hash (Long. -1)))
false

which breaks the contract of a hash table (keys that are equal should hash to values that are equal).

The following bad behavior results:

user=> (def bad-map (clojure.lang.PersistentHashMap/create {(Long. -1) :here}))
#'user/bad-map
user=> (contains? bad-map (Integer. -1))
false
user=> (get bad-map (Integer. -1))
nil
user=> (= bad-map (clojure.lang.PersistentHashMap/create {(Integer. -1) :here}))
false

Compared to:

user=> (def bad-map (clojure.lang.PersistentHashMap/create {(Long. 0) :here}))
#'user/bad-map
user=> (contains? bad-map (Integer. 0))
true
user=> (get bad-map (Integer. 0))
:here
user=> (= bad-map (clojure.lang.PersistentHashMap/create {(Long. 0) :here}))
true

This bug also infects PersistentHashSet:

user=> #{(Long. 0) (Integer. 0)}
IllegalArgumentException Duplicate key: 0  clojure.lang.PersistentHashSet.createWithCheck (PersistentHashSet.java:56)
user=> #{(Long. -1) (Integer. -1)}
#{-1 -1}
user=> (contains? #{(Long. 0)} (Integer. 0))
true
user=> (contains? #{(Long. -1)} (Integer. -1))
false


 Comments   
Comment by Paul Stadig [ 23/Oct/11 5:24 AM ]

There may be a couple different ways to fix this. I think what would work is adding a Util.hashEquiv method that has the same equality semantics as Util.equiv and using that to hash keys instead.

I'd be glad to create a patch for that.

Comment by Paul Stadig [ 23/Oct/11 7:28 PM ]

This ticket can be closed now. It is fixed by https://github.com/clojure/clojure/commit/b5f5ba2e15dc2f20e14e05141f7de7c6a3d91179

Comment by Rich Hickey [ 23/Oct/11 7:28 PM ]

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





[CLJ-860] Add ability to disable locals clearing Created: 21/Oct/11  Updated: 01/Mar/13  Resolved: 13/Mar/12

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

Type: Enhancement Priority: Major
Reporter: Hugo Duncan Assignee: Unassigned
Resolution: Completed Votes: 7
Labels: None

Attachments: File 0001-add-compiler-options-map.diff     File add-disable-locals-clearing.diff     File add-enable-locals-clearing.diff    
Patch: Code
Approval: Ok

 Description   

When using a debugger, it is useful to be able to see all variable values. Locals clearing reduces the number of variabls that are non-nil, and makes uncertain whether nil values are real or an artifact of locals clearing.

Please add a mechanism for disabling locals clearing.

The attached patch does this by introducing the disable-locals-clearing var, and using it to initialise the canBeCleared field of LocalBinding.



 Comments   
Comment by Aaron Brooks [ 29/Oct/11 12:19 PM ]

+1

The ability to debug is greatly degraded when locals on lower stack frames have already been cleared. I'll apply this patch and will test in the next few days.

Comment by Stuart Halloway [ 02/Dec/11 12:43 PM ]

It seems we might have a number of new debug settings ahead. Are there other approaches besides filling the core namespace? Names in a different namespace? A single name in core that points to a map?

Comment by Phil Hagelberg [ 02/Dec/11 12:50 PM ]

We could have this triggered by system properties. We've talked about having a clojure.debug system property; it would make sense to have this set a number of other properties including one specifically for locals clearing.

Comment by Hugo Duncan [ 02/Dec/11 1:07 PM ]

From a user perspective, I would like to be able to control locals clearing on a fine grained basis. In slime, for instance, C-u C-c C-c could be used to compile with locals disabled. This would improve the possibilities for debugging code that required locals clearing to work properly.

To me this would suggest the use of a var (though the var could be held in a map).

Comment by Kevin Downey [ 02/Dec/11 1:21 PM ]

clojure.settings/disable-locals-clearing sounds good to me

Comment by David Santiago [ 02/Dec/11 2:58 PM ]

I much prefer either the clojure.settings namespace (or similar), or the map of options. I think what Hugo is describing as an important use case to keep in mind, but also, if Clojure itself has the facilities to handle this functionality, I don't see any reason to tie it to Java's property system. If compilation options proliferate, it doesn't seem unlikely to me that multiple target Clojures (CLR, CLJS) might also use similar flags, and by keeping it in Clojure it could make that simpler to deal with.

Hugo also pointed out to me privately that we could also make these vars be initialized from system properties, which seems to me like it would get a lot of the convenience you would get from having this based on system properties.

Finally, I'd just like to say that I think clojure.settings/locals-clearing, default: true sounds a little better to me. It would make code using that variable a little clearer because then the enable/disable matches the true/false value it holds.

Comment by Stuart Sierra [ 02/Dec/11 3:31 PM ]

The proliferation of dynamic Vars in core has concerned me too. Java System Properties are nice for Java interop, but they can only have String values and they lack thread-local binding semantics.

A dynamic map would work, but binding it correctly is more complicated:

(binding [*settings* (merge *settings* {...})] ...)

This could be mitigated with a macro.

A namespace for settings is convenient for documentation purposes, but what about the cost of dynamic Vars? If they're only used at compile-time, it doesn't really matter, but a single Var lookup is still cheaper than many.

Comment by Phil Hagelberg [ 13/Dec/11 12:11 PM ]

Are we really concerned about thread-locality when dealing with compiler settings? Concurrent compilation already has issues with transactionality, so I got the impression that it's not encouraged. I don't see the string limitation being a problem here, but perhaps there are other use cases where it would be annoyingly limiting?

Comment by Stuart Sierra [ 16/Dec/11 7:58 AM ]

Phil wrote: Are we really concerned about thread-locality when dealing with compiler settings?

We aren't necessarily concerned with thread-locality, although the compiler itself a lot of dynamically-scoped Vars internally. For external use, all we really need is scoped, temporary settings, so Java system properties would work with some helper macrology like with-properties.

The string limitation isn't a problem right now, but it could be annoying if we added something like CL feature-test expressions.

Comment by Hugo Duncan [ 24/Feb/12 12:34 PM ]

Inverts the logic of the previous patch and uses enable-locals-clearing.

I'm unclear as to what the consensus is on how to avoid var proliferation.

Comment by Hugo Duncan [ 24/Feb/12 4:19 PM ]

This patch implements compiler-options as a map with the :locals-clearing key.
The initialisation is via the value of the clojure.compile.locals-clearing system property.

Comment by Hugo Duncan [ 24/Feb/12 4:22 PM ]

0001-add-compiler-options-map.diff

Implements compiler-options as a map, with a :locals-clearing key, initialised from the clojure.compile.locals-clearing system property.

Comment by Andy Fingerhut [ 09/Mar/12 9:39 AM ]

Just a note that all 3 patches attached right now fail to apply cleanly using 'git --keep-cr -s < patchfile.txt' to latest master as of March 9, 2012. Any consensus on which of these approaches to take? I can update that one if so.

Comment by Rich Hickey [ 13/Mar/12 10:15 AM ]

https://github.com/clojure/clojure/commit/4036c7720949cb21ccf53c5c7c54ed1daaff2fda

Comment by George Jahad [ 21/Mar/12 12:14 AM ]

Per RH's request, I tested this with CDT, and confirmed it works there.





[CLJ-855] catch receives a RuntimeException rather than the expected checked exception Created: 11/Oct/11  Updated: 01/Mar/13  Resolved: 29/Feb/12

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

Type: Defect Priority: Major
Reporter: Paul Michael Bauer Assignee: Ben Smith-Mannschott
Resolution: Completed Votes: 2
Labels: None

Attachments: File checked-exception-regression-test.diff     Text File CLJ-855-sneaky-throw.patch     Text File CLJ-855_throw_undeclared_checked.patch     Text File CLJ-855-try-unwraps.patch    
Patch: Code and Test
Approval: Ok

 Description   

Expressions passed to try that trigger the use of clojure.lang.Reflector result in their thrown checked exceptions being wrapped in RuntimeException. As a result, the subsequent set of catches won't switch on the expected checked exception.

Attached: patch for regression test that exposes the problem

(defn- get-exception [expression]
  (try (eval expression)
    nil
    (catch java.lang.Throwable t
      t)))

(deftest catch-receives-checked-exception
  (are [expression expected-exception] (= expected-exception
                                          (type (get-exception expression)))
    "Eh, I'm pretty safe" nil
    '(java.io.FileReader. "CAFEBABEx0/idonotexist") java.io.FileNotFoundException)) ; fails

Clojure ML Thread: https://groups.google.com/forum/#!topic/clojure/I5l1YHVMgkI/discussion
Introduced: https://github.com/clojure/clojure/commit/8fda34e4c77cac079b711da59d5fe49b74605553



 Comments   
Comment by Ben Smith-Mannschott [ 12/Oct/11 2:06 AM ]

(marked up code snippet in description to display as code.)

Comment by Ben Smith-Mannschott [ 12/Oct/11 4:23 PM ]

(This incorporates a slightly modified version of Paul Michael Bauer's
checked-exception-regression-test patch.)

ClojureException is an RTE. RTEs that originate from Clojure all are
of this type. This isn't strictly necessary for this proof of concept,
but it seemed prudent since it seems that the lack of specificity resulting
from using plain RTE everywhere got us into this mess in the first place.

WrappedException is a ClojureException. It is used when Clojure is
wrapping the underlying cause to work around Java's obsession with
checked exceptions. It's always the underlying cause of
WrappedException that is relevant to catchers.

Util.runtimeException(s) now returns a ClojureException
This is not strictly necessary for this patch to work

Util.runtimeException(s,e) now returns a WrappedException.

Util.runtimeException(e) now retuns a WrappedException when e is not
already a RuntimeException.

clojure.core/treye is a macro built on top of the compiler-provided
special form try. (A real solution to this problem would involve
altering the try special form itself, but this ist just a trial balloon
to figure out if this is the right way to go.)

Paul Michael Bauer's try_catch.clj uses treye in place of try and passes.

Comment by Kevin Downey [ 12/Oct/11 5:34 PM ]

> http://james-iry.blogspot.com/2010/08/on-removing-java-checked-exceptions-by.html

seems like refactoring Reflector.java to rethrow using this approach
is preferable to some wrapping/unwrapping scheme

Comment by Paul Michael Bauer [ 12/Oct/11 5:42 PM ]

(edit: missed Kevin's post)
http://james-iry.blogspot.com/2010/08/on-removing-java-checked-exceptions-by.html

I'd rather see Reflector.java modified to use this "chucking" technique than have a special try form that specially unwraps an exception and re-throws.

Maybe there's a need for WrappedException, but since it's not immediately necessary if we fix Reflector, then I'd rather see it in a separate commit.

(I'm swamped at work today and don't have time to fix Reflector myself right now)

Comment by Herwig Hochleitner [ 12/Oct/11 9:27 PM ]

I added Util.throwUnchecked and Util.throwCause (also Util.declareThrows and Util.invokeThrowing) as justified in https://groups.google.com/d/msg/clojure-dev/4QynV81W0Qk/rIN9fYUK-AkJ
I've used this to fix the invoke* methods in Reflector. Please review.

There are a lot of other instances in the code, where checked exceptions are runtime-ified, but that's a separate issue.

Comment by Ben Smith-Mannschott [ 13/Oct/11 1:07 AM ]

Applying the patch CLJ-855_throw_undeclared_checked.patch produces java that can not be compiled in four places:

catch (Exception e)
  {
  Util.throwCause(e);
  }

This leaves the compiler complaining about a missing return.

catch (Exception e) 
  {
  return Util.throwCause(e);
  }

compiles.

Also, the patch doesn't actually cause try_catch.clj to pass. I suspect the RuntimeException being caught by try_catch.clj is not, in fact, originating from one of the four points you have corrected. (Indeed, I discovered something similar when working on my patch, but it was late, and I ended up solving it in such a way that I didn't have to find the actual source of this particular wrapped exception.) I'm still investigating...

Comment by Paul Michael Bauer [ 13/Oct/11 1:11 AM ]

@Herwig thanks.
A few issues with the patch:
1. doesn't build. Use "return Util.throwCause(...)" instead of just "Util.throwCause(...)" Otherwise the compiler complains about a missing return statement.
2. could you observe the whitespace conventions in the rest of the Java source files (tabs for indentation)?
3. The semantics of throwCause are different than before. Previously, we threw the Throwable only if t wasnt 1) null 2) an instance of Exception or 3) instance of Error. Subtle, and maybe it's a non-issue?

Comment by Ben Smith-Mannschott [ 13/Oct/11 1:21 AM ]

The wrapping RuntimeException that causes try_catch.clj to fail is actually coming out of Compiler.eval(Object,boolean) without involving Reflector at all.

CLJ-855 appears to be about more than just oddness in Reflector.java. My guess is it requires a more general solution.

Comment by Herwig Hochleitner [ 13/Oct/11 10:06 AM ]

Oh, it seems I was too fast with the second commit. That's kind of embarrassing, sorry about that.
Please only consider the first commit of the patch, where I implemented the chucked exception helpers.

I had considered the issue with only rethrowing causes that are instances of Exception or Error. I dismissed it, because I considered it an artifact of not having chucked exceptions before. The throwCause is for unwrapping InvokationTargetExceptions and such. The only case where it could unwrap differently than the original code, is when having causes other than Exception or Error. i.e. a third subclass of Throwable, which is mostly unheard of (but should probably be unwrapped too).

Rich, do you remember, if there are any further implications to the catch idiom in Reflector?

catch(Exception e)
    {
    if(e.getCause() instanceof Exception)
      throw Util.runtimeException(e.getCause());
    else if(e.getCause() instanceof Error)
      throw (Error) e.getCause();
    throw Util.runtimeException(e);
    }

Regarding the actual source of the wrapping (Compiler.eval): It showed up in my `rgrep catch(Exception`, but I considered it part of the "other issue" (of general refactoring). In fact, I think it won't show up, mostly, because people noticed this issue when having reflective calls in clojure code, which don't use eval. So the test case is probably testing another source of RuntimeException wrapping.

This shows, IMO, that most occurrences of `throw Util.runtimeException` should be replaced by `return Util.throwUnchecked`, but probably not all. This will require some scrutiny and careful testing. We need to consider every `catch (Exception e)` in the clj code base.

I'll take a stab at it, tonight (CEST), also testing properly this time (sorry again, had some troubles with my build system yesterday)

Comment by Ben Smith-Mannschott [ 13/Oct/11 4:21 PM ]

I've added a unit test which provokes this issue in the Reflector, as opposed to in the Compiler by way of eval. You can browse it here:

https://github.com/bpsm/clojure/commits/CLJ-855

I've refined my patch using the "try unwraps" strategy. The refined patch renames Clojure's try special form to try* and provides try with transparent unwrapping as a macro on top of try*. The curious may look here:

https://github.com/bpsm/clojure/commits/CLJ-855-try-unwraps

I'll post new patches after I've had some sleep.

I intend to try my hand at the "sneaky throws" strategy that seems to be all the rage here. My experimentation with Herwig's patch showed that it will require investigating some failing unit tests. I believe it will prove a more invasive change than "try unwraps", but may be worth it. A nice challenge.

Comment by Ben Smith-Mannschott [ 14/Oct/11 12:04 AM ]

CLJ-855-try-unwraps.patch:

  1. Teaches the Java portion of Clojure core to use WrappedException (a RuntimeException) when wrapping checked exceptions for re-throwing.
  2. Changes try to transparently unwrap (only!) WrappedException, passing the cause on to be handled by one of its catch clauses. This appears to solve CLJ-855.
  3. The original behavior of the try special form (no transparent unwrapping) is still available under the name try*.
Comment by Ben Smith-Mannschott [ 14/Oct/11 2:34 PM ]

CLJ-855-sneaky-throw.patch

Using "sneaky throw" allows us to effectively rethrow checked exceptions without needing to declare or catch them. In effect, we're back to where we were before wrapping them all up in RTEs (8fda34e4c77...), but without needing to pollute seemingly every method signature in the Java portion of Clojure core with "throws Exception"

This patch was less work than I expected (thanks be to sed) but I'm not as confident in its correctness as I am in that of the try-unwrap variant. I can't claim to have really groked Clojure's approach to exceptions.

I don't understand why Reflector gives preference to rethrowing the cause of the exception it catches (especially now that we're no longer wrapping all over the place.)

I also don't understand why Var.dissoc returns the exception it catches rather than throwing it. (I've left a big fat TODO comment there, which should go from any final version of this patch, should it be accepted.)

Comment by Paul Stadig [ 10/Feb/12 9:08 AM ]

Another option, not to make things even more complicated, is to revert the original commit. Perhaps I'm missing the rationale, but...

Checked exceptions are irrelevant to Clojure code, since they are a fabrication of the Java compiler.

Using Clojure code from the Java side could be confusing if you call invoke on IFn and get a checked exception even though it is not declared with a throws clause. It is also confusing to get checked exceptions wrapped in one or more layers of RTEs. Was it not sufficient the way it was before having IFn declare "throws Exception"?

Comment by Rich Hickey [ 29/Feb/12 3:06 PM ]

CLJ-855-sneaky-throw.patch applied. Thanks Ben!





[CLJ-854] flatten doc incorrect Created: 11/Oct/11  Updated: 09/Dec/11  Resolved: 09/Dec/11

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

Type: Defect Priority: Trivial
Reporter: Chris Rosengren Assignee: Ben Smith-Mannschott
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File CLJ-854.patch    
Patch: Code
Approval: Ok

 Description   

Usage: (flatten x)
Takes any nested combination of sequential things (lists, vectors,
etc.) and returns their contents as a single, flat sequence.
(flatten nil) returns nil.

doc string for (flatten nil) should be removed / altered.

(flatten nil)
;=> ()



 Comments   
Comment by Ben Smith-Mannschott [ 19/Oct/11 12:45 PM ]

attached patch to "document that (flatten nil) returns an empty sequence"





[CLJ-853] Typo in the doc of get-in function Created: 11/Oct/11  Updated: 25/Oct/11  Resolved: 25/Oct/11

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

Type: Defect Priority: Minor
Reporter: Jingguo Yao Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None


 Description   

In " where ks is a sequence of ke(ys.", "ke(ys" should be "keys".



 Comments   
Comment by Steve Miner [ 23/Oct/11 3:05 PM ]

The fix is not on the current master, and not in 1.4.0-alpha1.





[CLJ-852] IllegalArgumentException thrown when defining a var whose value is calculated with a primitive fn. Created: 10/Oct/11  Updated: 23/Mar/12  Resolved: 23/Mar/12

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

Type: Defect Priority: Critical
Reporter: Alexander Taggart Assignee: Ben Smith-Mannschott
Resolution: Completed Votes: 4
Labels: None

Attachments: Text File CLJ-852-fix.patch     Text File clj-852-patch2.txt     Text File CLJ-852-rfc-fix.patch     Text File CLJ-852-test.patch    
Patch: Code and Test
Approval: Ok
Waiting On: Rich Hickey

 Description   

Note the following stacktrace line numbers are from the latest 1.4-snapshot, though this happens on 1.3 as well.

Example:

user=> (defn foo ^long [^long x] x)
#'user/foo
user=> (def x (inc (foo 10)))
CompilerException java.lang.IllegalArgumentException: Unable to resolve classname: long, compiling:(NO_SOURCE_PATH:2) 
user=> (pst)
CompilerException java.lang.IllegalArgumentException: Unable to resolve classname: long, compiling:(NO_SOURCE_PATH:2)
	clojure.lang.Compiler.analyzeSeq (Compiler.java:6444)
	clojure.lang.Compiler.analyze (Compiler.java:6244)
	clojure.lang.Compiler.analyze (Compiler.java:6205)
	clojure.lang.Compiler.analyzeSeq (Compiler.java:6432)
	clojure.lang.Compiler.analyze (Compiler.java:6244)
	clojure.lang.Compiler.access$100 (Compiler.java:37)
	clojure.lang.Compiler$DefExpr$Parser.parse (Compiler.java:492)
	clojure.lang.Compiler.analyzeSeq (Compiler.java:6437)
	clojure.lang.Compiler.analyze (Compiler.java:6244)
	clojure.lang.Compiler.analyze (Compiler.java:6205)
	clojure.lang.Compiler.eval (Compiler.java:6497)
	clojure.lang.Compiler.eval (Compiler.java:6459)
Caused by:
IllegalArgumentException Unable to resolve classname: long
	clojure.lang.Compiler$HostExpr.tagToClass (Compiler.java:1003)
	clojure.lang.Compiler$InvokeExpr.getJavaClass (Compiler.java:3474)
	clojure.lang.Compiler.getMatchingParams (Compiler.java:2304)
	clojure.lang.Compiler$StaticMethodExpr.<init> (Compiler.java:1510)
	clojure.lang.Compiler$HostExpr$Parser.parse (Compiler.java:910)
	clojure.lang.Compiler.analyzeSeq (Compiler.java:6437)
	clojure.lang.Compiler.analyze (Compiler.java:6244)

Note though that the following works fine:

user=> (def x (foo (inc 10)))
#'user/x


 Comments   
Comment by Ben Smith-Mannschott [ 15/Oct/11 10:13 AM ]

CLJ-852-test.patch
reproduces this issue on master

Comment by Ben Smith-Mannschott [ 15/Oct/11 10:41 AM ]

CLJ-852-test.patch initially failed to use (is ...) leading to ugly output on failure.

Comment by Ben Smith-Mannschott [ 15/Oct/11 2:59 PM ]

CLJ-852-rfc-fix.patch

add special cases for "int"…"boolean" to tagToClass()

This causes the test to pass, however, there are still some open questions:

Why does tagToClass first call maybeClass and then possibly overwrite the result it gets back without even looking at it? This strikes me as surprising. I would have expected a check for null, and then only running the if/else chain of special cases if maybeClass failed to return a non-null result.

The suppressed exception in maybeClass commented with "//aargh" makes me wonder if maybeClass and tagToClass are factored correctly.

Comment by Stuart Halloway [ 25/Oct/11 6:00 PM ]

This looks reasonable to me, but given the number of places tagToClass gets called I would definitely want Rich to take a look.

Comment by Ben Smith-Mannschott [ 02/Nov/11 6:12 AM ]

See also
http://groups.google.com/group/clojure/browse_thread/thread/f1672a7e09c34721

Comment by Ben Smith-Mannschott [ 13/Nov/11 9:18 AM ]

CLJ-852-fix.patch removes the RFC comments

This fixes the issue. It appears to me like the cases "int" .. "boolean" (i.e. the non-array primitive types) were simply forgotten in tagToClass().

It's not impossible that tagToClass is being misused somehow ant that's the cause of the problem, but I see no evidence for that. (I'd need a better understanding of the code base to make that determination – mind reading skills would help too.)

It's also conceivable that maybeClass() should be doing the resolution from primitive type to Class object, but that doesn't seem likely to me.

Comment by Andy Fingerhut [ 22/Feb/12 9:02 PM ]

clj-852-patch2.txt is exactly the same as Ben's CLJ-852-fix.patch (including keeping his name and dates on the commits), except:

(1) I moved his new unit tests to an already-existing file metadata.clj, which seemed to have related tests with def and metadata, whereas Ben's patch adds a brand new test file. Not sure whether keeping the number of files down is important or not.

(2) I removed some non-ASCII characters from his commit notes.

Both his patch and this newer one apply cleanly to latest master as of Feb 22, 2012. Neither adds any new errors or warnings to compilation or tests (unless you apply the new test part of the patch without his proposed compiler fix, where the new test does fail as expected). No docstrings need changing that I can think of. Ben and I have both signed CAs. Patch status is "Code and Test".

Comment by Stuart Sierra [ 24/Feb/12 1:32 PM ]

Screened. The addition seems harmless, merely recognizing more tags in tagToClass.





[CLJ-847] mapv, filterv Created: 05/Oct/11  Updated: 02/Dec/11  Resolved: 02/Dec/11

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

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

Attachments: Text File 0847-mapv-filterv.patch    
Patch: Code and Test
Approval: Ok

 Description   

Skip the intermediate sequence step when you know the result you want is a vector, i.e.

(defn mapv
  ([f coll]
     (-> (reduce (fn [v o] (conj! v (f o))) (transient []) coll)
         persistent!)))

(defn filterv
  [pred coll]
  (when-let [s (seq coll)]
    (-> (reduce (fn [v o] (if (pred o) (conj! v o) v))
                (transient [])
                coll)
        persistent!)))


 Comments   
Comment by Rich Hickey [ 05/Oct/11 6:37 AM ]

There's no perf benefit for other arities of map, so for those just define in terms of (into [] (map ...))

Why when-let in filterv? Not returning a vector then. You don't even use s.

Comment by Alan Dipert [ 05/Oct/11 9:39 PM ]

What was the problem with defining mapv and filterv in terms of reducev? This is what I had in mind:

(defn reducev
  [f coll]
  (persistent! (reduce f (transient []) coll)))

(defn mapv
  ([f coll]
     (reducev #(conj! % (f %2)) coll))
  ([f coll & colls]
     (into [] (apply map f coll colls))))

(defn filterv
  [pred coll]
  (reducev #(if (pred %2) (conj! % %2) %) coll))
Comment by Stuart Halloway [ 06/Oct/11 6:18 AM ]

Alan: My original thought was that map and filter are boths fns from collection to collection. reduce is not, so making a reducev that pretends reduce is like map and filter is a little confusing.

But looking at the code I like it.

Rich: ok on both points, thanks.

Comment by Rich Hickey [ 07/Oct/11 7:22 AM ]

The problem is - what are the semantics of reducev? Is this for the public or an implementation detail? mapv and filterv take the same f and pred as map and filter, but reducev doesn't take the same f as reduce, it requires a very special, implementation-dependent f.

Comment by Kevin Downey [ 26/Oct/11 3:28 AM ]

why not implement in terms of reduce (inline reducev)?

Comment by Stuart Halloway [ 15/Nov/11 7:14 PM ]

I see no benefit in separate reducev

Comment by Rich Hickey [ 02/Dec/11 8:12 AM ]

someone other than Stu should screen this

Comment by Stuart Sierra [ 02/Dec/11 9:05 AM ]

Microbenchmark shows performance improvement:

user=> (dotimes [i 5] (time (dotimes [j 10000] (vec (map inc (range 100))))))
"Elapsed time: 119.799 msecs"
"Elapsed time: 80.959 msecs"
"Elapsed time: 75.242 msecs"
"Elapsed time: 75.622 msecs"
"Elapsed time: 73.646 msecs"
nil
user=> (dotimes [i 5] (time (dotimes [j 10000] (mapv inc (range 100)))))
"Elapsed time: 199.096 msecs"
"Elapsed time: 61.737 msecs"
"Elapsed time: 49.911 msecs"
"Elapsed time: 50.421 msecs"
"Elapsed time: 48.437 msecs"
nil
Comment by Stuart Sierra [ 02/Dec/11 9:06 AM ]

Screened by the other Stuart.





[CLJ-846] Javadoc does not detect 1.7 when setting *core-java-api* Created: 03/Oct/11  Updated: 01/Mar/13  Resolved: 25/Oct/11

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

Type: Defect Priority: Major
Reporter: Greg Chapman Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: None
Environment:

Windows 7; Java 1.7


Attachments: Text File CLJ-846.patch    
Patch: Code
Approval: Ok

 Description   

When initializing core-java-api, javadoc.clj checks whether the java specification version is 1.5; if not, it uses a url for the 1.6 docs. Unfortunately, this won't find documentation for new in 1.7 classes (such as those in the java.nio.file package).






[CLJ-845] Unexpected interaction between protocol extension and namespaced method keyword/symbols Created: 29/Sep/11  Updated: 01/Mar/13  Resolved: 16/Dec/11

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

Type: Defect Priority: Minor
Reporter: Alexander Taggart Assignee: Alexander Taggart
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File clj-845-1.patch    
Patch: Code and Test
Approval: Ok

 Description   

If the keywords of a protocol's method map are namespaced, the map is accepted, but lookup fails since lookup uses non-namespaced keywords.

See TLOG-4 for an actual case of this being an issue.

Work-around for namespaced keywords with extend:
don't use namespaced keywords

Work-around for syntax-quoting with extend-type or extend-protocol:
use extend with non-namespaced keywords

Possible solutions:

  1. Inside extend, remove namespace of keywords
  2. or
    • Inside extend, error on namespaced keywords.
    • Inside emit-hinted-impl, only grab name portion of symbols before converting to keyword.


 Comments   
Comment by Alexander Taggart [ 29/Sep/11 8:01 PM ]

Simple test case to see the issue.

=> (defprotocol P
     (self [p]))
P
=> (extend String
     P
     {::self identity})
nil
=> (self "foo")
#<IllegalArgumentException java.lang.IllegalArgumentException: No implementation of method: :self of protocol: #'user/P found for class: java.lang.String>
=> (defmacro try-extend-type [c]
     `(extend-type String
        P
        (self [p#] p#)))
#'user/try-extend-type
=> (try-extend-type String)
nil
=> (self "foo")
#<IllegalArgumentException java.lang.IllegalArgumentException: No implementation of method: :self of protocol: #'user/P found for class: java.lang.String>
=> (keys (get-in P [:impls String]))
(:user/self)
Comment by Rich Hickey [ 07/Oct/11 7:28 AM ]

Most of this is a non-issue. :self and :foo/self are not the same.

This:

"Inside emit-hinted-impl, only grab name portion of symbols before converting to keyword."

is the only needed change.

Comment by Stuart Sierra [ 09/Dec/11 3:26 PM ]

Screened.





[CLJ-843] clojure.lang.RT should provide a loadLibrary static method Created: 26/Sep/11  Updated: 02/Dec/11  Resolved: 02/Dec/11

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

Type: Enhancement Priority: Minor
Reporter: Baishampayan Ghose Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-Add-a-static-method-loadLibrary-to-clojure.lang.RT-t.patch    
Patch: Code
Approval: Ok

 Description   

Right now, loading native libraries in Clojure doesn't work as expected because those libraries are loaded into the classloader of the invoking class (that is, Clojure's own classloader).

This problem has been discussed on the mailing list and a patch has been welcomed by Rich -

https://groups.google.com/group/clojure/browse_thread/thread/aa72e43091ec3228?pli=1
https://groups.google.com/forum/#!topic/clojure-dev/awe7-yeieIM
https://groups.google.com/forum/?hl=en#!topic/clojure-dev/OFQhDKHTyrw

To fix this, I am attaching a trivial patch that implements loadLibrary in RT.



 Comments   
Comment by Chouser [ 20/Nov/11 4:02 PM ]

Tests would be nice, though I can see how getting such a test to work without 3rd party deps might be difficult.

My naive attempts to reproduce the problem failed. For example, this works fine for me:
(System/loadLibrary "hprof")

Nevertheless, the patch applies cleanly and appears to be what Rich asked for, so I'm setting this to "screened".





[CLJ-839] contains? does not work with java.util.Set Created: 14/Sep/11  Updated: 09/Dec/11  Resolved: 09/Dec/11

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

Type: Enhancement Priority: Minor
Reporter: Meikel Brandmeyer Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-Make-contains-work-with-java.util.Set.patch    
Patch: Code and Test
Approval: Ok

 Description   

contains? does support j.u.Map, but not j.u.Set. However it works with Clojure sets.



 Comments   
Comment by Kevin Downey [ 30/Nov/11 5:25 AM ]

looks good

patch applies cleanly, tests pass

contains? working with java.util.Sets seems like a good idea





[CLJ-838] dev.clojure.org/display/doc/1. 3 renders badly Created: 14/Sep/11  Updated: 01/Mar/13  Resolved: 07/Oct/11

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

Type: Enhancement Priority: Minor
Reporter: Ben Smith-Mannschott Assignee: Ben Smith-Mannschott
Resolution: Completed Votes: 0
Labels: None

Attachments: Text File 0001-changes.txt-recoded-as-markdown.patch     Text File 0002-rename-changes.txt-changes.md.patch     Text File 0003-add-example-for-Add-docstring-support-to-def.patch    
Patch: Code
Approval: Ok

 Description   

The page http://dev.clojure.org/display/doc/1.3 has markup issues:

  • *earmuffs* become bold
  • clearly [ ] must mean something special, 'cause they all turn pink.
  • presumably there's some way to make confluence use monospace for code snippets.
  • I'm sure confluence has some way to mark headings, but It doesn't look like "==" is it.
  • I'd like to evict the "frowning face" from CLJ-751's clj-format example.
  • 2.29 seems to have suffered some encoding debacle, though I'm not quite sure what happened.

Further discussion revealed that this was just a copy/paste of changes.txt in the source repository. Discussions on clojure-dev suggested the idea of recoding changes.txt to changes.md and recoding it as proper markdown so that it will be rendered nicely by github.

This would make http://dev.clojure.org/display/doc/1.3 obsolete, its contents could be replaced by a link to 'changes.md' on github. That makes more sense than maintaining two copies of the document in mutually incompatible markups (markdown and whatever confluence uses).

See also the following clojure-dev thread:

http://groups.google.com/group/clojure-dev/browse_thread/thread/ecf65c4e50d3fd76



 Comments   
Comment by Ben Smith-Mannschott [ 14/Sep/11 2:23 PM ]

0001-CLJ-838-changes.txt-recoded-as-markdown.patch

This recodes changes.txt as proper Markdown. Text is not hard-wrapped because github's Markdown dialect handles hard line breaks differently than standard Markdown. Also, the original wasn't hard-wrapped either.

0002-CLJ-838-rename-changes.txt-changes.md.patch

This patch just renames changes.txt to changes.md. I didn't want to rename and edit changes.txt in a single patch, not knowing how well git would handle that when it came time to apply the changes.

0003-CLJ-838-add-example-for-Add-docstring-support-to-def.patch

Meikel Brandmeyer provided an example of docstring support in def in response to my query, so I've included a patch to integrate that.

Comment by Ben Smith-Mannschott [ 14/Sep/11 2:28 PM ]

I don't believe Markdown provides a good way of handling the contents section at the start of the document. I've improvised it by just formatting the thing as if it were a code sample.

Is the TOC really necessary? It's not ideal from a Don't Repeat Yourself standpoint. Also, manually maintaining the section numbers also strikes me as busy work? Can we do without?

Comment by Ben Smith-Mannschott [ 14/Sep/11 2:28 PM ]

You can view a rendered version of changes.md here:

https://github.com/bpsm/clojure/blob/CLJ-838/changes.md

Comment by Ben Smith-Mannschott [ 23/Sep/11 3:18 PM ]

patches updated for f0b092b66 "more changes.txt tweaks"





[CLJ-837] java.lang.VerifyError when compiling deftype or defrecord with argument name starting with double underscore characters. Created: 11/Sep/11  Updated: 09/Dec/11  Resolved: 09/Dec/11

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

Type: Defect Priority: Minor
Reporter: Tchavdar Roussanov Assignee: Fogus
Resolution: Completed Votes: 1
Labels: None
Environment:

clojure-1.3.0-beta3 and git master, JDK 1.6.0_26, OS X 1.7.1


Attachments: Text File 0837-fixed.patch     File CLJ-837-double-underscore-type-records.diff    
Patch: Code and Test
Approval: Ok
Waiting On: Rich Hickey

 Description   

The minimal test to trigger the verification error is :

(deftype t [__])
or
(defrecord r [__])

It works fine in clojure 1.2, 1.2.1

Works fine with one underscore.

Output from REPL:

user=> (defrecord r [__])
CompilerException java.lang.VerifyError: (class: user/r, method: create signature: (Lclojure/lang/IPersistentMap;)Luser/r Expecting to find object/array on stack, compiling:(NO_SOURCE_PATH:2)

Stack trace from slime session:

(class: clojure/data/priority_map/r, method: create signature: (Lclojure/lang/IPersistentMap;)Lclojure/data/priority_map/r Expecting to find object/array on stack
[Thrown class java.lang.VerifyError]

Restarts:
0: [QUIT] Quit to the SLIME top level

Backtrace:
0: java.lang.Class.forName0(Native Method)
1: java.lang.Class.forName(Class.java:247)
2: clojure.lang.RT.classForName(RT.java:2013)
3: clojure.lang.Compiler$HostExpr.maybeClass(Compiler.java:929)
4: clojure.lang.Compiler$HostExpr.access$400(Compiler.java:710)
5: clojure.lang.Compiler$NewExpr$Parser.parse(Compiler.java:2391)
6: clojure.lang.Compiler.analyzeSeq(Compiler.java:6368)
7: clojure.lang.Compiler.analyze(Compiler.java:6175)
8: clojure.lang.Compiler.analyze(Compiler.java:6136)
9: clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5531)
10: clojure.lang.Compiler$FnMethod.parse(Compiler.java:4967)
[No Locals]
11: clojure.lang.Compiler$FnExpr.parse(Compiler.java:3588)
12: clojure.lang.Compiler.analyzeSeq(Compiler.java:6366)
13: clojure.lang.Compiler.analyze(Compiler.java:6175)
14: clojure.lang.Compiler.analyzeSeq(Compiler.java:6356)
15: clojure.lang.Compiler.analyze(Compiler.java:6175)
16: clojure.lang.Compiler.access$100(Compiler.java:37)
17: clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:492)
18: clojure.lang.Compiler.analyzeSeq(Compiler.java:6368)
19: clojure.lang.Compiler.analyze(Compiler.java:6175)
20: clojure.lang.Compiler.analyzeSeq(Compiler.java:6356)
21: clojure.lang.Compiler.analyze(Compiler.java:6175)
22: clojure.lang.Compiler.analyze(Compiler.java:6136)
23: clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5531)
24: clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5832)
25: clojure.lang.Compiler.analyzeSeq(Compiler.java:6368)
26: clojure.lang.Compiler.analyze(Compiler.java:6175)
27: clojure.lang.Compiler.analyze(Compiler.java:6136)
28: clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5531)
29: clojure.lang.Compiler$FnMethod.parse(Compiler.java:4967)
30: clojure.lang.Compiler$FnExpr.parse(Compiler.java:3588)
31: clojure.lang.Compiler.analyzeSeq(Compiler.java:6366)
32: clojure.lang.Compiler.analyze(Compiler.java:6175)
33: clojure.lang.Compiler.eval(Compiler.java:6421)
34: clojure.lang.Compiler.eval(Compiler.java:6390)
35: clojure.core$eval.invoke(core.clj:2795)
36: swank.commands.basic$eval_region.invoke(basic.clj:47)
37: swank.commands.basic$eval_region.invoke(basic.clj:37)
38: swank.commands.basic$eval764$listener_eval__765.invoke(basic.clj:71)
39: clojure.lang.Var.invoke(Var.java:401)
40: clojure.data.priority_map$eval2759.invoke(NO_SOURCE_FILE)
41: clojure.lang.Compiler.eval(Compiler.java:6424)
42: clojure.lang.Compiler.eval(Compiler.java:6390)
43: clojure.core$eval.invoke(core.clj:2795)
44: swank.core$eval_in_emacs_package.invoke(core.clj:92)
45: swank.core$eval_for_emacs.invoke(core.clj:239)
46: clojure.lang.Var.invoke(Var.java:409)
47: clojure.lang.AFn.applyToHelper(AFn.java:167)
48: clojure.lang.Var.applyTo(Var.java:518)
49: clojure.core$apply.invoke(core.clj:600)
50: swank.core$eval_from_control.invoke(core.clj:99)
[No Locals]
51: swank.core$eval_loop.invoke(core.clj:104)
52: swank.core$spawn_repl_thread$fn_529$fn_530.invoke(core.clj:309)
53: clojure.lang.AFn.applyToHelper(AFn.java:159)
54: clojure.lang.AFn.applyTo(AFn.java:151)
55: clojure.core$apply.invoke(core.clj:600)
56: swank.core$spawn_repl_thread$fn__529.doInvoke(core.clj:306)
57: clojure.lang.RestFn.invoke(RestFn.java:397)
58: clojure.lang.AFn.run(AFn.java:24)
59: java.lang.Thread.run(Thread.java:680)



 Comments   
Comment by Fogus [ 11/Sep/11 9:18 PM ]

The privileged names __meta and __extmap are implementation details of types and records and should probably not be viewed as real. In other words, since they are not published as part of the type/record docs, then they may not exist in the future. Having said that, the current Clojure implementation treats any name starting with __ as a special field and doesn't count them toward the field count. As a result, when emitting the MyType.create method (another implementation detail), the fields marked as __ throw off the field count and as a result the bytecode is not emitted properly. I know how to change the code to allow names prefixed with __ and fixing the validation error (not called by those two special names listed above), but I hesitate to say that is the solution
without further reflection.

Comment by Fogus [ 12/Sep/11 8:32 AM ]

I think the prudent fix is as follows:

  • Allow any legal name except those in the set #{__meta __extmap}
  • Fix the #create emission
  • Documentation should be changed to reflect that the aforementioned set is disallowed

Thankfully this is an easy fix. However, the "magic" of __ prefixed names will go away. That is, the following behavior will differ from v1.2:

(defrecord R [__a ___b])
(:__a (R. 42 9))
;=> 42

(deftype T [__a])
(.__a (T. 36))
;=> 36

(deftype T2 [a b __meta])
;; AssertionError

Any thoughts?

Comment by Fogus [ 12/Sep/11 9:44 AM ]

If anyone is interested, the changes reflected in the previous comment can be found on my own branch: https://github.com/fogus/clojure/tree/CLJ-837 Code-review would be nice.

Comment by Rich Hickey [ 12/Sep/11 10:09 AM ]

differing behavior of undocumented features is a non-problem

Comment by Fogus [ 12/Sep/11 12:05 PM ]

Got it. Thanks Rich.

Comment by Fogus [ 12/Sep/11 12:25 PM ]

Added patch file.

Comment by Steve Miner [ 12/Sep/11 2:11 PM ]

I suggest that the documentation should simply say that all fields beginning with double-underscore (__) are reserved. You don't have to enforce that (as the patch allows any field name except __meta and __extmap), but it's better to be conservative with the documentation so that you have some room to innovate.

Comment by Steve Miner [ 12/Sep/11 2:13 PM ]

Just by inspection, it looks like patch has an extra space before the 'b' in the line near the end:

+ 2 (:___ b r)

Comment by Stuart Halloway [ 02/Dec/11 1:53 PM ]

0837-fixed is same as original patch, but with fixed unit test.





[CLJ-832] reify implements IObj but nowhere states so Created: 25/Aug/11  Updated: 09/Dec/11  Resolved: 09/Dec/11

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

Type: Enhancement Priority: Trivial
Reporter: Meikel Brandmeyer Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None
Environment:

1.3.0-beta1


Attachments: Text File 0001-Fix-docstring-of-reify.patch    
Patch: Code
Approval: Ok

 Description   

reify does implement clojure.lang.IObj but nowhere states so. The attached patch fixes the docstring and adds an example how metadata is transferred from the reify form to the object.






[CLJ-830] Report source file and line number when throwing syntax-related error from core macros Created: 24/Aug/11  Updated: 01/Mar/13  Resolved: 07/Oct/11

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

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

Attachments: File CLJ-830.diff    
Patch: Code and Test
Approval: Ok

 Description   

If you botch or misunderstand certain macros in core, they throw exceptions with very unhelpful error messages:

=> (let [a])
#<IllegalArgumentException java.lang.IllegalArgumentException:
  let requires an even number of forms in binding vector>

=> (if-let [a 5
            b 6]
     true)
#<IllegalArgumentException java.lang.IllegalArgumentException:
  if-let requires exactly 2 forms in binding vector>

No mention of userland file/namespace or line number is in the stack trace. If this code happens to be in code being loaded via some chain of :requires, tracking it down can be way more painful than it reasonably should be.

Something like this would be much better:

#<IllegalArgumentException java.lang.IllegalArgumentException:
  let requires an even number of forms in binding vector in com.foo.namespace:80>

These errors are all thrown via the same assert-args function used by a variety of macros in clojure.core; modifying it so it emits a reasonable error message should be straightforward.



 Comments   
Comment by Stuart Sierra [ 27/Aug/11 3:25 PM ]

Only affects the assert-args macro, which is private in clojure.core. Patch works as advertised.





[CLJ-829] Transient hashmaps mishandle hash collisions Created: 24/Aug/11  Updated: 01/Mar/13  Resolved: 07/Oct/11

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

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

Attachments: File clj-829-2.diff     File clj-829.diff    
Patch: Code and Test
Approval: Ok

 Description   

Clojure 1.2.1 and 1.3.0-beta1 both exhibit the following behavior:

user> (let [m (into {} (for [x (range 100000)] [(rand) (rand)]))]
(println (count (distinct (map hash (keys m)))))
((juxt count identity) (persistent!
(reduce dissoc! (transient m) (keys m)))))
99999
[2 {0.42548900739367024 0.8725039567983159}]

We create a large transient map with random keys and values, and check to see how many unique hashcodes we get. Then, we iterate over all the keys, dissoc'ing each out of the transient map. The resulting map has one element in it (wrong - it should be empty, since we dissoc'ed all the keys), and reports its count as being two (wrong - not sure whether it should be zero or one given the other breakage). As far as I can tell, each duplicated hash value is represented once in the output map, and the map's count is the number of keys that hashed to something duplicated.

The problem seems to be restricted to transients, as if we remove the transient/persistent! pair and use dissoc instead of dissoc!, the map is always empty.

Inspired by discussion at http://groups.google.com/group/clojure/browse_thread/thread/313ac122667bb4b5/c3e7faa8635403f1



 Comments   
Comment by Alan Malloy [ 25/Aug/11 12:26 PM ]

By the way, since this involves randomness, on occasion it doesn't fail. With the input as given it seems to fail around 80% of the time, but if you want to be sure to reproduce you can add another 0 to the input size.

Comment by Aaron Bedra [ 26/Aug/11 9:20 AM ]

Thanks for the update. I was able to reproduce with the extra zero. Moving this ticket to Release.Next so that it will ship with 1.3.

Comment by Christophe Grand [ 08/Sep/11 1:15 PM ]

Sorry for the delay. Here is a fix and a reduced test case.

Comment by Stuart Halloway [ 09/Sep/11 3:50 PM ]

I have checked behavior with an independent test

https://github.com/clojure/test.generative/commit/b1350235eb219b76f5b7c4cc21c8255f567892b3

which looks good, but don't have context to evaluate the code (particularly the array allocation) in the time I have available today.

Comment by Rich Hickey [ 09/Sep/11 4:14 PM ]

The array alloc looks suspicious:

+ Object[] newArray = new Object[2*(array.length+1)]; // make room for next assoc
+ System.arraycopy(array, 0, newArray, 0, 2*count);

should it not be array.length + 2, (or 2*(count + 1), whichever?

Comment by Christophe Grand [ 10/Sep/11 3:48 AM ]

Thanks Rich for spotting this copy&paste error.

I attached an updated patch.

The problem reported by Alan was double:

  • the misplaced removedLeaf.val = removedLeaf was causing the global count to be incorrect
  • the missing array copy in ensureEditable was causing the seq returned by (keys m) to be mutated (shortened) and this is why all values were not dissoced.

Mutable code is hard, one should invent a cool language with sane state management

Comment by Stuart Halloway [ 10/Sep/11 2:54 PM ]

New tests continue to pass.

It seems to me that the allocation in the second path is too big by two in some cases (e.g. it gets called in the dissoc! path when the array needs to be copied, but not get bigger). But this might be considered innocuous. The same method is called in the assoc! path where the +2 is needed, so avoiding two objects worth of allocation would require a more substantial patch.

Comment by Christophe Grand [ 12/Sep/11 3:39 AM ]

The larger array allocation is similar to the one performed in BitmapIndexedNode.ensureEditable. My line of thought behind this heuristic is that the copying dominates the allocation and that I prefer one slightly larger array than having to allocates two arrays in case of growth.

Anyway it's on collision nodes so it's a rare occurence: I won't bother arguing further if switching to a more conservative allocation helps the patch landing in 1.3.





[CLJ-817] print-deftype breaks when fields have dashes Created: 27/Jun/11  Updated: 01/Mar/13  Resolved: 22/Mar/12

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

Type: Defect Priority: Minor
Reporter: Alan Dipert Assignee: Alan Dipert
Resolution: Completed Votes: 0
Labels: None

Attachments: File 817-print-deftype-fields-dashes.diff     Text File test.log    

 Description   
user=> (deftype Foo [bar-quux])
user.Foo
user=> (def x (Foo. 1))
#'user/x 
user=> x
IllegalArgumentException No matching field found: bar-quux for class user.Foo  clojure.lang.Reflector.getInstanceField (Reflector.java:289)
#user.Foo[user=>

Printing records works fine. See this mailing list message for original report.



 Comments   
Comment by Aaron Bedra [ 28/Jun/11 12:59 PM ]

This patch breaks tests.

Comment by Alan Dipert [ 28/Jun/11 2:18 PM ]

Could you please attach your test output? I'm not able to reproduce.

Comment by Aaron Bedra [ 29/Jun/11 7:37 AM ]

I attached the full test run. This is

java version "1.6.0_24"
Java(TM) SE Runtime Environment (build 1.6.0_24-b07)
Java HotSpot(TM) 64-Bit Server VM (build 19.1-b02, mixed mode)

on Ubuntu 11.04 64

Comment by Alan Dipert [ 06/Jul/11 10:39 PM ]

I haven't been able to reproduce your error. Could you try ant clean and run again? Perhaps another patch was also applied when you tested? Thanks in advance for giving it another shot.

Platforms I've tried that compile and test cleanly:

  • Mac OS X 10.6.8, 1.6.0_24-b07
  • Ubuntu 11.04 32, Sun JDK 1.6.0_24-b07
  • Ubuntu 11.04 64, Sun JDK 1.6.0_24-b07 (EC2, ami-1aad5273)
  • Ubuntu 11.04 64, OpenJDK 1.6.0_22 (EC2, ami-1aad5273)
  • CentOS 5.6 64, Sun JDK 1.6.0_27-ea
Comment by Kevin Downey [ 29/Nov/11 7:42 PM ]

this patch no longer applies cleanly

Comment by Andy Fingerhut [ 24/Feb/12 4:50 PM ]

The bad behavior appears not to exist in latest master as of Feb 24, 2012, nor does it exist in release 1.3.0. The code in the neighborhood of this one-line patch is significantly different than it was when the patch was made, so perhaps this issue was corrected as a side effect of some commit before the 1.3.0 release.

Mac OS X 10.6.8, 1.6.0_29, Clojure 1.3.0 test transcript:

Clojure 1.3.0
user=> (deftype Foo [bar-quux])
user.Foo
user=> (def x (Foo. 1))
#'user/x
user=> x
#<Foo user.Foo@6d080876>
user=> (deftype Bar [bar-id])
user.Bar
user=> (Bar. 1)
#<Bar user.Bar@679801c>
user=> (deftype Baz [bar_id])
user.Baz
user=> (Baz. 1)
#<Baz user.Baz@54128635>

Comment by Alan Dipert [ 22/Mar/12 8:21 PM ]

This issue does not exist in Clojure 1.3 onwards, and the attached patch is no longer relevant.





[CLJ-791] Include argument type info in reflection warnings and method signatures in dispatch compilation errors Created: 10/May/11  Updated: 01/Mar/13  Resolved: 07/Oct/11

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

Type: Enhancement Priority: Major
Reporter: Chas Emerick Assignee: Chas Emerick
Resolution: Completed Votes: 0
Labels: None

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

 Description   

Errors like:

More than one matching method found: foo

are bad, especially when there are multiple overloads and the types of one's arguments aren't immediately obvious. In addition, reflection warnings like:

Reflection warning, %s:%d - call to %s can't be resolved

are similarly unhelpful.

The above should be modified to include as much type information as possible, i.e.:

More than one matching method found: foo(long[], int, int) and foo(long[], long, int) for arguments of type (long[], long, long)

Reflection warning, %s:%d - call to %s can't be resolved with arguments of type (clojure.lang.PersistentVector, String, int)

Such ideas were briefly discussed here.



 Comments   
Comment by Chas Emerick [ 10/May/11 1:30 PM ]

Note that the attached patch (incidentally) depends on the patch for CLJ-789.





[CLJ-786] The docstring for defn omits any mention of pre- and post-conditions Created: 02/May/11  Updated: 09/Dec/11  Resolved: 09/Dec/11

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

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

Attachments: File defndoc.diff    
Patch: Code
Approval: Ok

 Description   

Currently, the doc string for defn does not mention pre- or post-conditions in either the arg list or description. I always forget where it goes.

Current doc string:

clojure.core/defn
([name doc-string? attr-map? [params*] body] [name doc-string? attr-map? ([params*] body) + attr-map?])
Macro
  Same as (def name (fn [params* ] exprs*)) or (def
    name (fn ([params* ] exprs*)+)) with any doc-string or attrs added
    to the var metadata

Attached is a patch to update defn to:

clojure.core/defn
([name doc-string? attr-map? [params*] prepost-map? body] [name doc-string? attr-map? ([params*] prepost-map? body) + attr-map?])
Macro
  Same as (def name (fn [params* ] exprs*)) or (def
    name (fn ([params* ] exprs*)+)) with any doc-string or attrs added
    to the var metadata. prepost-map defines a map with optional keys
    :pre and :post that contain collections of pre or post conditions.





[CLJ-773] macros that are expanded away still have their vars referenced in the emitted byte code Created: 07/Apr/11  Updated: 09/Dec/11  Resolved: 09/Dec/11

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

Type: Enhancement Priority: Trivial
Reporter: Kevin Downey Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File clj-773.diff    
Patch: Code
Approval: Ok

 Description   

if you have a macro like:

(defmacro f [a b] a)

and compile a function like:

(fn [x] (f x (/ 1 0)))

the macro is expand before compilation into:

(fn [x] x) ;; no reference to f

but the emitted byte code still holds the var for the macro 'f' in a static field.



 Comments   
Comment by Kevin Downey [ 07/Apr/11 8:21 PM ]

patch to not register vars during macro expansion

Comment by Kevin Downey [ 29/Nov/11 7:50 PM ]

patch still applies cleanly and all tests pass

Comment by Stuart Halloway [ 02/Dec/11 1:38 PM ]

tests pass and approach seems sound





[CLJ-736] Docstring of defrecord (and =) does not correctly describe equality behavior for records. Created: 09/Feb/11  Updated: 01/Mar/13  Resolved: 02/Sep/11

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

Type: Enhancement Priority: Minor
Reporter: Jason Wolfe Assignee: Stuart Halloway
Resolution: Completed Votes: 0
Labels: None

Approval: Accepted
Waiting On: Stuart Halloway

 Description   

http://groups.google.com/group/clojure/browse_frm/thread/eba691b38c45196b#

The docstring of defrecord says it "will define type-and-
value-based equality and hashCode". In reality, record types are included in = but not .equals (or hashCode), and so records of different types can collide as map keys.

Along the same lines, the docstring of = says "same as Java x.equals, except it ... compares numbers and collections in a type-independent manner". To me, this seems to imply the opposite of what actually happens with records.

FWIW I think it would be more clear if the behavior for = and .equals were the same in this respect, but in any case the implemented behavior should be properly documented.



 Comments   
Comment by Stuart Sierra [ 06/Jun/11 9:37 AM ]

Also discussed at https://groups.google.com/d/topic/clojure/e6UhXVny8Xc/discussion

Per Rich, "The policy is: = includes the type and .equals doesn't. In this way records can still be proper j.u.Maps and = remains most useful for records (e.g. including type)."

Comment by Jason Wolfe [ 06/Jun/11 11:25 AM ]

On a related note:

http://dev.clojure.org/display/doc/Enhanced+Primitive+Support?focusedCommentId=1573146#comment-1573146

Regarding "hash maps and sets now use = for keys ... will use stricter .equals when calling through java.util interfaces":

Note that "=" is actually stricter than ".equals" for record types currently, because it includes type information.

This has important consequences for how (and whether) hash maps and sets actually obey the java.util interfaces.

For instance, if

(defrecord P []), ...

what should (.get {(P.) 1 (Q.) 2} (Q.)) return? How about if we .get an element of a third type (R.)?

In a similar vein, this behavior seems confusing at best:

user=> (java.util.HashMap. {(P.) 1 (Q.) 2})
#<HashMap {user.P@0=2}>

Comment by Aaron Bedra [ 28/Jun/11 6:36 PM ]

Open question:

  • the phrase "type-independent" is confusing. It is intended to talk about concrete numeric and collection types, but I can see why it gets confusing with records. Need an idea for two different terms here (and define them someplace). If somebody has one, please add it to this ticket.

Non-issues:

  • = and .equals are not and will not be the same. The whole point is to avoid Java's broken semantics (=) while providing an interop formula for those who need it (.equals)
  • Any types can collide as hash keys.
Comment by Aaron Bedra [ 19/Aug/11 11:34 AM ]

Any thoughts on the open questions part Rich?

Comment by Stuart Halloway [ 02/Sep/11 9:37 AM ]

I think the following is better:

monospaced
In addition, defrecord will define type-and-value-based =,
and will defined Java .hashCode and .equals consistent with the
contract for java.util.Map.
monospaced

But I don't want to hold release for discussion, so bumping this to approved backlog for further bikeshedding.

Comment by Stuart Halloway [ 02/Sep/11 9:39 AM ]

Better yet, let's just agreed on my prose and call this done.





[CLJ-718] Document 1.3 numeric support Created: 16/Jan/11  Updated: 02/Dec/11  Resolved: 02/Dec/11

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

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

Attachments: File conjfeatures.key    

 Description   

Should include

  • usage
  • rationale
  • what else
  • guidance for people extending math support in Clojure (e.g. Mark Engelberg's expt)

I want something I can link to whenever old ground is being retrod, and add to whenever new material comes up.

Rich: do you want do this, or edit and extend a first cut from me?



 Comments   
Comment by Rich Hickey [ 26/Jan/11 7:21 AM ]

Slides from Clojure Features talk given at Conj 2010

Comment by Rich Hickey [ 26/Jan/11 7:23 AM ]

I've uploaded my features talk slides. It has a nice outline for you to start with. If you would, start a confluence page from this. The entire job of documenting this will have to include auditing the main clojure.org docs as well.

Comment by Luke VanderHart [ 19/Aug/11 11:39 AM ]

I have put a draft of this (with a couple rounds of feedback) at http://dev.clojure.org/display/doc/Documentation+for+1.3+Numerics

Comment by Stuart Halloway [ 02/Dec/11 2:07 PM ]

first pass of this is done. Not to say it couldn't be done more thoroughly...





[CLJ-683] broken example in ns docstring Created: 27/Nov/10  Updated: 30/Mar/12  Resolved: 30/Mar/12

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

Type: Defect Priority: Trivial
Reporter: Colin Jones Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: None

Attachments: Text File 0001-Fix-ns-docstring-to-avoid-dotted-lib-names.patch     Text File ns-docstring-exception-fix.patch    
Patch: Code
Approval: Ok

 Description   

The docstring for `ns` currently has a line

(:require (clojure.contrib sql sql.tests))

which fails with

Java.lang.Exception: lib names inside prefix lists must not contain periods

because of the `sql.tests` value. While the `:use` line will likely also fail,
that one - `(:use (my.lib this that))` - is clearly just an example, whereas the
`:require` line actually shows an incorrect syntax that will never work.

The latest patch swaps `sql.tests` in the docstring for another contrib library, `combinatorics`.



 Comments   
Comment by Colin Jones [ 30/Nov/10 2:56 PM ]

This patch is the correct format; using `git format-patch`, instead of dumping the output of `git diff` to a file. Sorry about that.

Comment by Stuart Sierra [ 23/Mar/12 8:35 AM ]

Trivial docstring fix. Screened.





[CLJ-452] Miscellaneous improvements to Clojure runtime usability from Java Created: 06/Oct/10  Updated: 07/Oct/11  Resolved: 07/Oct/11

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

Type: Enhancement Priority: Major
Reporter: Chas Emerick Assignee: Chas Emerick
Resolution: Declined Votes: 0
Labels: None


 Description   

Using Clojure from Java is very pleasant overall, but there are some rough edges, many of which could be smoothed over with very simple enhancements. I would suggest:

  • there should be an RT.var or Var.find that takes a single string
  • an IFn wrapper should be available that catches and rethrows Exceptions as RuntimeExceptions
  • a simple class (clojure.lang.Clojure perhaps?) that provides a well-documented and stable API for common operations: requiring namespaces, loading files, finding vars, evaluating strings, etc. This also would have a side benefit of allowing breaking changes to the implementation details without breaking interop code.

There are others of this sort that I've come across; will update the above as I remember them.



 Comments   
Comment by Assembla Importer [ 26/Oct/10 10:30 PM ]

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

Comment by Assembla Importer [ 26/Oct/10 10:30 PM ]

cemerick said: Here's an implementation of that IFn wrapper that rethrows Exceptions as RuntimeExceptions: SafeFn.java

This makes sense to me, but perhaps not everyone. Thoughts?

Comment by Chas Emerick [ 21/Mar/11 9:03 AM ]

See http://dev.clojure.org/display/design/Improvements+to+interop+from+Java for a discussion of these changes.

Comment by Stuart Halloway [ 07/Oct/11 12:22 PM ]

While the title of this ticket is broad, it looks like the actual discussion is about rethrowing RuntimeException. Clojure 1.3 and later solve this problem a different way, by not declaring Exception on the relevant interface points.

Please add tickets for the other items discussed on the wiki as appropriate.





[CLJ-389] slurp should not read one character at a time Created: 25/Jun/10  Updated: 26/Jul/13  Resolved: 02/Dec/11

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

Type: Enhancement Priority: Minor
Reporter: Aaron Bedra Assignee: Aaron Bedra
Resolution: Declined Votes: 0
Labels: None

Attachments: Text File 0001-Improve-slurp-performance-by-using-native-Java-Strin.patch     Text File 0001-needles-StrinbBuilder-in-slurp.patch     File 0389_buffered_slurp.diff    
Patch: Code
Approval: Incomplete

 Description   

A discussion of slurp performance came up on the mailing list. The attached patch makes slurp use a 4kb buffer size instead of reading one character at a time, resulting in a significant performance improvement.



 Comments   
Comment by Assembla Importer [ 25/Oct/10 9:30 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/389
Attachments:
slurp-buffer-size.patch - https://www.assembla.com/spaces/clojure/documents/c7INhmGfSr34YDeJe5cbCb/download/c7INhmGfSr34YDeJe5cbCb

Comment by Assembla Importer [ 25/Oct/10 9:30 PM ]

stu said: Updating tickets (#389, #371)

Comment by Assembla Importer [ 25/Oct/10 9:30 PM ]

aaron said: Patch does not apply. It is not in the proper format. Please take a look at http://clojure.org/patches and resubmit. I will continue to review the code itself against the current master.

Comment by Aaron Bedra [ 29/Oct/10 9:43 AM ]

Contacted original poster about this to inform him of the move to JIRA and ensure interest in this patch

Comment by Aaron Bedra [ 12/Nov/10 1:55 PM ]

The patch has been reformatted and submitted.

Comment by Jürgen Hötzel [ 13/Nov/10 2:07 AM ]

Maybe duplicate: CLJ-668

Comment by Aaron Bedra [ 26/Nov/10 10:17 AM ]

CLJ-668 is a duplicate of this ticket. I have tested your patch and would like to consider it as a resolution to this ticket along with closing CLJ-668 as a duplicate. Can you please submit a patch to this ticket instead of a link to github? I am going to test all of the patches presented on OSX, Linux, EC2, and Windows.

Comment by Jürgen Hötzel [ 27/Nov/10 4:57 AM ]

Seems like the changes are already applied by your previous commit:

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

Just a leftover StringBuilder Object (patch attached)

Comment by Aaron Bedra [ 28/Nov/10 10:06 AM ]

Yes, that was a mistake. It should not have gone in under #441. I was doing some testing and somehow I didn't properly revert what I was working on. Please consider that a mistake for now and include a patch here so you can get credit for your work.

Comment by Jürgen Hötzel [ 29/Nov/10 2:50 AM ]

Thanks. Patch enclosed.

Comment by Aaron Bedra [ 03/Dec/10 8:19 AM ]

Thanks. I am going to be testing the various ideas on multiple platforms to see which (if any) is the best fit.

Comment by Aaron Bedra [ 30/Nov/11 7:59 PM ]

This patch carries a performance hit. For example:

Clojure 1.4.0

File Size best timed 10x slurp
7.3 MB 2960.363 msecs
260 KB 105.255 msecs

With Patch

File Size best timed 10x slurp
7.3 MB 3700.406 msecs
260 KB 129.311 msecs

This was tested with

(time (dotimes [_ 10] (slurp "file")))

with each execution of this form done 20 times.





[CLJ-369] Check for invalid interface method names Created: 01/Jun/10  Updated: 23/Mar/12  Resolved: 23/Mar/12

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

Type: Enhancement Priority: Minor
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Approval: Ok

 Description   

Dashes are not allowed in method names. However gen-interface does not complain about invalid method names.

See also: http://groups.google.com/group/clojure/browse_thread/thread/c740b3835c437d0



 Comments   
Comment by Assembla Importer [ 28/Sep/10 8:01 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/369
Attachments:
0001-Check-interface-methods-for.patch - https://www.assembla.com/spaces/clojure/documents/b8JJfgBByr374seJe5cbCb/download/b8JJfgBByr374seJe5cbCb

Comment by Assembla Importer [ 28/Sep/10 8:01 AM ]

richhickey said: We might have some better method name validity checking somewhere (i.e. not just '-', although that is probably most common)

Comment by Stuart Sierra [ 17/Feb/12 2:44 PM ]

Screened.

Without this patch, gen-interface can create methods with invalid names.





Generated at Thu Oct 30 08:15:35 CDT 2014 using JIRA 4.4#649-r158309.