<< Back to previous view

[CLJ-1576] clojure.pprint should print vars as pr does Created: 29/Oct/14  Updated: 29/Oct/14

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

Type: Enhancement Priority: Major
Reporter: Kevin Downey Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: None


 Description   

clojure.pprint/pprint currently by default lets vars fall through to its IDeref printing which prints them as something like:

#<Var@107e78: #<core$inc clojure.core$inc@f278dd>>

which is not a super representation of a var. vars have names.

generally when I pprint a data structure containing vars it is because at some point in writing the code that constructed that data structure I decided I wanted a history of the functions called, and since vars are invokable as functions and have a name, I can just use those as the history. the history then turns in to a big structure so I pretty print it, which then doesn't print the vars.

it is possible to work/around change the behaviour of the pretty printer by using its customizing options, but it is not a simple change to make, and means that for a small program a large percentage of it is spent making the pretty printer print something useful for vars.






[CLJ-1575] Using a (def ^:const instance) of a deftype that implements IPersistentCollection, triggers compiler errors Created: 29/Oct/14  Updated: 30/Oct/14

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

Type: Defect Priority: Minor
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

fresh repl


Attachments: Text File 0001-Test-for-analyzer-bug-CLJ-1575.patch    

 Description   

The compiler has a lot of assumptions about the possible types of IPersistentCollection literals and rightfully so. The strange thing with this case is, that taking the (constant) value works as soon as count is defined, but using it as an argument hits a closed dispatch for emitting the empty variants of the various literals.

> (deftype T [] clojure.lang.IPersistentCollection (count [_] 0)
> (def ^:const t (T.))
> (meta t)
java.lang.UnsupportedOperationException: Unknown Collection type
Compiler.java:2860 clojure.lang.Compiler$EmptyExpr.emit
Compiler.java:3632 clojure.lang.Compiler$InvokeExpr.emitArgsAndCall
...

EDIT updated the ticket after some investigation
NOTE attached test patch doesn't even implement (count []) for the deftype, which just triggers a rightful AbstractMethodError



 Comments   
Comment by Herwig Hochleitner [ 29/Oct/14 10:00 PM ]

The test had a typo, sorry

Comment by Alex Miller [ 30/Oct/14 7:14 AM ]

Looks like a variant of CLJ-1093.





[CLJ-1574] Vars defined in wrong namespace if ns form is not top-level Created: 28/Oct/14  Updated: 28/Oct/14  Resolved: 28/Oct/14

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

Type: Defect Priority: Major
Reporter: Tassilo Horn Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

I have a macro that given some file containing some data model description generates an API for accessing instances of that data model. That's the scenario although it's not really relevant. I've tracked down the issue to this minimal example.

This does the right thing:

;; in namespace user
(do (ns myns1)
    (defn myns1-fn [] nil)
    (in-ns 'user))

A new namespace myns1 is created containing one var myns1-fn. Now I can call (myns1/myns1-fn) and get 1.

However, the following does not work correctly:

;; in namespace user
(when-not (find-ns 'myns2)
  (do (ns myns2)
    (defn myns2-fn [] nil)
    (in-ns 'user)))

My intention is not to re-create the namespace myns2 in case it already exists. However, the result after the first evaluation (where myns2 doesn't exist yet) is that a new namespace myns2 is created, but the var myns2-fn is created in the user namespace (or whatever the current namespace is).

I know that `do` has some special casing to allow the first example. And the second example has an `if` at the top-level, so that's probably why it doesn't work. But it seems like a legit thing to do to test if a namespace exists, and if not, define it. E.g., you might have some optional dependency, and if it's not fulfilled, you just define the vars that you need yourself.



 Comments   
Comment by Nicola Mometto [ 28/Oct/14 6:15 AM ]

You can do what you are asking for by using intern rather than def

Comment by Alex Miller [ 28/Oct/14 9:02 AM ]

Something like this should work:

(when-not (find-ns 'myns2)
  (create-ns 'myns2)
  (intern 'myns2 'myns2-fn (fn [] "hello")))
Comment by Tassilo Horn [ 28/Oct/14 10:17 AM ]

Thanks Nicola and Alex. Using `intern` and `create-ns` is probably the better approach as it works in both cases.

But shouldn't that be somehow visible from the docs? Currently, `ns` says it changes the current value of `ns`, and `def` says it defines a var in the current namespace (`ns`). That leaves the impression that the second example is valid.

So maybe the docs of `def` and `ns` should contain a sentence like "If you want to create namespaces/Vars dynamically, prefer using `create-ns`/`intern` over `ns`/`def`."

Comment by Nicola Mometto [ 28/Oct/14 10:28 AM ]

Tassillo, I don't think ns is problematic here.
The issue is that def interns the var at compile time rather than at runtime and thus uses the compile time value of ns rather than the runtime one.

Comment by Tassilo Horn [ 28/Oct/14 2:48 PM ]

Thanks for the clarification, Nicola.





[CLJ-1573] Support (Java) transient fields in deftype, e.g. for hashcodes Created: 26/Oct/14  Updated: 26/Oct/14

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

Type: Enhancement Priority: Minor
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, deftype

Attachments: Text File 0001-transient-field-deftype.patch    
Patch: Code and Test

 Description   

Enhance deftypes to allow fields to be marked ACC_TRANSIENT.

strawman syntax:
(deftype AType [^:transient hash])

Came across this need while experimenting with a reified range written in a deftype, not in Java.

Patch doesn't include docstring change, but has a test.






[CLJ-1572] into does not work with IReduceInit Created: 24/Oct/14  Updated: 24/Oct/14

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

Type: Defect Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File clj-1572.patch    
Patch: Code
Approval: Incomplete

 Description   

This should work:

(into []
  (reify clojure.lang.IReduceInit
    (reduce [_ f start]
      (reduce f start (range 10)))))
IllegalArgumentException Don't know how to create ISeq from: user$eval5$reify__6
	clojure.lang.RT.seqFrom (RT.java:506)
	clojure.lang.RT.seq (RT.java:487)
	clojure.core/seq--seq--4091 (core.clj:135)
	clojure.core.protocols/seq-reduce (protocols.clj:30)
	clojure.core.protocols/fn--6422 (protocols.clj:42)
	clojure.core.protocols/fn--6369/f--6255--auto----G--6364--6382 (protocols.clj:13)
	clojure.core/reduce (core.clj:6469)
	clojure.core/into (core.clj:6550)


 Comments   
Comment by Alex Miller [ 24/Oct/14 10:40 AM ]

into calls reduce which calls into CollReduce. CollReduce extends to IReduce, but not to IReduceInit. If CollReduce were extended to IReduceInit for the arity it can support, into work as expected in the given example. Patch clj-1572.patch does this.





[CLJ-1571] Transducer of partition-by over take gives wrong answer Created: 20/Oct/14  Updated: 21/Oct/14  Resolved: 21/Oct/14

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

Type: Defect Priority: Critical
Reporter: Alex Miller Assignee: Rich Hickey
Resolution: Completed Votes: 1
Labels: transducers

Attachments: Text File 0001-CLJ-1571-fix-regression-introduced-by-43cc1854508d65.patch     Text File CLJ-1571.patch    
Approval: Ok

 Description   
(partition-by pos? (take 2 [-1 1]))
=> ((-1) (1))
(sequence (comp (take 2) (partition-by pos?)) [-1 1])
=> ([-1])


 Comments   
Comment by Nicola Mometto [ 21/Oct/14 7:49 AM ]

Given that it works fine when using transduce instead of sequence, the bug might be in LazyTransformer rather than in partition-by.

(into [] (comp (take 2) (partition-by pos?)) [-1 1])
=> [[-1] [1]]
Comment by Ghadi Shayban [ 21/Oct/14 9:21 AM ]

Patch fixes the test case, but needs eyes, I certainly may have broken something. This highlights the importance of CLJ-1554, something similar to the existing defequiv tests for reducers, but between #'into and #'sequence, also covering edge cases in reduced unwrapping.

Comment by Alex Miller [ 21/Oct/14 9:41 AM ]

Thanks Ghadi. This bug was found by the tests I wrote for CLJ-1554, so yes.

Comment by Nicola Mometto [ 21/Oct/14 9:53 AM ]

Applying this patch causes a regression in the lazyiness of sequence.
The lines that Ghadi removed for this patch were added by Rich in this commit https://github.com/clojure/clojure/commit/43cc1854508d655e58e377f84836ba128971f90c to address http://dev.clojure.org/jira/browse/CLJ-1497

Example of the regression:
current master:

user=>  (sequence (take 2) (map #(do (println (str "~" %)) %) (iterate inc 1)))
~1
~2
(1 2)

with this patch:

user=>  (sequence (take 2) (map #(do (println (str "~" %)) %) (iterate inc 1)))
~1
~2
~3
(1 2)
Comment by Nicola Mometto [ 21/Oct/14 10:03 AM ]

Patch 0001-CLJ-1571-fix-regression-introduced-by-43cc1854508d65.patch addresses this issue while preserving the current lazyness factor of `sequence`

Comment by Alex Miller [ 21/Oct/14 11:09 AM ]

Rich has a (different) patch for this on the way.

Comment by Alex Miller [ 21/Oct/14 1:16 PM ]

Fixed directly by Rich in commit https://github.com/clojure/clojure/commit/38d7572e4254afdd7f02b78095ccdb27065754d2





[CLJ-1570] Core clojure code mixes tabs with spaces Created: 20/Oct/14  Updated: 20/Oct/14

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

Type: Defect Priority: Trivial
Reporter: Michael Blume Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

A handful of functions in clojure.core, clojure.core-proxy, clojure.inspector, clojure.xml, clojure.pprint, clojure.stacktrace, clojure.set, and clojure.test switch partway through from indenting with spaces to indenting with tabs. This may cause them to display incorrectly depending on how the developer's editor is configured.

(not sure if this should be marked defect or task)



 Comments   
Comment by Andy Fingerhut [ 20/Oct/14 1:41 PM ]

Some similarities to CLJ-1026, although this problem does not cause the same issues with warnings on git patches as CLJ-1026 does, as far as I know.

One similarity is that if it is of interest (I don't know if it is), Alex or other Clojure screeners may want a procedure to clean them all up, and perhaps repeat that process periodically, e.g. before each major release.





[CLJ-1569] transduce does not respect the init arity of transducers Created: 19/Oct/14  Updated: 20/Oct/14  Resolved: 20/Oct/14

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

Type: Defect Priority: Minor
Reporter: Daniel James Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: transducers


 Description   

Note: I initially raised this issue for discussion on the mailing list
https://groups.google.com/d/msg/clojure/uVKP4_0KMwQ/-oUJahvUarIJ

transduce and other transducible processes currently ignore the 'init' arity of transducers. The currently implementation of transduce takes the 'init' from the reducing function before being transformed by the transducer, rather the reducing function after being transformed.

The current implementation of transduce is equivalent to the following (simplified for exposition purposes):

Current implementation of transduce
(defn transduce
  ([xform f coll]
     (transduce xform f (f) coll))
  ([xform f init coll]
     (let [rf (xform f)]
       (rf (reduce rf init coll)))))

The arity 3 case uses (f) to construct the seed value of the reduction. The arity 4 case uses the explicitly provided seed, init.

I would like to propose an alternate implementation of transduce, one which makes use of the transducer when seeding the reduction.

Proposed implementation of transduce
(defn alt-transduce
  ([xform f coll]
     (let [rf (xform f)]
       (rf (reduce rf (rf) coll))))
  ([xform f init coll]
     (let [rf (xform
               (fn
                 ([] init)
                 ([result] (f result))
                 ([result input] (f result input))))]
       (rf (reduce rf (rf) coll)))))

Now, the arity 3 case uses (xform f) to construct the seed value of the reduction. The arity 4 case combines both f and init into a new reducing function that is given to xform. Both of these ensure that the init arity of the transducer is used.

As into is implemented in terms of transduce, it is also taken care of. However, sequence is separate, and would also have to be tweaked to respect the init arity.



 Comments   
Comment by Daniel James [ 19/Oct/14 1:24 PM ]

As a small addition, I just wanted to point out an example of where the current implementation raised curiosity:
https://groups.google.com/d/msg/clojure/M-13lRPfguc/IspgdpKDaGsJ

Comment by Alex Miller [ 20/Oct/14 9:12 AM ]

In transduce, the transducer is applied to the elements of the input and should not be entangled with the accumulation at all (either in initializing it or the act of accumulation). f is the final reducing function that deals with accumulation and initialization.

Comment by Daniel James [ 20/Oct/14 10:00 AM ]

Hi Alex,

I feel that you've misunderstood my proposal.

Could you explain how you consider

(defn init-with [x]
  (fn [rf]
    (fn
      ([] (rf (rf) x))
      ([result] (rf result))
      ([result input] (rf result input)))))

to be “entangled with the accumulation at all (either in initializing it or the act of accumulation).”

This seems like a completely legitimate transducer to me. It makes use of the init arity, while remaining oblivious to the accumulation.

Your explanation also seems to be at odds with

http://clojure.org/transducers

The inner function is defined with 3 arities used for different purposes:

  • Init (arity 0) - in most cases, this will just call the init arity on the nested transform xf, which will eventually call out to the transducing process to supply an initial value. It is also a place to establish the initial reducing state for the transducer.
Comment by Alex Miller [ 20/Oct/14 11:57 AM ]

By "entangling" I mean that in your alternate transduce you invoke the xform to obtain the initial value: ((xform f)) instead of (f). Transducers should not know about or be involved in the accumulating process.

The transducers page is in error and I will correct it (I wrote it; the error is mine).

Comment by Daniel James [ 20/Oct/14 3:25 PM ]

Ok, at the risk of belaboring the point (I have enough self-awareness to realized that I am probably about to do exactly that…) I feel that you are still missing something here.

Permit me to try one more time to explain my position.

Consider map

the map transducer
(defn map [f]
  (fn [rf]
    (fn
      ([] (rf))
      ([result] (rf result))
      ([result input] (rf result (f input))))))

It defines all three arities, init, step, and completion. It doesn’t have anything to do in init arity, and so the only thing it can do is “call the init arity on the nested transform rf, which will eventually call out to the transducing process.” (taken from your update to http://clojure.org/transducers)

Saying that transducers should not be involved in the accumulating process has the right spirit, but you are missing something. It is involved, but in a strictly constrained way. The transducer’s responsibility is to carefully thread the accumulator value around. Sure, it should not know what the value is, or what type it has, but it is still there. Every arity of map has access to it! In the init arity, map delegates to rf to construct it. In the completion arity, map has the result, but the only valid thing it can do with it is to pass it on to rf. Again, in the step arity, map has the result, and again the only legitimate thing it can do with it is to thread to through to rf.

Now consider the identity transducer:

the identity transducer
(def identity
  (fn [rf]
    ([] (rf))
    ([result] (rf result))
    ([result input] (rf result input))))

This is a transducer in its purest form. All it has to do is correctly thread the accumulation value around. It doesn’t and shouldn’t know any details of what that value is, nonetheless, it still has the responsibility of threading that value correctly.

In each arity the identity transducer does the ‘trivial’ thing. In my post to the mailing list, I illustrated three example of transducers that do something beyond the trivial thing in each of the three arities. (I’ll copy them here for completeness.)

non trivial threading of the accumulator in the init arity
(defn init-with
  [x]
  (fn [rf]
    (fn
      ([] (rf (rf) x))
      ([result] (rf result))
      ([result input]
         (rf result input)))))
non trivial threading of the accumulator in the completion arity
(defn complete-with
  [x]
  (fn [rf]
    (fn
      ([] (rf))
      ([result]
         (rf (rf result x)))
      ([result input]
         (rf result input)))))
non trivial threading of the accumulator in the step arity
(defn dupl
  []
  (fn [rf]
    (fn
      ([] (rf))
      ([result] (rf result))
      ([result input]
         (rf (rf result input)
             input)))))

I would consider all of these to be perfectly valid transducers. However, unless I’ve misunderstood, you appear to be taking issue with init-with. If so, I’m very curious as to why!

a closer look at the init arity of init-with
(defn init-with
  [x]
  (fn [rf]
    (fn
      ([] (rf (rf) x))
      ...

Rather than just delegating to (rf), it threads that value immediately into rf with (rf (rf) x). So I don’t agree at all that any of these, init-with, complete-with, or dupl, are “entangled” with the accumulation value or the accumulation process. They are completely oblivious to both its value and its type!

So, returning to transduce,

the first case of an alternate transduce
(defn alt-transduce
  ([xform f coll]
     (let [rf (xform f)]
       (rf (reduce rf (rf) coll))))
  ...

A valid transducer is one that threads the accumlation value correctly. Therefore, ((xform f)) is (f) threaded through xform. All the transducers in clojure.core have the trivial ([] (rf)), so ((xform f)) built from these core transducers degenerates into (identity (f)).
However, as transduce, into, and sequence never even invoke the init arity, it begs the question, why even require that transducers have that arity in the first place? Personally, I think that init arity is great as it enables a transducer such as init-with (while remaining stateless), but that requires transducible processes to actually make use of the init arity! Hence why I raised this issue.
It seems troubling to me that complete-with works perfectly fine in the current framework, yet init-with, its dual, does not.

I recognize that the various discussions around ‘typing transducers’ have made various approximations at elucidating the properties of transducers, but I feel strongly that the discussions around rank-2 polymorphism have some bearing on exactly this issue. In fact, it says rather a lot about correctly threading the accumulation value throught transducers without ever “entangling” it in the precise accumulation process of where a transducer is being used.

And on this, it appears that Rich Hickey agrees: “The rank-2 type in particular captures an important property.” (http://conscientiousprogrammer.com/blog/2014/08/07/understanding-cloure-transducers-through-types/#comment-1533318972) Maybe I’ve got him all wrong, but as of right now I’m pretty convinced I don’t. Still, I’m willing to be convinced otherwise

Comment by Alex Miller [ 20/Oct/14 10:03 PM ]

Rich asked me to decline the ticket because the init arity of the xform should not be involved in the reducing function accumulation.

Comment by Daniel James [ 20/Oct/14 10:34 PM ]

Ok, as you can guess I’m a little perplexed by that design choice, but I’ll accept it.

I’d appreciate any further insight you can offer on why this design choice has been taken.
Is the init arity simply a case of compatibility, despite it not being used? Is this a case of attempting to prevent the transducer writer from erroneously corrupting a transducible process? Is init-with actually actually considered to be an invalid transducer, and thus the only way to implement something equivalent would be as a stateful transducer?





[CLJ-1568] Incorrect error locations reported in the stacktrace Created: 19/Oct/14  Updated: 22/Oct/14

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

Type: Defect Priority: Major
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 16
Labels: errormsgs, ft

Attachments: Text File 0001-CLJ-1568-fix-incorrect-error-locations.patch    
Patch: Code
Approval: Triaged

 Description   

The following code produces an incorrect stacktrace:

(ns clojure-demo.core)

(defn foo
  "I don't do a whole lot."
  [x]
  (println x "Hello, World!"))

(/ 1 0)
Exception in thread "main" java.lang.ArithmeticException: Divide by zero, compiling:(clojure_demo/core.clj:6:31)

The problem is actually on the 8th line. As a matter of fact - there's nothing at location 6:31.
This is a pretty serious problem as many tools parse stacktraces for error locations.
Here's a related discussion in cider's issue tracker.

Patch: 0001-CLJ-1568-fix-incorrect-error-locations.patch
Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 19/Oct/14 1:39 PM ]

Maybe a dupe of CLJ-1561 ?

Comment by Andy Fingerhut [ 19/Oct/14 4:16 PM ]

I tried out the example given in the description, with the latest Clojure master as of today plus the patch for CLJ-1561 called 0002-Mark-line-number-after-emitting-children.patch, dated Oct 10 2014.

The line:column number 6:31 is the same for that patched version as it is in the ticket description, which is for Clojure 1.6.0.

The issue of misleading line:column numbers is common between the two tickets, but at least the proposed improvement in CLJ-1561's patch is not effective for improving this issue.

Comment by Bozhidar Batsov [ 20/Oct/14 1:36 AM ]

I know that the issue list for 1.7 is pretty much finalised, but I think that this issue and and CLJ-1561 should be fixed as soon as possible.
Correct error reporting is extremely important IMO.

Comment by Nicola Mometto [ 20/Oct/14 8:28 AM ]

Attached a patch that fixes the issue by consuming all the whitespaces before retrieving line/column info for the next form.

Comment by Alex Miller [ 20/Oct/14 8:39 AM ]

Are there possible downsides to more eagerly consuming whitespace as done in the patch?

Comment by Nicola Mometto [ 20/Oct/14 8:44 AM ]

I can't think of any

Comment by Paul Stadig [ 22/Oct/14 2:59 PM ]

The defect on master does not have effect when using compile:

user=> (require 'clojure-demo.core)

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(clojure_demo/core.clj:6:31) 
user=> (load "/clojure_demo/core")

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(clojure_demo/core.clj:6:31) 
user=> (compile "clojure_demo/core")

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(core.clj:8:1) 

With the patch applied all the line numbers are the same in all cases:

user=> (require 'clojure-demo.core)

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(clojure_demo/core.clj:8:1) 
user=> (load "/clojure_demo/core")

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(clojure_demo/core.clj:8:1) 
user=> (compile "clojure_demo/core")

CompilerException java.lang.ArithmeticException: Divide by zero, compiling:(core.clj:8:1) 

Agreed that this seems to be orthogonal to CLJ-1561.





[CLJ-1567] Unused local in clojure.core/condp definition Created: 17/Oct/14  Updated: 20/Oct/14

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

Type: Enhancement Priority: Trivial
Reporter: Jan Krajicek Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: ft, newbie

Attachments: Text File 0001-Remove-unused-local-in-clojure.core-condp.patch    
Patch: Code
Approval: Triaged

 Description   

The 'gres' local in clojure.core/condp definition is not used:

https://github.com/clojure/clojure/blob/eccff113e7d68411d60f7204711ab71027dc5356/src/clj/clojure/core.clj#L6071

Screened by: Alex Miller



 Comments   
Comment by Bozhidar Batsov [ 19/Oct/14 12:07 AM ]

Patch added.





[CLJ-1566] Documentation for clojure.core/require does not document :rename Created: 16/Oct/14  Updated: 19/Oct/14

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

Type: Defect Priority: Minor
Reporter: James Laver Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File refer.patch    
Patch: Code

 Description   

By contrast, clojure.core/use does mention :rename.

I attach a patch



 Comments   
Comment by Andy Fingerhut [ 16/Oct/14 1:33 PM ]

James, your patch removes any mention of the :all keyword, and that keyword is not mentioned in the doc string for clojure.core/refer.

I haven't checked whether refer can take :all as an argument, but clojure.core/require definitely can.

Comment by James Laver [ 16/Oct/14 1:39 PM ]

Ah, you're quite right. Fixed now. See updated patch in a sec.

Comment by Andy Fingerhut [ 16/Oct/14 8:16 PM ]

For sake of reduced confusion, it would be better if you could either name your patches differently, or delete obsolete ones with identical names as later ones. JIRA allows multiple patches to have the same names, without replacing the earlier ones.

Comment by James Laver [ 17/Oct/14 12:44 AM ]

Okay, that's done. The JIRA interface is a bit tedious in places.

Comment by Bozhidar Batsov [ 19/Oct/14 1:34 AM ]

Seems to me the sentence should end with a dot.

Comment by James Laver [ 19/Oct/14 4:36 AM ]

Added a dot.





[CLJ-1565] pprint issues infinite output for a protocol Created: 15/Oct/14  Updated: 15/Oct/14

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

Type: Defect Priority: Minor
Reporter: Michael Nygard Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Using pprint with a protocol name generates an unending stream of output. pprint appears to recurse through the Var reference as the value of the :var key in the protocol definition itself.

To reproduce:

user=> (defprotocol Foo (foo-you [this]))
Foo
user=> (pprint Foo)
{:on user.Foo,
:on-interface user.Foo,
:sigs {:foo-you {:doc nil, :arglists ([this]), :name foo-you}},
:var
#<Var@6a3b02d8:
{:on user.Foo,
:on-interface user.Foo,
:sigs {:foo-you {:doc nil, :arglists ([this]), :name foo-you}},
:var
#<Var@6a3b02d8:
{:on user.Foo,
:on-interface user.Foo,
:sigs {:foo-you {:doc nil, :arglists ([this]), :name foo-you}},
:var
#<Var@6a3b02d8:
{:on user.Foo,
:on-interface user.Foo,
:sigs
{:foo-you {:doc nil, :arglists ([this]), :name foo-you}},
:var
#<Var@6a3b02d8:
{:on user.Foo,
:on-interface user.Foo,
:sigs
{:foo-you {:doc nil, :arglists ([this]), :name foo-you}},
:var
#<Var@6a3b02d8:
{:on user.Foo,
:on-interface user.Foo,
:sigs
{:foo-you
{:doc nil, :arglists ([this]), :name foo-you}},






[CLJ-1564] Sum/sub decimals operation bug Created: 15/Oct/14  Updated: 15/Oct/14  Resolved: 15/Oct/14

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

Type: Defect Priority: Critical
Reporter: Luca Gugole Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: math

Patch: Code

 Description   

The result of operation (+ 0.7 0.1) is 0.7999999999999999 and not 0.7

Other operations with the same behaviour:
(+ 0.11 0.1) => 0.21000000000000002
(+ 0.31 0.1) => 0.41000000000000003
(- 0.8 0.1) => 0.7000000000000001
(- 0.41 0.1) => 0.30999999999999994



 Comments   
Comment by Oliver Charles [ 15/Oct/14 6:44 AM ]

Uh, isn't this just normal floating point arithmetic?

Comment by Luca Gugole [ 15/Oct/14 7:32 AM ]

But the result of other operations ((+ 0.1 0.1), (+ 0.2 0.2), (+ 0.2 0.3) ...) has only one decimal place.
It's normal?
I have to perform a math round operation to obtain only one decimal place?

Comment by Jozef Wagner [ 15/Oct/14 7:41 AM ]

This is not a bug. Please read What Every Programmer Should Know About Floating-Point Arithmetic

Comment by Luca Gugole [ 15/Oct/14 7:51 AM ]

Sorry about my lack of knowledge on this subject.
Thank you for the answer.





[CLJ-1563] How About Default Implementations on Protocols Created: 11/Oct/14  Updated: 12/Oct/14

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

Type: Enhancement Priority: Minor
Reporter: David Williams Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Consider this example

user=> (defprotocol Foo (foo [x] x))
Foo
user=> (defrecord Bar [gaz waka] Foo)
user.Bar
user=> (def bar (Bar. 1 2))
#'user/bar
user=> (.foo bar)

AbstractMethodError user.Bar.foo()Ljava/lang/Object;  sun.reflect.NativeMethodAccessorImpl.invoke0 (NativeMethodAccessorImpl.java:-2)
user=>

What about the default implementation.



 Comments   
Comment by David Williams [ 11/Oct/14 8:48 PM ]

As it stands you have to workaround with this

http://stackoverflow.com/questions/15039431/clojure-mix-protocol-default-implementation-with-custom-implementation

Comment by Jozef Wagner [ 12/Oct/14 1:01 AM ]

I don't think we need it. What's the rationale behind extending some protocol, not implementing its methods, and then calling those methods, expecting them not to throw. Be explicit about what yout type should do, whether it is a default or custom behavior. You basically have three options

(defn default-foo 
  [this] 
  :foo)

(defprotocol P
  (-foo [this]))

(deftype T
  P
  (-foo [this] (default-foo))

(defn foo 
  [x]
  (-foo x))

or

(defprotocol P
  (-foo [this]))

(deftype T)

(defn foo 
  [x]
  (if (satisfies? P x)
    (-foo x)
    :foo))

or

(defprotocol P
  (-foo [this]))

(extend-protocol P
  java.lang.Object
  (-foo [this] :foo))

(deftype T)

(defn foo 
  [x]
  (-foo x))

I think however that my first approach is unidiomatic and you should prefer the latter ones.

Comment by David Williams [ 12/Oct/14 12:36 PM ]

I agree, this is a low priority enhancement. I think it could make the Protocol experience more DWIMy, and Java 8 has default implementations on interfaces for the same kind of convenience.





[CLJ-1562] some->,some->>,cond->,cond->> and as-> doesn't work with (recur) Created: 11/Oct/14  Updated: 11/Oct/14

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

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

Patch: Code
Approval: Triaged

 Description   

some-> and his friends doesn't work with recur, because they never place the last expression in tail position. For example:

(loop [l [1 2 3]] 
  (some-> l 
          next 
          recur))

raises UnsupportedOperationException: Can only recur from tail position

This is similar to the bug reported for as-> at http://dev.clojure.org/jira/browse/CLJ-1418 (see the comment at http://dev.clojure.org/jira/browse/CLJ-1418?focusedCommentId=35702&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-35702)

It can be fixed by changing the some-> definition to:

(defmacro some->
  "When expr is not nil, threads it into the first form (via ->),
  and when that result is not nil, through the next etc"
  {:added "1.5"}
  [expr & forms]
  (let [g (gensym)
        pstep (fn [step] `(if (nil? ~g) nil (-> ~g ~step)))]
    `(let [~g ~expr
           ~@(interleave (repeat g) (map pstep (butlast forms)))]
       ~(if forms
          (pstep (last forms))
          g))))

Similar fixes can be done for some->>, cond->, cond->> and as->.

Note -> supports recur without problems, fixing this will homogenize *-> macros behaviour.






[CLJ-1561] Incorrect line numbers are emitted Created: 10/Oct/14  Updated: 22/Oct/14

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

Type: Defect Priority: Major
Reporter: Paul Stadig Assignee: Unassigned
Resolution: Unresolved Votes: 22
Labels: errormsgs

Attachments: Text File 0001-Mark-line-number-after-emitting-children.patch     Text File 0002-Mark-line-number-after-emitting-children.patch    
Patch: Code
Approval: Triaged

 Description   

The Clojure JVM compiler marks the line number for a form before emitting the children for that form. Marking the line number before emitting children leads to incorrect line numbers when a runtime error occurs. For example, when

 (foo bar
      baz)

is emitted the compiler will visit the line number for the expression, then emit the children expressions ('bar' and 'baz') which will mark their own line numbers, then come back and emit the invoke bytecode for 'foo', but since the last line number to be marked was that of 'baz', if 'foo' throws an exception the line number of 'baz' will be reported instead of the line number for the expression as a whole.

This same issue was being manifested with special forms and inlined functions, and was especially bad in the case of the threading macro '->', because it is usually spread across several lines, and the line number reported could end up being very different than the line actually causing an exception.

A demonstration of the incorrect line numbers (and how the fix affects line numbers) can be seen here https://github.com/pjstadig/clojure-line-numbers



 Comments   
Comment by Jozef Wagner [ 10/Oct/14 1:57 PM ]

additions in your patch mixes tabs and spaces. Could you please update the patch so that your added lines indent only with tab characters? Not everyone has tab set at 4 spaces...

Comment by Paul Stadig [ 10/Oct/14 2:42 PM ]

There's already a mixture of just tabs, just spaces, and tabs & spaces in Compiler.java. I'm not sure what the "standard" is, but I've changed the patch to match the surrounding lines.

Comment by Paul Stadig [ 10/Oct/14 2:42 PM ]

Patch with whitespace changes.

Comment by Alex Miller [ 20/Oct/14 8:38 AM ]

These changes will affect the line number tables for a variety of Clojure constructs when compiled. It would be very helpful to me to have a set of examples that covered each case touched in the patch so that I could compile them and look at the bytecode vs the source. This would greatly accelerate the screening process.

Comment by Paul Stadig [ 20/Oct/14 2:29 PM ]

Alex,
I have created a repo on github that has a sample file demonstrating the line number changes.

https://github.com/pjstadig/clojure-line-numbers

Hope that helps!

BTW, I'd be glad to do a skype call or hangout, if you have questions.

Comment by Alex Miller [ 20/Oct/14 2:34 PM ]

This is very helpful, thanks!!

Comment by Alex Miller [ 22/Oct/14 11:35 AM ]

In the hunk at 3191 in KeywordInvokeExpr, a call to visitLineNumber was added, but the prior call 4 lines earlier was not removed. Should it be?

Comment by Paul Stadig [ 22/Oct/14 12:05 PM ]

I left that in thinking that if something goes wrong with the getstatic instruction (null pointer exception? class cast exception?) it should report the line number of the KeywordInvokeExpr. It may be that there isn't a realistic possibility that anything could actually happen with that getstatic instruction, but that was the thought process.

My general rule of thumb was if an emit method emits any instructions before it calls the emit method on another expr, then it should mark its line number before and after the recursive emit call (assuming that the recursive emit call would mark its own line number). In cases where an emit method immediately calls another emit method, then I don't bother to mark a line number until afterwards.





[CLJ-1560] Forbid closing over mutable fields completely Created: 10/Oct/14  Updated: 10/Oct/14

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

Type: Defect Priority: Major
Reporter: Jozef Wagner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Closing over mutable fields should be forbidden completely (and generate compiler exception), not just when trying to set! them. As the change of the mutable field does not propagate into closed over ones, this leads to surprising bugs:

(defprotocol P 
  (-set [this]) 
  (-get [this]) 
  (-get-fn [this]))

(deftype T [^:unsynchronized-mutable val] 
  P 
  (-set [this] (set! val :bar)) 
  (-get [this] val) 
  (-get-fn [this] (fn [] val)))

(def x (->T :foo))

(def xf (-get-fn x))

user> (-set x)
:bar
user> (-get x)
:bar
user> (xf)
:foo ;; should be :bar !!!


 Comments   
Comment by Jozef Wagner [ 10/Oct/14 1:42 PM ]

related issue CLJ-274





[CLJ-1559] A function bound in let can only be used in a macro if it is parameterless Created: 09/Oct/14  Updated: 10/Oct/14  Resolved: 09/Oct/14

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

Type: Defect Priority: Minor
Reporter: Colin Smith Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

MacOS


Attachments: File stack.trace    

 Description   

This works:

(defn make-fn [] (fn [x] (+ 3 x)))

(defmacro m [x]
(let [a-function (make-fn)]
`(fn [z#]
(+ ~x (~a-function z#)))))

(prn ((m 1) 2))

;;;;;;;;;;;;;;;;;;

But this does not (adding a parameter to make-fn foils things):

(defn make-fn [y] (fn [x] (+ y x)))

(defmacro m [x]
(let [a-function (make-fn 3)]
`(fn [z#]
(+ ~x (~a-function z#)))))

(prn ((m 1) 2))

;;;;;;;;;;;;;;;;;;;

stack trace attached.



 Comments   
Comment by Alex Miller [ 09/Oct/14 11:51 PM ]

At a glance, you appear to be dropping the evaluated function object (a-function) inside the syntax quote, which is pretty much always a problem in how the macro is written.

Probably really want something like:

(defn make-fn [y] (fn [x] (+ y x)))
(defmacro m [x] 
  `(let [a-function# (make-fn 3)]
     (fn [z#] (+ ~x (a-function# z#)))))
(prn ((m 1) 2))
Comment by Alex Miller [ 09/Oct/14 11:53 PM ]

If you look at the expanded macro in your example:

user=> (pprint (clojure.walk/macroexpand-all '(prn ((m 1) 2))))
(prn
 ((fn*
   ([z__25__auto__]
    (clojure.core/+
     1
     (#<user$make_fn$fn__22 user$make_fn$fn__22@72995b29>
      z__25__auto__))))
  2))

Any time you see something like #<user$make_fn$fn_22 user$make_fn$fn_22@72995b29>, that's a good hint.

Comment by Colin Smith [ 10/Oct/14 1:21 AM ]

Thank you Alex. I had done the macro expansion.

I see my mistake: I am coming to Clojure from Scheme, where the result of (lambda ...) is a primitive value that I could expect to paste into the form produced by a macro. Now, I get the impression that (fn) is not required to produce quite that sort of thing.
Would you say that's the right way to look at it?

Thanks for looking at this so quickly! I am back on track.





[CLJ-1558] lazy-seq and seq return different values for (lazy-seq []) (seq []) Created: 09/Oct/14  Updated: 09/Oct/14  Resolved: 09/Oct/14

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

Type: Enhancement Priority: Minor
Reporter: Jeremy Betts Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

(lazy-seq [])
=> ()
(seq [])
=> nil

would expect both to return nil



 Comments   
Comment by Andy Fingerhut [ 09/Oct/14 10:02 AM ]

Jeremy, lazy-seq's documentation string says it: "returns a Seqable object that will invoke the body only the first time seq is called". Even (lazy-seq nil) does not return nil. seq's documentation string explicitly says that it will return nil for empty collections.

What leads you to expect both to return nil?

Comment by Alex Miller [ 09/Oct/14 10:12 AM ]

Working as expected per docs.

seq -> returns a sequence
lazy-seq -> returns a seqable (when seq is called on it, will return a sequence)

Comment by Jeremy Betts [ 09/Oct/14 10:38 AM ]

I listed as enhancement and not defect based on documentation.
lazy-seq:
"Takes a body of expressions that returns an ISeq or nil, and yields
a Seqable object that will invoke the body only the first time seq
is called, and will cache the result and return it on all subsequent
seq calls. See also - realized?"

it's not clearly stated what it should return for [] where as for seq, it clearly stated that it will return nil for [].

given the intent of lazy-seq is to make the same result as seq, except that its members are evaluated when called for and not upfront.

The current implementation forces code to be aware of if it's dealing with lazy or non lazy sequences. This is not ideal.

Once again, listed as feature enhancement, because of the less than ideal design and documentation. Listed as minor, as it's fairly easy to work around it.

-Jeremy

Comment by Jeremy Betts [ 09/Oct/14 10:41 AM ]

how do i reopen this as the answers to this are not well thought out.

Comment by Alex Miller [ 09/Oct/14 11:57 AM ]

Despite the similarity in naming, seq and lazy-seq have different purposes which I believe are adequately stated in their doc strings. Of particular note, the intent of lazy-seq is NOT to make a seq, but to make a seqable (conceptually similar to the difference between Iterator and Iterable in Java).

Sequences are a logical list. Empty sequences are represented by nil. seq produces a sequence from the input. Because it produces either nil or a sequence with at least one value, seq is often used in a condition or termination check when walking through a sequence.

lazy-seq is a tool that can be used to create lazy sequences from a function, but it delays that computation by returning a seqable (not a seq) so that computation will only be forced at the point where you start producing a seq from it.

Most sequence functions implicitly call seq on their input (thus producing seqs from seqables), so the difference between them can often be missed.

[] is a seqable. lazy-seq will itself call seq on the result of the generator function if it is not a lazy seq. So, you give [] to lazy-seq and it creates a seqable with a function that returns []. When you call seq on the seqable, the function is evaluated to a PersistentVector (not a lazy seq) and then seq is called on it, which produces a nil, which is returned.

I do not see how this affects callers. Because sequence functions implicitly call seq, all of the sequence functions will work with either and yield the same results. Explicit use of lazy-seq is relatively rare (it's most commonly used when a sequence is produced by repeated evaluation of a function).

You might find this page to be helpful: http://clojure.org/sequences

Comment by Jeremy Betts [ 09/Oct/14 1:06 PM ]

(if (lazy-seq []) "yes" "no")
=> "yes"
(if (seq []) "yes" "no")
=> "no"

The truth value of an empty seq and an empty lazy-seq is different.

I guess i'm still not understand why this would be a "bad" change to the behavior?

Comment by Andy Fingerhut [ 09/Oct/14 1:15 PM ]

Jeremy, a lot of Clojure code uses the return value of seq in the way you show in your example to decide whether there is more to a sequence to process.

I have never seen Clojure code use lazy-seq for any purpose other than to construct a lazy sequence and return it from a recursive function, to avoid blowing the stack.

(lazy-seq x) returns a truthy value for all values of x, even nil.

Comment by Jozef Wagner [ 09/Oct/14 1:30 PM ]

Jeremy, have look on a sequence if you do not want nil returned. And Andy had a good point. lazy-seq is a constructor of LazySeq type, and as such should not return nil. seq returns either nil or an object that implements ISeq interface, but does not prescribe any concrete type. the nil return value from seq is a feature and is a one of differences between how seq and sequence behaves.

Comment by Jeremy Betts [ 09/Oct/14 2:16 PM ]

Jozef Wagner's answer is good. 'sequence' is the equivalent to 'lazy-seq' This was a problem for me as the laziness of something could not be changed without the inadvertent changing of behavior. This is what i ran into and why i entered the issue.

I'd still argue that a document update would would be a good thing.

-Jeremy





[CLJ-1557] Nested reduced is broken Created: 09/Oct/14  Updated: 10/Oct/14  Resolved: 10/Oct/14

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

Type: Defect Priority: Critical
Reporter: Christophe Grand Assignee: Rich Hickey
Resolution: Completed Votes: 0
Labels: transducers

Attachments: File re-reduced.diff    
Patch: Code
Approval: Vetted

 Description   

Re-reduced from composed transformation functions:

  • re-wraps the Reduced when it should not (take)
  • forget to unwrap the Reduced (partition-by, partition-all)
; nested reduced
=> (transduce (comp (take 1)) conj [:a])
[:a]
=> (transduce (comp (take 1) (take 1)) conj [:a])
#<Reduced@65979031: [:a]>
=> (transduce (comp (take 1) (take 1) (take 1)) conj [:a])
#<Reduced@fcbc8d1: #<Reduced@60bea99a: [:a]>>
=> (transduce (comp (take 1) (take 1) (take 1) (take 1)) conj [:a])
#<Reduced@6e9915bb: #<Reduced@5c712302: #<Reduced@472b9f70: [:a]>>>
 
; problems not appearing in all contexts
; not ok with transduce
=> (transduce (comp (partition-by keyword?) (take 1)) conj [] [:a])
#<Reduced@5156c42e: [[:a]]>
; but ok with sequence
=> (sequence (comp (partition-by keyword?) (take 1)) [:a])
([:a])
; well, not always
=> (sequence (comp (partition-by keyword?) (take 1)  (partition-by keyword?) (take 1)) [:a])
ClassCastException clojure.lang.Reduced cannot be cast to clojure.lang.LazyTransformer  clojure.lang.LazyTransformer$Stepper$1.invoke (LazyTransformer.java:104)

See also: https://groups.google.com/d/msg/clojure-dev/cWzMS_qqgcM/7IAhzMKzVigJ



 Comments   
Comment by Ghadi Shayban [ 09/Oct/14 11:11 PM ]

Same with partition-all

(transduce (comp (take 1) (partition-all 3) (take 1)) conj [] (range 15))
 #<Reduced@84f8976: [[0]]>
Comment by Christophe Grand [ 10/Oct/14 5:50 AM ]

patch for take, partition-by and partition-all





[CLJ-1556] Add instance check functions to defrecord/deftype Created: 09/Oct/14  Updated: 09/Oct/14

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

Type: Enhancement Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: defrecord, deftype

Attachments: Text File 0001-CLJ-1556-Generate-type-functions-with-instance-check.patch    
Patch: Code

 Description   

It is often necessarty to test for instance? on deftypes/defrecords, this patch makes the two macros automatically generate a type? function implemented as (fn [x] (instance? type x)), to complement ->type and map->type
Example:

user=>(deftype x [])
user.x
user=>(x? (x.))
true


 Comments   
Comment by Jozef Wagner [ 09/Oct/14 9:11 AM ]

What about camel cased types? predicate SomeType? does not look like an idiomatic type predicate. I suggest to have this type predicate function and its name optional, through e.g. :predicate metadata on a type name. Moreover, it is far more useful to have such predicate on protocols, rather than types.

Comment by Nicola Mometto [ 09/Oct/14 9:17 AM ]

I don't think camel cased types should pose any issue. we use ->SomeType just as fine, I don't see why SomeType? should be problematic.

I disagree that it's more useful to have a predicate for protocols since protocols are already regular Vars and it's just a matter of (satisfies? theprotocol x), the value of the predicate on types/record is to minimize the necessity of having to import the actual class





[CLJ-1555] Set literal duplicate check not consistent with set semantics Created: 09/Oct/14  Updated: 09/Oct/14  Resolved: 09/Oct/14

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

Type: Defect Priority: Minor
Reporter: Tassilo Horn Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None


 Description   

Having two functions with the same definition in a set literal signals an error:

#{(fn []) (fn [])}
; IllegalArgumentException Duplicate key: (fn [])

On the other hand, even though the definitions are the same, it still are two different functions that aren't equal.

(= (fn []) (fn []))
;=> false
(conj #{} (fn []) (fn []))
;=> #{#<user$eval14553$fn__14556 user$eval14553$fn__14556@5f04ed52> #<user$eval14553$fn__14554 user$eval14553$fn__14554@6f3d47f5>}

Therefore, the set literal above should not complain about duplicate keys.



 Comments   
Comment by Jozef Wagner [ 09/Oct/14 7:30 AM ]

duplicate of CLJ-1538





[CLJ-1554] Need to expand tests to cover transducers Created: 07/Oct/14  Updated: 24/Oct/14

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

Type: Defect Priority: Critical
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Unresolved Votes: 0
Labels: transducers

Attachments: Text File clj-1554-2.patch     Text File clj-1554-3.patch     Text File clj-1554.patch    
Patch: Code and Test
Approval: Vetted

 Description   

Attached patch contains both some generative and example tests for transducers. The generative tests build a series of sequence functions (take 5, filter odd?, etc) and apply them to a random vector of numbers as seq transformations, sequence of transducer, into of transducer, and transduce of transducer. The results are compared.

Note: these tests depend on the patch in CLJ-1349 to run as tests.

Patch: clj-1554-3.patch



 Comments   
Comment by Fogus [ 24/Oct/14 1:44 PM ]

I downloaded and applied this patch and its dependent patch (1349) and ran the tests. The coverage is a good start and the approach of verifying results against results gathered from other approaches is important. One note of style is that the use of `doall` is inconsistent in the `apply-as-*` functions. i would recommend that at least one other person screen this patch as my grasp of test.check is tenuous.

Comment by Alex Miller [ 24/Oct/14 2:52 PM ]

Updated patch slightly to clean up the doall stuff.





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

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: transducers

Approval: Vetted

 Description   

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






[CLJ-1552] Consider kv support for transducers (similar to reducers fold) Created: 07/Oct/14  Updated: 07/Oct/14

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: transducers

Approval: Vetted

 Description   

In reducers, fold over a map has special support for kv. Consider whether/how to add this for transducers.






[CLJ-1551] Consider transducer support for primitives Created: 07/Oct/14  Updated: 07/Oct/14

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: transducers

Approval: Vetted

 Description   

Need to consider how we can support primitives for transducers. In particular it may be that IFn needs overloading for L/D in addition to O.






[CLJ-1550] Classes generated by deftype and defrecord don't play nice with .getPackage Created: 07/Oct/14  Updated: 07/Oct/14

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

Type: Defect Priority: Major
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: bug


 Description   
(.getPackage String)
;; => #<Package package java.lang, Java Platform API Specification, version 1.7>
(deftype T [])
(.getPackage T)
;; => nil

This seems like a bug to me as it's not obvious why the class generated by deftype should exhibit different behaviour.



 Comments   
Comment by Alex Miller [ 07/Oct/14 8:54 AM ]

According to http://docs.oracle.com/javase/7/docs/api/java/lang/Class.html#getPackage() this method returns the package information found by the class loader or null if there is none. Its not clear to me that the current behavior is wrong per the spec. I would need to experiment more to see if this is unusual or not.

Comment by Bozhidar Batsov [ 07/Oct/14 9:05 AM ]

A bit of background for the issue. I'm no expert on the topic, but being able to procure all the class information except its package definitely looks strange to me.

Comment by Kevin Downey [ 07/Oct/14 11:46 AM ]

if you AOT compile(generate a class file on disk for a deftype), getPackage works fine, which suggests to me it is a jvm issue

Comment by Kevin Downey [ 07/Oct/14 11:49 AM ]

actually, it must just be that dynamicclassloader doesn't define a package for classes it loads

Comment by Alex Miller [ 07/Oct/14 12:13 PM ]

Yep, I believe that's correct.





[CLJ-1549] split IReduce Created: 06/Oct/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

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

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

Attachments: Text File clj-1549-2.patch     Text File clj-1549.patch    
Patch: Code
Approval: Ok

 Description   
  • IReduceInit should take arity-2 version from existing IReduce
  • IReduce should extend IReduceInit and add arity-1
  • new stuff should implement IReduceInit only (audit everything added for 1.7)
  • old stuff should not change or break

Patch: clj-1549-2.patch



 Comments   
Comment by Alex Miller [ 06/Oct/14 4:56 PM ]

Patch does as requested. Did not change the CollReduce extension which currently needs both arities:

(extend-protocol CollReduce
  ...

  clojure.lang.IReduce
  (coll-reduce
   ([coll f] (.reduce coll f))
   ([coll f val] (.reduce coll f val)))
Comment by Rich Hickey [ 07/Oct/14 8:29 AM ]

Can we please use the name IReduceInit instead of ILeftReduce?





[CLJ-1548] primitive type hints on protocol methods break call sites Created: 04/Oct/14  Updated: 04/Oct/14

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

Type: Defect Priority: Minor
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   
user=> (defprotocol P (f [this ^long x]))
P
user=> (deftype T [] P (f [_ x] x))
#<java.lang.Class class user.T>
user=> (f (T.) 5)

ClassCastException user$eval7289$fn__7290$G__7280__7297 cannot be cast to clojure.lang.IFn$OLO  user/eval7313 (NO_SOURCE_FILE:1)





[CLJ-1547] range cause OutOfMemoryError when start > end AND step is zero Created: 04/Oct/14  Updated: 04/Oct/14  Resolved: 04/Oct/14

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

Type: Defect Priority: Minor
Reporter: Édipo L Féderle Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

Mac OSX 10.9.4
java version "1.7.0_51"
Java(TM) SE Runtime Environment (build 1.7.0_51-b13)
Java HotSpot(TM) 64-Bit Server VM (build 24.51-b03, mixed mode)



 Description   

I am playing with range function and I tried the following args:

(range 10 4 0) ;=> OutOfMemoryError

I add a test on range code to check for this condition, but probably I dont do it right, one test fail and I dont know why. If this is really a bug and someone can help me with this patch I appreciate it.



 Comments   
Comment by Nicola Mometto [ 04/Oct/14 8:43 AM ]

Not a bug, the docstring is explicit about this: "When step is equal to 0, returns an infinite sequence of start."

Comment by Édipo L Féderle [ 04/Oct/14 8:49 AM ]

Oh yeah, my fault. I will close this "issue".

Thanks Nicola.

Comment by Édipo L Féderle [ 04/Oct/14 8:49 AM ]

not a bug





[CLJ-1546] Widen vec to take Iterable/IReduce Created: 02/Oct/14  Updated: 24/Oct/14

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

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

Attachments: Text File clj-1546-2.patch     Text File clj-1546-3.patch     Text File clj-1546.patch    
Patch: Code and Test
Approval: Vetted

 Description   

These examples should work but does not:

Something Iterable but not IReduce:

user> (def i (eduction (map inc) (range 100)))
#'user/i
user> (instance? java.util.Collection i)
false
user> (instance? Iterable i)
true
user> (vec i)
RuntimeException Unable to convert: class clojure.core.Iteration to Object[]

Something IReduceInit but not Iterable:

user=> (vec
  (reify clojure.lang.IReduceInit
    (reduce [_ f start]
      (reduce f start (range 10)))))
RuntimeException Unable to convert: class user$reify__15 to Object[]

Proposal: Add PersistentVector.create(Iterable) and PersistentVector.create(IReduceInit) to efficiently create PVs from those. Add a number of cases to vec to support different cases.

Patch: clj-1546-3.patch

Screened by:



 Comments   
Comment by Timothy Baldridge [ 02/Oct/14 9:44 AM ]

Is there a reason the final case for (vec something) can't just be a call to (into [] coll)? It seems a bit odd to do (to-array) on anything thats not a java collection or Iterable, when we have IReduce.

Comment by Rich Hickey [ 02/Oct/14 10:02 AM ]

re: Tim - yes, this needs to support IReduce (and thereby educe) as well

Comment by Alex Miller [ 14/Oct/14 9:56 AM ]

Added new patch that handles Iterable and IReduceInit in vec. It also makes calling with a vector much faster due to the first check. into is still faster for chunked seqs (due to special InternalReduce handling of chunking).

It would be possible to move more of the variant checking into LazilyPersistentVector or PersistentVector so it could be used in more contexts. I'm not sure how much to do with that.

It would also be possible to instead lean on reduce more from the Java side if there was a Java version of reduce (as defined in mikera's branch for http://dev.clojure.org/jira/browse/CLJ-1192 at https://github.com/mikera/clojure/compare/clj-1192-vec-performance. Something like that is the only way I can see of leveraging that same InternalReduce logic that makes into faster than vec.





[CLJ-1545] Add unchecked-divide, unchecked-remainder Created: 02/Oct/14  Updated: 06/Oct/14

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

Type: Enhancement Priority: Major
Reporter: Andy Fingerhut Assignee: Colin Taylor
Resolution: Unresolved Votes: 0
Labels: math, newbie

Attachments: File CLJ-1545-2.diff     File CLJ-1545.diff    
Patch: Code and Test
Approval: Triaged

 Description   

This appears like it might be an oversight that these are missing. There are unchecked-divide-int and unchecked-remainder-int functions, but not equivalents for longs, even though there are equivalents for longs for every other unchecked operation. The JVM has bytecodes for long division and remainder.

The Clojure documentation in the section "Support for Java Primitives" on page http://clojure.org/java_interop has links for unchecked-divide and unchecked-remainder, but since they don't exist in Clojure, the API link targets don't exist.

It seems like a good idea to either add these to Clojure, or remove them from the documentation.



 Comments   
Comment by Colin Taylor [ 03/Oct/14 6:17 PM ]

Having a go at this.

Comment by Colin Taylor [ 04/Oct/14 6:02 AM ]
  • Added tests for unchecked-divide-int and unchecked-remainder-int too.
  • Unchecked fns only support binary arity and will throw CompilerException(ArityException)s where checked will not.
  • Is there any value to (int,long) (long,int) overrides for java interop cases e.g. using java collections from Clojure in high perf code?
Comment by Alex Miller [ 04/Oct/14 9:13 AM ]

Thanks for taking this on Colin!

1) When I apply the patch (git apply CLJ-1545.diff), I get a bunch of whitespace errors which will need to be cleaned up but also the patch seems to fail to apply at all on the changes in test/clojure/test_clojure/numbers.clj. It looks like perhaps the diff is just not the right diff format. You might want to check out the instructions at http://dev.clojure.org/display/community/Developing+Patches about using git format-patch.

2) If you could put a more useful git commit message, that would be helpful. Something like "CLJ-1545 Adds missing unchecked-divide and unchecked-remainder for primitive longs."

Thanks!

Comment by Colin Taylor [ 04/Oct/14 4:47 PM ]

Uggh, sorry Alex.

New patch with better commit message.

Comment by Alex Miller [ 04/Oct/14 7:24 PM ]

The patch format looks better. Pulling out farther to the ticket itself, afaict Clojure will already use the right byteocode for checked or unchecked so this may not even be needed?

If I compile (without the patch):

(defn foo-div ^long [^long a ^long b]
  (quot a b))

then the bytecode for that fn is:

public final long invokePrim(long, long);
    Code:
       0: lload_1
       1: lload_3
       2: ldiv
       3: lreturn

similarly, quot of two longs yields the same code but with lrem. I think patch has no net effect on the resulting bytecode?

Comment by Andy Fingerhut [ 04/Oct/14 7:42 PM ]

Alex, did you do the testing in your previous comment with *unchecked-math* true or false? If false, then I would think that if CLJ-1254 is judged a bug, then the behavior you saw is a bug, too, that misses the same corner case.

Comment by Alex Miller [ 04/Oct/14 10:19 PM ]

The current results are the same with either unchecked-math setting, but I see your point.

Refreshing my memory of the (/ Long/MIN_VALUE -1) case, I think you're right. The (new) unchecked-divide / remainder should do what the current (checked) forms do and the regular division and remainder cases should be making the overflow check. I think CLJ-1254 should cover the quot changes.

Comment by Colin Taylor [ 04/Oct/14 10:19 PM ]

user=> (dotimes [_ 6] (time (dotimes [_ 50000000] (unchecked-divide 4 (System/currentTimeMillis)))))
"Elapsed time: 1806.942 msecs"
"Elapsed time: 1808.747 msecs"
"Elapsed time: 1865.074 msecs"
"Elapsed time: 1802.777 msecs"
"Elapsed time: 1839.468 msecs"
"Elapsed time: 1830.61 msecs"
nil
user=> (dotimes [_ 6] (time (dotimes [_ 50000000] (/ 4 (System/currentTimeMillis)))))
"Elapsed time: 5003.598 msecs"
"Elapsed time: 4998.182 msecs"
"Elapsed time: 4941.237 msecs"
"Elapsed time: 5036.517 msecs"
"Elapsed time: 4965.867 msecs"
"Elapsed time: 4982.693 msecs"





[CLJ-1544] AOT bug involving protocols Created: 01/Oct/14  Updated: 01/Oct/14

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

Type: Defect Priority: Major
Reporter: Allen Rohner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: aot, protocols

Approval: Triaged

 Description   

See the example in this repo:

https://github.com/arohner/clj-aot-repro

Note that the test, `lein compile aot-repro.main aot-repro.main` seems like a pathological case, I've seen the same behavior in 'real' repos by just running `lein uberjar`. I haven't found the minimal testcase for 'normal' usage, but the errors I see are the same.

The bug seems to involve:

  • AOT
  • defining a protocol
  • getting the protocol-defining namespace loaded before compilation starts.





[CLJ-1543] Type tags on argument vector appear to help avoid reflection when used with defn, but not with def foo (fn ...) Created: 30/Sep/14  Updated: 02/Oct/14

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

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop, typehints


 Description   

I would have expected that both of the Java interop calls below would avoid reflection, but only the first involving f1 does.

Clojure 1.6.0
user=> (set! *warn-on-reflection* true)
true
user=> (defn f1 ^java.util.LinkedList [coll] (java.util.LinkedList. coll))
#'user/f1
user=> (def f2 (fn ^java.util.LinkedList [coll] (java.util.LinkedList. coll)))
#'user/f2
user=> (.size (f1 [2 3 4]))
3
user=> (.size (f2 [2 3 4]))
Reflection warning, NO_SOURCE_PATH:5:1 - reference to field size can't be resolved.
3

Not sure if this has anything to do with CLJ-1232, but was discovered when testing variants of that issue.



 Comments   
Comment by Andy Fingerhut [ 30/Sep/14 9:08 PM ]

What a nice number for a ticket, 1543. The year Copernicus's most celebrated book was published: http://en.wikipedia.org/wiki/Nicolaus_Copernicus

Comment by Jozef Wagner [ 01/Oct/14 4:05 AM ]

Isn't type hinting of arg vector meant only for primitive type hints? AFAIK non-primitive type hints should be on a function name, everything else is non idiomatic.

Comment by Nicola Mometto [ 01/Oct/14 7:05 AM ]

This isn't an issue of arg vector hinting vs function name hinting.
The issue here is that return type hinting cannot be put on anonymous functions but only on defns as the :arglists will be added by defn on the Var's metadata.

This is one of the reasons why I'd like to have that information as a field on the fn rather than as metadata on the Var

Comment by Andy Fingerhut [ 01/Oct/14 10:55 AM ]

Jozef, you may be correct that non-primitive type hints on the argument vector are non idiomatic. Do you have any source for that I could read?

Comment by Tassilo Horn [ 02/Oct/14 12:19 AM ]

Only the version with hints on the argument vectors is documented at http://clojure.org/java_interop#Java Interop-Type Hints. However, in the case you have just one arity (or all arities return a value of the same type) the hint on the var name also works. But the two versions seem to have different semantics. Have a look at CLJ-1232.

Comment by Jozef Wagner [ 02/Oct/14 5:48 AM ]

Type hinting is a very intricate part of Clojure but you can almost always apply a 'place hint on a symbol' idiom. Type hinting on an arg vector must be done only in two cases:

  • primitive hints
  • different return classes for different arities

In the first case, compiler needs type hints when compiling fn* (see [1]), not later, thus you must specify them on arg vector.

Second case, which is the issue discussed here, must be used only when defining with defn. Compiler first looks for the tag in the metadata of a var, and if it does not find one, it has a special case in which it looks for a return class inside :arglist metadata. This is clearly a very special case [2] to handle situations where you have different return classes for different arities. Obviously, using def instead of defn won't create an :arglist metadata for you thus you see a reflection warning. Example:

user=> (def f2 (fn ^java.util.LinkedList [coll] (java.util.LinkedList. coll)))
#'user/f2
user=> (.size (f2 [2 3 4]))
Reflection warning, /tmp/form-init.clj:1:1 - reference to field size can't be resolved.
3
user=> (alter-meta! #'f2 assoc :arglists '(^java.util.LinkedList [coll]))
{:ns #<Namespace user>, :name f2, :file "/tmp/form-init.clj", :column 1, :line 1, :arglists ([coll])}
user=> (.size (f2 [2 3 4]))
3

BTW CLJ-1491 has a discussion slightly relevant to this topic.

[1] https://github.com/clojure/clojure/blob/03cd9d159a2c49a21d464102bb6d6061488b4ea2/src/jvm/clojure/lang/Compiler.java#L5134
[2] https://github.com/clojure/clojure/blob/03cd9d159a2c49a21d464102bb6d6061488b4ea2/src/jvm/clojure/lang/Compiler.java#L3572

Comment by Jozef Wagner [ 02/Oct/14 7:15 AM ]

Andy, I've found sources that speak against my recommendations See CLJ-811 and [1].

[1] https://groups.google.com/d/msg/clojure/b005zQCPxOQ/6G0AlWKKKa0J





[CLJ-1542] Docstring for deliver should describe its return value Created: 30/Sep/14  Updated: 30/Sep/14

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

Type: Enhancement Priority: Minor
Reporter: Gary Fredericks Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring

Approval: Triaged

 Description   

It is presumably useful when delivering a promise to know if the delivery was successful or not (where it might be unsuccessful if it was already delivered, perhaps on another thread).

The deliver function seems to currently communicate this by returning a truthy value (the promise itself) on success and a falsy value (nil) on failure. If this is intentional, the docstring should say so so that users can comfortably rely on it.

In CLJ-1038 Rich elected for the docstring to not describe the return value; I'm not sure if that was a reluctance to fully specify the return value (promise vs nil) even if partially describing it (truthy vs falsy) would be okay.






[CLJ-1541] System/getProperty "user.dir" gives wrong output Created: 30/Sep/14  Updated: 30/Sep/14  Resolved: 30/Sep/14

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

Type: Defect Priority: Major
Reporter: Khuram U. Khalid Assignee: Unassigned
Resolution: Not Reproducible Votes: 0
Labels: System/getProperty, bug, user.dir
Environment:

Windows 8.1 - Java version 1.8.0 - Clojure 1.6.0 - IntelliJ IDEA - Maven 3.0.5



 Description   

;; For example if current project is in C:\Projects\My Project
;; Following gives...

(ns my.project.com.core)
(defn -main [] (println (System/getProperty "user.dir"))

;;=> C:\Projects\My Project\src\main\my\project\com

;; While when same Clojure code is run from a Java project gives...
public static void main(String[] args) { my.project.com.core.main(); }
;;=> C:\Projects\My Project

Expected same behavior and hence correct output in Clojure.



 Comments   
Comment by Alex Miller [ 30/Sep/14 9:18 AM ]

I tried this on a simple project at the command line and saw no difference in behavior between Java and Clojure. Clojure does not modify the user.dir system property and you are calling directly into the System class, just like Java does, so the only difference is in how your environment is configured when running in these two contexts.

It's possible that your environment (IDEA) is configuring Java and Clojure projects differently, but you should ask on the mailing list or issue tracker for the tool.





[CLJ-1540] Make main function to run when using on the fly compilation, not just ahead-of-time compilation Created: 29/Sep/14  Updated: 29/Sep/14  Resolved: 29/Sep/14

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

Type: Enhancement Priority: Minor
Reporter: macdevign Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

From this doc
http://clojure.org/compilation

Clojure compiles all code you load on-the-fly into JVM bytecode, but sometimes it is advantageous to compile ahead-of-time (AOT). Some reasons to use AOT compilation are:
To generate named classes for use by Java among the reason

and only named classes can run off main function.

So if not using AOT, the main method will not be executed.

Hence the following main can only run in AOT using named classes.
(defn -main
(println "runme")))

Will that be possible to run the main function using on the fly compilation ?

Basically, it should work similarly to Java. If the clojure file has a main function then it should run the file if user select it to run (eg in IDE) regardless of mode of compilation.

For example, in IntelliJ ide, a clojure file (eg hello.clj) has the following code

(defn testme[]
(println "hello"))

(defn -main
(println "runme")
(testme))

if user choose "Run hello.clj" from Intellij, then it should execute the main function.

Will be great if this feature is consider in next release of clojure

thank



 Comments   
Comment by Alex Miller [ 29/Sep/14 12:49 PM ]

There are (already) a variety of ways to start a Clojure script or program and I believe what you request (and more) is already possible.

See: http://clojure.org/repl_and_main

An example command-line for your hello.clj example would be:

java -cp clojure.jar clojure.main -i hello.clj -e "(-main)"

but if you are only running this as a script you could embed the code to run your app at the end of the hello.clj script file and do:

java -cp clojure.jar clojure.main hello.clj
Comment by Kevin Downey [ 29/Sep/14 3:44 PM ]

checkout the `-m` option





[CLJ-1539] Allow Records to be imported "Normally" Created: 28/Sep/14  Updated: 28/Sep/14  Resolved: 28/Sep/14

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

Type: Enhancement Priority: Minor
Reporter: David Williams Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

I know about records, and how they are compiled to Java classes, etc. The thing is, the import of a record type has an undocumented quirk, the need to turn dashes into underscore

(:require [my-fancy.namespace])
(:import [my_fancy.namespace MyRecord])

Granted this is trivial, but I just spent an hour or two tracking this down after some initial unsuccessful attempts to import a record between namespaces. IMHO this is not user friendly and could be smoothed out.



 Comments   
Comment by Alex Miller [ 28/Sep/14 10:42 PM ]

There are no plans to change this. Typically you don't need to import the record class at all, just require the ns and use the > and map> constructor functions. When you do import the class, you are doing so as a Java class, so it follows java class import rules.





[CLJ-1538] Set literal duplicate check occurs too early. Created: 27/Sep/14  Updated: 09/Oct/14

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

Type: Defect Priority: Minor
Reporter: Chhi'mèd Künzang Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reader


 Description   

I cannot use literal syntax to create a set/map with unique members/keys if the elements are generated with an identical form. Examples of such legal forms: (rand), (read), (clojure.core.async/<!!), etc. I will use (rand) in these examples.

user=> #{(rand) (rand)}
IllegalArgumentException Duplicate key: (rand)  clojure.lang.PersistentHashSet.createWithCheck (PersistentHashSet.java:68)

user=> {(rand) 1, (rand) 2}

IllegalArgumentException Duplicate key: (rand)  clojure.lang.PersistentArrayMap.createWithCheck (PersistentArrayMap.java:70)

It appears that the input is being checked for duplicates before the arguments to the collection constructors are evaluated. However, this doesn't prevent the need to run the check again later.

Note that duplicates are still (correctly) detected, after evaluation, even if duplicates do not appear as literals in the source:

user=> #{(+ 1 1) 2}

IllegalArgumentException Duplicate key: 2  clojure.lang.PersistentHashSet.createWithCheck (PersistentHashSet.java:56)
user=> {(+ 1 1) :a, 2 :b}

IllegalArgumentException Duplicate key: 2  clojure.lang.PersistentArrayMap.createWithCheck (PersistentArrayMap.java:70)

The first duplicate check therefore seems to be both redundant and incorrect.

Note that this eager duplicate-checking seems to have higher precedence even than the syntax-quote reader macro.

user=> `#{~(rand) ~(rand)}

IllegalArgumentException Duplicate key: (clojure.core/unquote (rand))  clojure.lang.PersistentHashSet.createWithCheck (PersistentHashSet.java:68)

user=> `{~(rand) 1, ~(rand) 2}

IllegalArgumentException Duplicate key: (clojure.core/unquote (rand))  clojure.lang.PersistentArrayMap.createWithCheck (PersistentArrayMap.java:70)

This is odd – since syntax-quote should not realize a collection at all at read time:

For Lists/Vectors/Sets/Maps, syntax-quote establishes a template of the corresponding data structure. Within the template, unqualified forms behave as if recursively syntax-quoted, but forms can be exempted from such recursive quoting by qualifying them with unquote or unquote-splicing, in which case they will be treated as expressions and be replaced in the template by their value, or sequence of values, respectively. (http://clojure.org/reader)

Definitions aside, based on the apparent expansion of syntax-quote, I would expect the previous to have worked correctly.

If I fake the expected macroexpansion by manually substituting the desired inputs, I get the expected results:

user=> '`#{~:a ~:b}
(clojure.core/apply clojure.core/hash-set (clojure.core/seq (clojure.core/concat (clojure.core/list :b) (clojure.core/list :a))))
user=> (clojure.core/apply clojure.core/hash-set (clojure.core/seq (clojure.core/concat (clojure.core/list (rand)) (clojure.core/list (rand)))))
#{0.27341896385866227 0.3051522362827035}
user=> '`{~:a 1, ~:b 2}
(clojure.core/apply clojure.core/hash-map (clojure.core/seq (clojure.core/concat (clojure.core/list :a) (clojure.core/list 1) (clojure.core/list :b) (clojure.core/list 2))))
user=> (clojure.core/apply clojure.core/hash-map (clojure.core/seq (clojure.core/concat (clojure.core/list (rand)) (clojure.core/list 1) (clojure.core/list (rand)) (clojure.core/list 2))))
{0.12476921225204185 2, 0.5807961046096718 1}

It seems to me that there is a superfluous duplicate check being run before the set/map reader macros evaluate their arguments. This check should seemingly be removed. Even if the check did not catch some false-positive duplicates (as it does), it would be unnecessary since the apparent second post-evaluation check would catch all true duplicates.

All that said, it's unclear that this check should happen at all. If I try to create sets/map with duplicate members/keys, I don't get an error. The duplicates are silently removed or superseded.

user=> (set (list 1 1))
#{1}
user=> (hash-map 1 2 1 3)
{1 3}

It seems it would be most consistent for literals constructed by the reader syntax to do the same.

I can see the argument that a literal representation is not a 'request to construct' but rather an attempt to simulate the printed representation of a literal data object. From that perspective, disallowing 'illegal' printed representations seems reasonable. Unfortunately, the possibility of evaluated forms inside literal vectors, sets, and maps (since lists are evaluated at read time) already breaks this theory. That is, the printed representation of such collections is not an accurately readable form, so read-time duplicate checking still cannot prevent seeming inconsistencies in print/read representations:

user=> '#{(+ 1 1) 2}
#{(+ 1 1) 2}
user=> #{(+ 1 1) 2}

IllegalArgumentException Duplicate key: 2  clojure.lang.PersistentHashSet.createWithCheck (PersistentHashSet.java:56)

Given that the problem cannot be completely avoided at all, it seems simplest and most consistent to treat reader literal constructors like their run-time counterparts, as syntax quote would in the absence of the spurious duplicate check.



 Comments   
Comment by Alex Miller [ 09/Oct/14 8:04 AM ]

Also see CLJ-1555

Comment by Nicola Mometto [ 09/Oct/14 8:09 AM ]

Potentially related: http://dev.clojure.org/jira/browse/CLJ-1425





[CLJ-1537] Audit IReduce usages for proper Reduced handling Created: 26/Sep/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

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

Type: Defect Priority: Major
Reporter: Timothy Baldridge Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File audit-ireduce.diff     File clj-1537-v2.diff     File clj-1537-v3.diff    
Patch: Code
Approval: Ok

 Description   

Rich asked that we make sure that all usages of IReduce properly handle Reduced semantics.

Approach: I did a "Find Usages" in InteliJ and updated usages of IReduce as needed.

Example: Before the patch:

user=> (transduce (take 1) conj (seq (subvec [1 2 3 4 5] 1)))
#<Reduced@13df2a8c: #<Reduced@1ebea008: #<Reduced@72d6b3ba: #<Reduced@1787f2a0: [2]>>>>

user=> (transduce (take 1) conj '(1 2 3 4))
#<Reduced@51bd8b5c: #<Reduced@7b50df34: #<Reduced@1b410b60: #<Reduced@2462cb01: [1]>>>>

Patch: clj-1537-v3.diff
Screened by: Alex Miller



 Comments   
Comment by Alex Miller [ 30/Sep/14 5:59 PM ]

Should be same as audit-ireduce.diff but w/o whitespace diffs.

Comment by Alex Miller [ 30/Sep/14 8:25 PM ]

Should these changes be deref'ing ret?

Also, can you add an example to the description (not sure if it needs to be a test) of where these are an issue?

Comment by Timothy Baldridge [ 03/Oct/14 10:18 AM ]

Following the pattern here: https://github.com/clojure/clojure/blob/master/src/clj/clojure/core/protocols.clj#L85 they should deref the reduced value.

Comment by Timothy Baldridge [ 03/Oct/14 10:29 AM ]

Failure examples from master, are added to the description.

Comment by Timothy Baldridge [ 03/Oct/14 2:23 PM ]

clj-1537-v2.diff didn't properly deref the reduced box. clj-1537-v3.diff does this now.





[CLJ-1536] Remove usage of sun.misc.Signal (which may not be available in Java 9) Created: 26/Sep/14  Updated: 26/Sep/14

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

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


 Description   

It looks like Java 9 will not continue to provide access to "internal" classes like sun.misc.Signal. Clojure currently uses this in the REPL to trap ctrl-c (SIGINT) and cancel current evaluation instead of process shutdown.

There is a page of alternatives here:
https://wiki.openjdk.java.net/display/JDK8/Java+Dependency+Analysis+Tool

But there is no suggested alternative for sun.misc.Signal and I'm not aware of a portable solution to it.






[CLJ-1535] Make boxed math warning suppressible Created: 26/Sep/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: math

Attachments: Text File clj-1535-2.patch     Text File clj-1535-3.patch     Text File clj-1535.patch     Text File silence-boxed-patch-10-01-2014.patch    
Patch: Code and Test
Approval: Ok

 Description   

Clojure 1.7.0-alpha2 included a new warning that will notify on use of boxed math when unchecked-math is set to true (CLJ-1325). Based on feedback, would like to make these warnings optional.

Approach: Revert (set! *unchecked-math* true) to prior behavior. Only emit warnings when (set! *unchecked-math* :warn-on-boxed).

Patch: clj-1535-3.patch

Screened by: Stuart Halloway



 Comments   
Comment by Michael Blume [ 01/Oct/14 7:45 PM ]

So I decided to take a shot at writing a patch for this. This is my first Clojure core patch, so I've probably messed up some formatting, but the implementation was pretty simple and the tests pass.

I introduced a variable, clojure.core/silence-boxed which defaults false and, when true, silences boxed math warnings. If the reverse is preferred (warn-boxed or similar) I can do that too.

Comment by Alex Miller [ 01/Oct/14 8:34 PM ]

Hi Michael, we have other plans for how this should be implemented, so will likely not use your patch. In the future, it's always good to check if the ticket is already assigned to someone before working on it.

Comment by Alex Miller [ 07/Oct/14 8:12 AM ]

Added clj-1535-3.patch, which is exactly the same diff as clj-1535-2.patch, but just squashes into a single commit.





[CLJ-1534] Adding condp-> and condp->> macros to core library Created: 24/Sep/14  Updated: 01/Oct/14

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

Type: Enhancement Priority: Major
Reporter: Kuldeep Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: enhancement, macro

Attachments: File condp-threading-macros-25sept2014.diff    
Patch: Code

 Description   

After introduction of cond-> and cond->> macros in 1.5. It makes sense to have condp-> and condp->> macros in the core library.

(condp-> {}
(complement :a) (assoc :a 1)
:a (assoc :b 2)) ;=> {:b 2, :a 1}

In the above example the result of each expr which was evaluated is being passed to the next predicate.



 Comments   
Comment by Andy Fingerhut [ 01/Oct/14 6:37 PM ]

Kuldeep, I cannot comment on whether this change is of interest to the Clojure developers, because I do not know.

I can say that the patch you have attached is not in the expected format. See the page below for instructions on creating a patch in the expected format:

http://dev.clojure.org/display/community/Developing+Patches





[CLJ-1533] Oddity in type tag usage for primInvoke Created: 24/Sep/14  Updated: 03/Oct/14

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

Type: Defect Priority: Minor
Reporter: Andy Fingerhut Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: ft, typehints

Attachments: Text File 0001-CLJ-1533-inject-original-var-form-meta-in-constructe.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Some odd behavior demonstrated in Clojure 1.6.0 REPL below. Why does the (Math/abs (f2 -3)) call issue a reflection warning? It seems like perhaps it should not, given the other examples.

user=> (clojure-version)
"1.6.0"
user=> (set! *warn-on-reflection* true)
true
user=> (defn ^{:tag 'long} f1 [x] (inc x))
#'user/f1
user=> (Math/abs (f1 -3))
2
user=> (defn ^{:tag 'long} f2 [^long x] (inc x))
#'user/f2
user=> (Math/abs (f2 -3))
Reflection warning, NO_SOURCE_PATH:6:1 - call to static method abs on java.lang.Math can't be resolved (argument types: java.lang.Object).
2
user=> (defn ^{:tag 'long} f3 ^long [^long x] (inc x))
#'user/f3
user=> (Math/abs (f3 -3))
2

Cause: invokePrim path does not take into account var or form meta

Approach: apply var and form meta to invokePrim expression

Patch: 0001-CLJ-1533-inject-original-var-form-meta-in-constructe.patch

Screened by: Alex Miller



 Comments   
Comment by Nicola Mometto [ 25/Sep/14 9:47 AM ]

The issue is similar to http://dev.clojure.org/jira/browse/CLJ-1491

Comment by Nicola Mometto [ 25/Sep/14 9:58 AM ]

The root cause was also almost the same, the proposed patch is a superset of the one proposed for CLJ-1491

Comment by Alex Miller [ 25/Sep/14 10:09 AM ]

Can we include 1491 cases in this ticket and mark 1491 a duplicate?

Comment by Alex Miller [ 25/Sep/14 10:09 AM ]

Also needs tests in the patch.

Comment by Nicola Mometto [ 25/Sep/14 10:23 AM ]

Updated the patch with testcases for both issues, I agree that CLJ-1491 should be closed as duplicate





[CLJ-1532] pr-str captures stdout from printing side-effects of lazily evaluated expressions. Created: 23/Sep/14  Updated: 23/Sep/14

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

Type: Defect Priority: Minor
Reporter: Silas Davis Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: print
Environment:

Linux



 Description   

Because clojure.core/pr-str uses with-out-str to capture the output of pr (and pr cannot be parsed a writable thing - just uses out).

If you pr-str the result of something lazy you can get side-effects written to stdout with println interspersed with the output. For example in my case I was extracting benchmarks from the library criterium and trying to print the data structure to the file. The solution would be to provide an overload of pr/pr-str that takes a writer. I note that pr-on provides some of the functionality but it is private.

This is an ugly bug when you're trying to persist program output in EDN, because the randomly interspersed stdout messages make it invalid for read-string. We shouldn't need our functions to be pure for pr-str to work as expected.

I've omitted a patch because although I think a fix is straight-forward I'm not sure quite where it should go (e.g. make pr-on public, change pr, change pr-str)






[CLJ-1531] inc always warns when *unchecked-math* is set Created: 23/Sep/14  Updated: 23/Sep/14  Resolved: 23/Sep/14

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

Type: Defect Priority: Major
Reporter: Pierre-Yves Ritschard Assignee: Pierre-Yves Ritschard
Resolution: Not Reproducible Votes: 0
Labels: bug, errormsgs


 Description   

While testing 1.7-alpha2 I stumbled this (affects clojure.data.codec amongst others). inc inlines a call to clojure.lang.Numbers's inc method which according to the rules of CLJ-1325 will warn.

I can't find a way around it for now, except maybe having a primitive-inc and primitive-dec java method which would be inlined in that case.

Happy to work on a patch but would prefer discussing it first.



 Comments   
Comment by Nicola Mometto [ 23/Sep/14 12:42 PM ]

I cannot reproduce this:

Clojure 1.7.0-master-SNAPSHOT
user=> (set! *unchecked-math* true)
true
user=> (inc 1)
2

Looking at Numbers.java I see both unchecked_inc and inc have long/double taking methods.

Comment by Pierre-Yves Ritschard [ 23/Sep/14 1:49 PM ]

you're right, i must have been confused.

Comment by Pierre-Yves Ritschard [ 23/Sep/14 1:50 PM ]

not a bug





[CLJ-1530] Make foo/bar/baz unreadable Created: 22/Sep/14  Updated: 28/Oct/14

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

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

Attachments: Text File 0001-fix-LispReader-and-EdnReader-so-that-foo-bar-baz-is-.patch    
Patch: Code and Test

 Description   

Currently keywords and symbols containing more than one slash are disallowed by the spec, but allowed by the readers.
This trivial patch makes them unreadable by the readers too.

Pre:

user=> :foo/bar/baz
:foo/bar/baz

Post:

user=> :foo/bar/baz
RuntimeException Invalid token: :foo/bar/baz  clojure.lang.Util.runtimeException (Util.java:221)


 Comments   
Comment by Andy Fingerhut [ 22/Sep/14 12:14 PM ]

Perhaps overlap with CLJ-1527 ?

Comment by Thomas Engelschmidt [ 28/Oct/14 4:36 AM ]

Please notice that keywords with more than one slash has a different hashcode across clojure version 1.5 and 1.6

This creates a problem when using a datomic version that works with clojure 1.5 under clojure 1.6 and the schema have one or more keys with more than one slash.





[CLJ-1529] Significantly improve compile time by reducing calls to Class.forName Created: 21/Sep/14  Updated: 27/Oct/14

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

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

Attachments: File class-for-name.diff     File clj-1529-no-cache.diff     PNG File clj-1529.png     File clj-1529-with-cache.diff     Text File maybe-class-cache-2.patch     Text File maybe-class-cache.patch    
Patch: Code
Approval: Vetted

 Description   

Compilation speed has been a real problem for a number of my projects, especially Aleph [1], which in 1.6 takes 18 seconds to load. Recently I realized that Class.forName is being called repeatedly on symbols which are lexically bound. Hits on Class.forName are cached, but misses are allowed to go through each time, which translates into tens of thousands of calls after calling `(use 'aleph.http)`.

This patch improves compilation time from 18 seconds to 7 seconds. The gain is exaggerated by the number of macros I use, but I would expect at least 50% improvements across a wide variety of codebases.

This patch does introduce a slight semantic change by privileging lexical scope over classnames. Consider this code:

(let [String "foo"]
(. String substring 0 1))

Previously, this would be treated as a static call to 'java.lang.String', but with the patch would be treated as a call to the lexical variable 'String'. Since the new semantic is what I (and I think everyone else) would have expected in the first place, it's probably very likely that no one is shadowing classes with their variable names, since someone would have complained about this. If anyone feels this is at all risky, however, I'm happy to discuss it further.

[1] https://github.com/ztellman/aleph



 Comments   
Comment by Ghadi Shayban [ 21/Sep/14 4:30 PM ]

One of our larger projects (not macro-laden) just went from 36 seconds to 23 seconds to start with this patch.

Comment by Ramsey Nasser [ 03/Oct/14 12:34 PM ]

I ported this patch to Clojure-CLR for the Unity integration project and we have seen significant speedups as well. I too agree that this is the behavior I expect as a user.

Comment by Alex Miller [ 06/Oct/14 12:19 PM ]

I ran this on a variety of open-source projects. I didn't find that it produced any unexpected behavior or test errors. Most projects were about 10% faster to run equivalent of "lein test" with a few as high as 35% faster.

Comment by Alex Miller [ 07/Oct/14 12:52 PM ]

We're interested in comparing this and the class caching in fastload branch to get something in for 1.7. Next step is to extract a patch of the stuff in fastload so we can compare them better.

Comment by Alex Miller [ 07/Oct/14 4:06 PM ]

Add maybe class cache patch from fastload branch

Comment by Alex Miller [ 08/Oct/14 8:57 AM ]

Times below to run "time lein test" on a variety of projects with columns:

  • master = current 1.7.0 master
  • maybe-cache = maybe-class-cache.patch extracted from Rich's fastload branch
  • class-for-name = class-for-name.diff from Zach
  • % maybe-cache = % improvement for maybe-cache over master
  • % class-for-name = % improvement for class-for-name patch over master (sorted desc)

project,master,maybe-cache,class-for-name,% maybe-cache,% class-for-name
aleph,25.605,16.572,14.460,35.278,43.527
riemann,40.550,27.656,24.734,31.798,39.004
lamina,37.247,30.072,29.045,19.263,22.021
trapperkeeper,11.979,11.158,10.3,6.854,14.016
plumbing,73.777,68.388,66.922,7.304,9.292
cheshire,5.583,5.089,5.086,8.848,8.902
tools.analyzer,5.411,5.289,5.023,2.255,7.171
core.async,19.161,18.090,17.942,5.589,6.362
tools.reader,4.686,4.435,4.401,5.356,6.082
clara-rules,43.964,42.140,41.542,4.149,5.509
core.typed,158.885,154.954,151.445,2.474,4.683
instaparse,9.286,8.922,8.859,3.920,4.598
schema,45.3,43.914,43.498,3.060,3.978
mandoline,76.295,74.831,74.425,1.919,2.451

The summary is that both patches improve times on all projects. In most cases, the improvement from either is <10% but the first few projects have greater improvements. The class-for-name patch has a bigger improvement in all projects than the maybe-cache patch (but maybe-cache has no change in semantics).

Comment by Nicola Mometto [ 08/Oct/14 9:03 AM ]

Are the two patches mutually exclusive?

Comment by Alex Miller [ 08/Oct/14 9:35 AM ]

They are non-over-lapping. I have not considered whether they could both be applied or whether that makes any sense.

Comment by Alex Miller [ 08/Oct/14 9:53 AM ]

The two patches both essentially cut off the same hot code path, just at different points (class-for-name is earlier), so applying them both effectively should give you about the performance of class-for-name.

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

Added a picture of the data for easier consumption.

Comment by Deepak Giridharagopal [ 10/Oct/14 4:35 PM ]

One of our bigger projects saw a reduction of startup time of 16% with class-for-name, 14% with maybe-cache, and a whopping 23% with both patches applied. This was actually starting up the program, as opposed to running "lein test", FWIW.

Maybe it's worth re-running the benchmarks with a "both-patches" variant?

Comment by Alex Miller [ 10/Oct/14 5:28 PM ]

Hey Deepak, I did actually run some of them with both patches and saw times similar to class-for-name.

Were your times consistent across restarts? The times in the data above are the best of 3 trials for every data point (although they were relatively consistent).

Comment by Deepak Giridharagopal [ 10/Oct/14 6:08 PM ]

Hi Alex, the tests I ran did 20-iteration loops, and I took the mean (though it was pretty consistent between restarts). I can redo stuff and upload the raw data for you if that will help.

Comment by Deepak Giridharagopal [ 10/Oct/14 6:43 PM ]

So repeating the experiment several times does in fact behave as you suspected...apologies for my previous LOLDATA.

Comment by Alex Miller [ 24/Oct/14 3:01 PM ]

maybe-class-cache-2.patch removes some debugging stuff

Comment by Alex Miller [ 27/Oct/14 4:41 PM ]

I've done more testing and made mods to both patches and moved them closer together.

On the maybe-class-cache patch (new version = clj-1529-with-cache.diff):
1) I found that adding a final else branch that returned null was an improvement - this avoids caching things that will never hit in the future (Cons, PersistentList, Symbols with namespaces, etc). That's both a perf improvement (avoids hashing those things) and a memory improvement.
2) The tagToClass improvement from Zach's patch is orthogonal and also valid here so I added it.
3) I added Zach's check, but moved the placement lower so that it doesn't alter semantics. It helps avoid caching locals that aren't classes.

On the class-for-name patch (new version = clj-1529-no-cache.diff):
1) Same change as #3 above - moved check lower to avoid semantics change.

With these changes, both patches have tagToClass and local checks, neither patch changes semantics, and the only difference is whether to keep or remove the cache.

aleph timings (for "lein test"):

  • 1.7 master = 25.415 s
  • 1.7 + clj-1529-with-cache.diff = 14.329 s
  • 1.7 + clj-1529-no-cache.diff = 14.808 s

lamina timings (for "lein test"):

  • 1.7 master = 37.340 s
  • 1.7 + clj-1529-with-cache.diff = 28.680 s
  • 1.7 + clj-1529-no-cache.diff = 28.759 s

The cache helps slightly in both cases, but it does not seem worth adding the new dynamic var and unbounded memory use inherent in the cache.





[CLJ-1528] clojure.test/inc-report-counter is not thread safe Created: 19/Sep/14  Updated: 22/Sep/14

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

Type: Defect Priority: Minor
Reporter: Alexander Redington Assignee: Alexander Redington
Resolution: Unresolved Votes: 0
Labels: test
Environment:

OS X, Clojure 1.7, Macbook pro


Attachments: File fix-CLJ-1528.diff    
Patch: Code
Approval: Triaged

 Description   

clojute.test/inc-report-counter, as implemented at https://github.com/clojure/clojure/blob/919a7100ddf327d73bc2d50d9ee1411d4a0e8921/src/clj/clojure/test.clj#L313, is not thread safe.

The commute operation described combines dereferencing the report-counters ref and operating on the previous state of the ref, leading to race conditions during concurrent access.

Specifically, the report-counters ref is dereferenced on 320, instead of the commute function operating entirely as a function of its inputs.



 Comments   
Comment by Alexander Redington [ 19/Sep/14 10:58 AM ]

Fixes 1528





[CLJ-1527] Harmonize accepted / documented symbol and keyword syntax over various readers Created: 18/Sep/14  Updated: 19/Oct/14

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

Type: Defect Priority: Minor
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: reader

Approval: Triaged

 Description   

Documentation Issues

http://clojure.org/reader#The%20Reader--Reader%20forms is ambigous on whether foo/bar/baz is allowed. Also, it doesn't mention the tick ' as a valid constituent character.
The EDN spec also currently omits ', ticket here: https://github.com/edn-format/edn/issues/67

Implementation Issues

clojure.core/read, as well as clojure.edn/read accept symbols like foo/bar/baz, even though they should be rejected.

References

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



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

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

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

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





[CLJ-1526] clojure.core/> inconsistent behavior wrt to documentation. Created: 17/Sep/14  Updated: 22/Sep/14

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

Type: Enhancement Priority: Minor
Reporter: Phillip Lord Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: math


 Description   

The > function is inconsistent wrt to their behaviour for 0 arity.

user> (doc >)
-------------------------
clojure.core/>
([x] [x y] [x y & more])
  Returns non-nil if nums are in monotonically decreasing order,
  otherwise false.
nil
user> (> 3 2)
true
user> (> 3)
true
user> (>)
ArityException Wrong number of args (0) passed to: core/>  clojure.lang.AFn.throwArity (AFn.java:429)

This is mostly likely to become problematic when using > via apply where

(or (= 0 (count l))
    (apply > l))

It seems that the documentation should be updated, 0-arg case should return true, or the 1-arg case should also throw an exception.

This affects the other comparators also.



 Comments   
Comment by Robert Tweed [ 17/Sep/14 9:48 AM ]

As per my original post on this (here: https://groups.google.com/d/msg/clojure/8zkpO9FBN64/u2LAQsR93IgJ), while the question of whether an empty set has monotonic order perhaps has more than one answer in theory, from a purely pragmatic engineering perspective, it makes the most sense to evaluate to true here.

This /should/ not be a breaking change. Therefore it is fairly safe to introduce into a minor revision. It's a also a trivial fix. But it is possible (though highly unlikely) that someone could have code that depends on the exception being raised at runtime (as it does now) to handle empty lists in some special way. Such code is horrible and ought to be rewritten, so should not be seen as justification for retaining the current behaviour, which limits the general usefulness of these functions and may be responsible for subtle bugs in existing production code.

However such a change should probably not be backported to existing 1.6.x branches, just to be 100% safe, since it is not a security issue. My suggestion therefore would be to add a note to the docs in existing maintenance branches (any future 1.6.x) and evaluate to true in future versions (1.7+).





[CLJ-1525] bean function returns mutable maps Created: 16/Sep/14  Updated: 22/Sep/14  Resolved: 22/Sep/14

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

Type: Defect Priority: Major
Reporter: Simone Mosciatti Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None
Environment:

Linux



 Description   

Please take a look at this snippet.

user> (import 'java.util.Date)
java.util.Date
user> (def now (Date.))
#'user/now
user> now
#inst "2014-09-17T03:14:13.821-00:00"
user> (def bean-map (bean now))
#'user/bean-map
user> bean-map
{:day 3, :date 17, :time 1410923653821, :month 8, :seconds 13, :year 114, :class java.util.Date, :timezoneOffset -120, :hours 5, :minutes 14}
user> (.setMonth now 1)
nil
user> bean-map
{:day 1, :date 17, :time 1392610453821, :month 1, :seconds 13, :year 114, :class java.util.Date, :timezoneOffset -60, :hours 5, :minutes 14}

The same snippet here. https://gist.github.com/siscia/032bff669bbc6fb0fe57



 Comments   
Comment by Jozef Wagner [ 17/Sep/14 1:32 AM ]

It works as expected. bean fn returns a clojuresque abstraction on top of live bean. map-like abstraction returned from bean is intended to be 'mutable' in sense that it always return the latest value. Otherwise it is read only.

Comment by Simone Mosciatti [ 17/Sep/14 1:42 AM ]

Hi,

sorry, the documentation didn't mention the "mutable" part so I was expecting an immutable map as always.

Sorry, about that.

Greets

Comment by Alex Miller [ 22/Sep/14 9:38 AM ]

this is the expected behavior





[CLJ-1524] SeqIterator constructor change broke binary compatibility in 1.7.0-alpha2 Created: 09/Sep/14  Updated: 09/Sep/14  Resolved: 09/Sep/14

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

Type: Defect Priority: Critical
Reporter: Alex Miller Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

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

 Description   

Running code AOT-compiled against Clojure 1.6.0 (or older) with 1.7.0-alpha2 runtime will encounter this error with SeqIterator:

CompilerException java.lang.NoSuchMethodError: clojure.lang.SeqIterator.<init>(Lclojure/lang/ISeq;)V, compiling:(form-init5913779045640355531.clj:1:11)

Cause: This is due to a type change in the constructor of SeqIterator from ISeq to Object (commit: https://github.com/clojure/clojure/commit/43cc1854508d655e58e377f84836ba128971f90c ).

Proposed: Add the ISeq constructor back so that calls into that constructor retain backwards binary compatibility.

Patch: clj-1524.diff

Screened by:

More: From Datomic mailing list - https://groups.google.com/forum/#!topic/datomic/KZqhY6hUHz0



 Comments   
Comment by Alex Miller [ 09/Sep/14 11:06 AM ]

Patch not applied, but similar change applied directly here:

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





[CLJ-1523] Add 'doseq' like macro for transducers Created: 08/Sep/14  Updated: 09/Sep/14

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

Type: Enhancement Priority: Trivial
Reporter: Timothy Baldridge Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: File doreduced2.diff     File doreduced.diff    
Patch: Code and Test

 Description   

Doseq is currently a good way to execute a lazy sequence and perform side-effects. It would be nice to have a matching macro for transducers.

Approach: The included patch simply calls transduce with the provided xform, collection, and a reducing function that throws away the accumulated value at each step. The value from each reducing step is bound to the provided symbol. A shorter arity is provided for those cases when no xform is desired, but fast doseq-like semantics are still wanted.

Patch: doreduced2.diff



 Comments   
Comment by Jozef Wagner [ 09/Sep/14 4:19 AM ]

How about making xform parameter optional? And you have a typo in docstring example, doseq -> doreduced.

Comment by Timothy Baldridge [ 09/Sep/14 7:52 AM ]

Good point, fixed typeo, added other arity.





[CLJ-1522] Enhance multimethods metadata Created: 08/Sep/14  Updated: 09/Sep/14

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

Type: Enhancement Priority: Minor
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: metadata


 Description   

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

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

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



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

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





[CLJ-1521] A little improvement for parsing let expr Created: 07/Sep/14  Updated: 08/Sep/14

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

Type: Enhancement Priority: Trivial
Reporter: dennis zhuang Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: let, parser
Environment:

Mac OSX 10.9.4
java version "1.7.0_17"
Java(TM) SE Runtime Environment (build 1.7.0_17-b02)
Java HotSpot(TM) 64-Bit Server VM (build 23.7-b01, mixed mode)


Attachments: File improve_parse_let_expr.diff    
Patch: Code

 Description   

The recurMismatches vector in LetExpr parser as see in

https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L6062-6065

There is not necessary to add initialize value 'false' into it when it is not a loop expression.

We can rewrite it into:

if(isLoop)
			    {
				for (int i = 0; i < bindings.count()/2; i++)
				    {
				    recurMismatches = recurMismatches.cons(RT.F);
				    }				
			    }

It's a little improvement for parsing let expression.



 Comments   
Comment by Andy Fingerhut [ 07/Sep/14 11:16 AM ]

Dennis, you might want to clarify the description a little bit, if I understand this ticket correctly. The proposed change would be no change to the behavior of the compiler, except a small speed improvement during compilation?

Comment by dennis zhuang [ 08/Sep/14 2:36 AM ]

Yep,the patch doesn't change the behavior of the compiler.All test is fine.

The recurMismatches vector in LetExpr parser as see in

https://github.com/clojure/clojure/blob/master/src/jvm/clojure/lang/Compiler.java#L6062-6065

is only used when detecting type mismatch for loop special form,it's not necessary to be initialized for let special form.So i just added a if(isLoop) clause before initializing it.





[CLJ-1520] assoc-in with empty key path assoc-es to nil Created: 05/Sep/14  Updated: 05/Sep/14

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

Type: Defect Priority: Major
Reporter: Francis Avila Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   
(assoc-in {} [] 1) ;=> {nil 1}

This should probably throw an exception.

CLJ-373 has a patch (CLJ-373-nested-ops.patch) which fixes this (by throwing an exception on empty key paths), the related broken behavior of update-in, and documents empty key path behavior in get-in et al. I can pull just the assoc-in stuff out of that into a separate patch, but I am really hoping that all the issues in the patch addresses are resolved at once, I.e.:

(get-in {} [] :notfound) ;=> {} ; ok
(get-in {nil 1} [] :notfound) ;=> {nil 1} ; ok
(assoc-in {} [] 1) ;=> {nil 1} ; wat?
(assoc-in {nil 0} [] 1) ;=> {nil 1} ; wat?
(update-in {} [] identity) ;=> {nil nil} ; wat?
(update-in {nil 0} [] inc) ;=> {nil 1} ; wat?





[CLJ-1519] Added extra arity to clojure.core/ns-* fns Created: 04/Sep/14  Updated: 10/Sep/14

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

Type: Enhancement Priority: Major
Reporter: Alex Baranosky Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: enhancement, patch

Attachments: Text File new-ns-arity.patch    
Patch: Code and Test

 Description   

Hello,

Adds another arity where the "ns" parameter is set to a default value of *ns* in these fns:

ns-unmap, ns-resolve, ns-name, ns-map, ns-publics, ns-imports, ns-interns, ns-refers, ns-aliases, ns-unalias

I find I very often use ns-unalias and ns-unmap from the repl, and passing the *ns* arg gets a little tedious.






[CLJ-1518] Patch for removing transient thread owner check broke rrb-vector Created: 03/Sep/14  Updated: 04/Sep/14  Resolved: 04/Sep/14

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

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

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

 Description   

The patch for CLJ-1498 changed the field types inside the persistent data structures, which inadvertently broke core.rrb-vector, which relies on reusing some of those internals. It is not necessary to change the type to satisfy the patch, so we would like to rollback that aspect of the change to minimize breakage.



 Comments   
Comment by Alex Miller [ 03/Sep/14 2:05 PM ]

In the patch I rolled back the changes in the Persistent*.java from CLJ-1498 and re-applied. The only "real" changes after the rollback are in ensureEditable(). Tests were left of course.





[CLJ-1517] unrolled small collections Created: 01/Sep/14  Updated: 30/Oct/14

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

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

Attachments: File unrolled-collections-2.diff     File unrolled-collections.diff    
Patch: Code
Approval: Vetted

 Description   

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

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

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

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

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



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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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





[CLJ-1516] Throw an exception if def name contains a dot Created: 29/Aug/14  Updated: 29/Aug/14

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

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

Attachments: Text File 0001-throw-an-exception-on-def-names-containing-dots.patch    
Patch: Code
Approval: Triaged

 Description   

In this comment: http://dev.clojure.org/jira/browse/CLJ-1100?focusedCommentId=35510&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-35510 Rich said that Vars whose name contains a dot are not supported, but the current implementation allows their definition.
This patch makes `(def foo.bar)` throw a compile-time exception



 Comments   
Comment by Alex Miller [ 29/Aug/14 10:41 AM ]

I'm curious whether this breaks existing code in the wild.

Comment by Nicola Mometto [ 29/Aug/14 10:45 AM ]

I find this hard to believe given the current behaviour:

user=> (def a.b 1)
#'user/a.b
user=> a.b
CompilerException java.lang.ClassNotFoundException: a.b, compiling:(NO_SOURCE_PATH:0:0)

one would need to go out of his way and refer to the var namespace qualified everywhere to make it work

Comment by Nicola Mometto [ 29/Aug/14 11:03 AM ]

After a brief conversation on #clojure, I updated the patch to only throw on non-macro defs so that macros like clojure.core/.. and clojure.core.incubator/.?. will work fine





[CLJ-1515] Reify the result of range Created: 29/Aug/14  Updated: 24/Oct/14

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

Type: Enhancement Priority: Major
Reporter: Timothy Baldridge Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: File patch.diff     File range-patch3.diff    
Patch: Code and Test
Approval: Incomplete

 Description   

Currently range simply returns a lazy seq. If the return value of range were reified into a type (as it is in ClojureScript) we could optimize many functions on that resulting type. Some operations such as count and nth become O(1) in this case, while others such as reduce could receive a performance boost do to the reduced number of allocations.

Approach: this patch revives the unused (but previously existing) clojure.lang.Range class. This class acts as a lazy seq and implements several other appropriate interfaces such as Counted and Indexed. This type is implemented in Java since range is needed fairly on in core.clj before deftype is defined. The attached patch uses Numbers.* methods for all math due to the input types to range being unknown. The class also supplies a .iterator() method which allows for allocation free reducing over range.

Note: this code keeps backwards compatibility with the existing range code. This means some parts of the class (mostly relating to a step size of 0) are a bit more complex than desired, but these bits were needed to get all the tests to pass.

Note: this code does not preserve the chunked-seq nature of the original range. The fact that range used to return chunked seqs was not published in the doc strings and so it was removed to allow for simpler code in Range.java.

Performance:
(timings done at the repl run via java -jar)

(dotimes [x 100] (time (dotimes [x 1] (count (range (* 1024 1024))))))
master => 80-110ms
patch => 0.014ms


(dotimes [x 100] (time (dotimes [x 1] (reduce + (map inc (range (* 1024 1024)))))))
master => 76-87ms
patch => 340-360ms


(dotimes [x 100] (time (dotimes [x 1] (reduce + (map inc (map inc (range (* 1024 1024))))))))
master => 97-123ms
patch=> 490-577ms



(dotimes [x 100] (time (dotimes [x 1] (count (filter odd? (range (* 1024 1024)))))))
master => 87-104ms
patch => 370-330ms


(dotimes [x 100] (time (dotimes [x 1] (transduce (comp (map inc) (map inc)) + (range (* 1024 1024))))))
master=>76-116ms
patch => 44ms-59ms

Patch: range-patch3.diff



 Comments   
Comment by Alex Miller [ 29/Aug/14 3:19 PM ]

1) Not sure about losing chunked seqs - that would make older usage slower, which seems undesirable.
2) RangeIterator.next() needs to throw NoSuchElementException when walking off the end
3) I think Range should implement IReduce instead of relying on support for CollReduce via Iterable.
4) Should let _hash and _hasheq auto-initialize to 0 not set to -1. As is, I think _hasheq always would be -1?
5) _hash and _hasheq should be transient.
6) count could be cached (like hash and hasheq). Not sure if it's worth doing that but seems like a win any time it's called more than once.
7) Why the change in test/clojure/test_clojure/serialization.clj ?
8) Can you squash into a single commit?

Comment by Timothy Baldridge [ 29/Aug/14 3:40 PM ]

1) I agree, adding chunked seqs to this will dramatically increase complexity, are we sure we want this?
2) exception added
3) I can add IReduce, but it'll pretty much just duplicate the code in protocols.clj. If we're sure we want that I'll add it too.
4) fixed hash init values, defaults to -1 like ASeq
5) hash fields are now transient
6) at the cost of about 4 bytes we can cache the cost of a multiplication and an addition, doesn't seem worth it?
7) the tests in serialization.clj assert that the type of the collection roundtrips. This is no longer the case for range which starts as Range and ends as a list. The change I made converts range into a list so that it properly roundtrips. My assumption is that we shouldn't rely on all implementations of ISeq to properly roundtrip through EDN.
8) squashed.

Comment by Alex Miller [ 29/Aug/14 3:49 PM ]

6) might be useful if you're walking through it with nth, which hits count everytime, but doubt that's common
7) yep, reasonable

Comment by Andy Fingerhut [ 18/Sep/14 6:52 AM ]

I have already pointed out to Edipo in personal email the guidelines on what labels to use for Clojure JIRA tickets here: http://dev.clojure.org/display/community/Creating+Tickets

Comment by Timothy Baldridge [ 19/Sep/14 10:02 AM ]

New patch with IReduce directly on Range instead of relying on iterators

Comment by Alex Miller [ 01/Oct/14 2:00 PM ]

The new patch looks good. Could you do a test to determine the perf difference from walking the old chunked seq vs the new version? If the perf diff is negligible, I think we can leave as is.

Another idea: would it make sense to have a specialized RangeLong for the (very common) case where start, end, and step could all be primitive longs? Seems like this could help noticeably.

Comment by Timothy Baldridge [ 03/Oct/14 10:00 AM ]

Looks like chunked seqs do make lazy seq code about 5x faster in these tests.

Comment by Ghadi Shayban [ 03/Oct/14 10:22 AM ]

I think penalizing existing code possibly 5x is a hard cost to stomach. Is there another approach where a protocolized range can live outside of core? CLJ-993 has a patch that makes it a reducible source in clojure.core.reducers, but it's coll-reduce not IReduce, and doesn't contain an Iterator. Otherwise we might have to take the chunked seq challenge.

Alex: Re long/float. Old reified Ranged.java in clojure.lang blindly assumes ints, it would be nice to have a long vs. float version, though I believe the contract of reduce boxes numbers. (Unboxed math can be implemented very nicely as in Prismatic's Hiphip array manipulation library, which takes the long vs float specialization to the extreme with different namespaces)

Comment by Timothy Baldridge [ 03/Oct/14 10:38 AM ]

I don't think anyone is suggesting we push unboxed math all the way down through transducers. Instead, this patch contains a lot of calls to Numbers.*, if we were to assume that the start end and step params of range are all Longs, then we could remove all of these calls and only box when returning an Object (in .first) or when calling IFn.invoke (inside .reduce)

Comment by Alex Miller [ 03/Oct/14 10:46 AM ]

I agree that 5x slowdown is too much - I don't think we can give up chunked seqs if that's the penalty.

On the long case, I was suggesting what Tim is talking about, in the case of all longs, create a Range that stores long prims and does prim math, but still return boxed objects as necessary. I think the only case worth optimizing is all longs - the permutation of other options gets out of hand quickly.

Comment by Ghadi Shayban [ 03/Oct/14 11:00 AM ]

Tim, I'm not suggesting unboxed math, but the singular fast-path of all-Longs that you and Alex describe. I mistakenly lower-cased Long/Float.





[CLJ-1514] Use qualified class names for return type hints of standard Clojure functions Created: 28/Aug/14  Updated: 28/Aug/14

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

Type: Enhancement Priority: Minor
Reporter: Tassilo Horn Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: enhancement, interop, patch, typehints

Attachments: Text File 0001-Use-fully-qualified-class-names-for-return-type-hint.patch    
Patch: Code

 Description   

The attached patch converts all function return type hints to spell out the class name fully qualified. There are two reasons for doing this:

1. Simple names in return type hints cause the issue described in http://dev.clojure.org/jira/browse/CLJ-1232. That's usually not a problem with return type hints referring to java.lang-classes because those are always imported. However, using `ns-unmap` you can remove them. For example, after `(ns-unmap ns 'String)` in my namespace, `(.length (format "foo = %s") 1)` throws an IllegalArgumentException: Unable to resolve classname: String. By using fully-qualified class names, that problem goes away.

2. tools.analyzer (used by the Clojure lint tool Eastwood) crashes when encountering such a simple-named return type hint. So currently, I cannot lint parts of my project because there's code that calls `clojure.core/format`.



 Comments   
Comment by Alex Miller [ 28/Aug/14 9:34 AM ]

1. that seems like a pretty weird thing to do
2. sounds like an issue with tools.analyzer, not with Clojure?

Comment by Nicola Mometto [ 28/Aug/14 10:46 AM ]

Just to clarify, tools.analyzer(.jvm) can analyze just fine forms in the form (defn x ^Class []) as long as Class is resolvable, whereas it will throw an exception if that function is then used in a namespace where that class is no longer resolvable, which is similar to what Clojure already does, except tools.analyzer.jvm will throw an exception even if the type hint is not used.

Since version 0.5.1 there's an handler that can be provided to change that behaviour, see https://github.com/clojure/tools.analyzer.jvm/blob/master/src/main/clojure/clojure/tools/analyzer/passes/jvm/validate.clj#L232

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

Now a comment regarding this ticket: the patch in this ticket is just a work-around for the issue exposed in http://dev.clojure.org/jira/browse/CLJ-1232, IMHO the correct move would be to actually recognize that issue as a bug rather than as an accepted "limitation" as Rich's comment seems to suggest so that a fix might be commited.

Comment by Tassilo Horn [ 28/Aug/14 1:29 PM ]

@Alex: 1. is not as weird as it sounds at first. For example, consider you have macros that generate complete APIs for something into some new namespace. Then it can make sense to use a real vanilla namespace, i.e., without referring clojure.core and importing java.lang. With 2. I side with Nicola and consider CLJ-1232 a bug.

@Nicola: Today I've used Eastwood (0.1.4) to lint my project. It crashed when it encountered this definition:

(defmacro error
  "Throws an exception with the given message and cause."
  ([msg]
     `(error ~msg nil))
  ([msg cause]
     `(throw (java.lang.Exception. ~msg ~cause))))

(defmacro errorf
  "Throws an exception with the given `msg` and `objs` passed to `format`.
  `msg` is a format string."
  [msg & objs]
  `(error (format ~msg ~@objs)))  ;; This is line 112 where the crash occurs

The message was:

Exception thrown during phase :analyze+eval of linting namespace funnyqt.tg-test
A function, macro, protocol method, var, etc. named clojure.core/format has been used here:
{:file "funnyqt/utils.clj",
 :end-column 19,
 :column 12,
 :line 112,
 :end-line 112}
Wherever it is defined, or where it is called, it has a type of String
This appears to be a Java class name with no package path.
Library tools.analyzer, on which Eastwood relies, cannot analyze such files.
If this definition is easy for you to change, we recommend you prepend it with
a full package path name, e.g. java.net.URI
Otherwise import the class by adding a line like this to your ns statement:
    (:import (java.net URI))

An exception was thrown while analyzing namespace funnyqt.tg-test 
Lint results may be incomplete.  If there are compilation errors in
your code, try fixing those.  If not, check above for info on the
exception.

So it seems it crashes because `format` has a `^String` return type hint. The namespace containing the `errorf` macro above has no modified ns-imports, i.e., all java.lang classes are imported there, too.

Comment by Nicola Mometto [ 28/Aug/14 1:46 PM ]

Tassilo, since `errorf` is a macro, that error is probably caused at the expansion point of that macro in a namespace that unmaps 'String.
If that's not the case, please open a ticket in the eastwood repo

Comment by Tassilo Horn [ 28/Aug/14 2:16 PM ]

Nicola, you are correct. As I've explained above to Alex, I generate APIs in fresh namespaces that don't refer clojure.core and also ns-unmap all java.lang classes, and the generated code also contains `errorf`-forms.

Well, since `ns-unmap` is there, I think it's legit to use it. So that makes CLJ-1232 even more important. But until that gets fixed which requires a common agreement that it is indeed a bug, I'd be very happy if this patch could be accepted. I mean, when it cannot do any harm and doesn't obscure anything but helps at least one person, then why not do it?





[CLJ-1513] Enhancing reader Created: 25/Aug/14  Updated: 25/Aug/14

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

Type: Enhancement Priority: Trivial
Reporter: Anton Rambold Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: edn, reader


 Description   

Attach "character start" and "character end" to the meta information of read forms produced by clojure.lang.EdnReader and clojure.lang.LispReader.
This will allows for better code inspection by linters for example. Currently only line number and column are attached to the meta information.



 Comments   
Comment by Andy Fingerhut [ 25/Aug/14 4:59 PM ]

I am not certain, but perhaps the EDN and regular reader in the tools.reader contrib library already do what you want here? That is, besides :line and :column metadata, they also have :end-line and :end-column metadata for the end of the expression.





[CLJ-1512] Create volatile box for managing state Created: 25/Aug/14  Updated: 07/Oct/14  Resolved: 03/Sep/14

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

Type: Enhancement Priority: Major
Reporter: Rich Hickey Assignee: Rich Hickey
Resolution: Completed Votes: 0
Labels: transducers

Attachments: File volatile2.diff     File volatile3.diff    
Patch: Code and Test
Approval: Ok

 Description   

Motivation:

Clojure needs a faster variant of Atom for managing state inside transducers. That is, Atoms do the job, but they provide a little too much capability for the purposes of transducers. Specifically the compare and swap semantics of Atoms add too much overhead. Therefore, it was determined that a simple volatile ref type would work to ensure basic propagation of its value to other threads and reads of the latest write from any other thread. While updates are subject to race conditions, access is controlled by JVM guarantees.

Solution overview: Create a concrete type in Java, akin to clojure.lang.Box, but volatile inside supports IDeref, but not watches etc.

API:

(volatile! x) ;;ctor
(vreset! vol newval) ;;like reset
(vswap! vol f args) ;;same shape as swap!, but MACRO over vreset!

Patch: volatile3.diff

Screened by: fogus



 Comments   
Comment by Alex Miller [ 25/Aug/14 9:11 AM ]

Dumb benchmark before/after...

java -cp target/classes -Xmx512m -server clojure.main
(def t (take 1000000))
(def v (doall (range 1000000)))
(defn bench [t v]
  (time (into [] t v)))
(dotimes [_ 30] (bench t v))

before - 29-32 ms after warmup
after - 22-23 ms after warmup

Comment by Alex Miller [ 25/Aug/14 9:12 AM ]

From Stu H elsewhere:

Three questions:
1) Should we keep volatile? in the public API?
2) Should we work in terms of IVolatile interface (guessing no)
3) Do we need a CLJS version of these APIs?

Comment by Alex Miller [ 25/Aug/14 9:13 AM ]

1. We have many tickets requesting predicates over types that are "internal" and generally I find these to be helpful. They also can help in making core more portable to cljs (maybe those fns would fall back to atoms in cljs?).
2. We have tickets requesting the equivalent of this for IAtom (CLJ-803) etc. I don't think an interface adds any value to us here though. There seems to be some requests for this kind of passthrough interface from tooling as a decoupling point. Not putting my finger on those discussions but I know I've heard this, maybe on the mailing list.
3. I think yes if that allows us to be more efficient than whatever is being done now. Not obvious to me.

Comment by Nicola Mometto [ 25/Aug/14 9:40 AM ]

Why is vswap! a macro?

Comment by Ambrose Bonnaire-Sergeant [ 26/Aug/14 8:04 AM ]

An IAtom conversation: https://groups.google.com/forum/#!searchin/clojure-dev/iatom/clojure-dev/y5QoMqd44Lc/y4YmW09blk0J

Comment by Max Penet [ 26/Aug/14 10:28 AM ]

the vswap! macro is probably for performance reasons (the main motivation of this code to begin with), to avoid using apply or unrolling tons of arities

Comment by Nicola Mometto [ 26/Aug/14 1:07 PM ]

If that is the only reason, why can't it be a regular fn + :inline metadata?

Comment by Jozef Wagner [ 27/Aug/14 3:50 AM ]

why the bang in the name of volatile! function? If the reason is to warn users that this is an 'expert only' stuff, I suggest to use a verbose name instead, e.g. volatile-reference. (This will also be consistent with approach chosen in the names of volatile-mutable and unsynchronized-mutable hints.)

Comment by Rich Hickey [ 27/Aug/14 6:37 AM ]

Can you please lift the with-meta stuff out of the syntax-quote?
Actually, if volatile! ctor returned a type-hinted value that extra hinting might not even be needed. Let's do both for now.

Also the type hint on the volatile? arg makes no sense - it's a predicate asking if something is a volatile.

Comment by Alex Miller [ 28/Aug/14 9:05 AM ]

Made changes as requested.

Comment by Fogus [ 29/Aug/14 11:01 AM ]

I downloaded the patch and applied to latest master. I ran the isolated tests and the full test suite and also ensured that the patch didn't add any reflection warnings. I then modified the ticket description to add a little more context and motivation (for future readers). The code is straight-forward and clean.

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

Updated to volatile3.diff to address offline comment from Rich.





[CLJ-1511] stack overflow when comparing sequence results Created: 24/Aug/14  Updated: 07/Oct/14  Resolved: 27/Aug/14

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

Type: Defect Priority: Critical
Reporter: Chhi'mèd Künzang Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: transducers
Environment:

OS X 10.9.4


Attachments: Text File 0001-provide-working-implementations-for-LazyTransform-eq.patch    
Patch: Code and Test
Approval: Ok

 Description   

Comparing sequences created with sequence causes a stack overflow when used as first argument to =.

Consider this transducer:

user=> (def map-inc (map inc))
#'user/map-inc

When creating a sequence and comparing with expected results, it works fine as the second argument to the comparison:

user=> (= (range 1 11) (sequence map-inc (range 10)))
true

But a stack overflow occurs when the order of arguments is reversed:

user=> (= (sequence map-inc (range 10)) (range 1 11))

StackOverflowError   clojure.lang.LazyTransformer.equals (LazyTransformer.java:202)
user=> (clojure.stacktrace/print-stack-trace *e 10)
java.lang.StackOverflowError: null
 at clojure.lang.LazyTransformer.equiv (LazyTransformer.java:185)
    clojure.lang.LazyTransformer.equals (LazyTransformer.java:202)
    clojure.lang.LazyTransformer.equiv (LazyTransformer.java:185)
    clojure.lang.LazyTransformer.equals (LazyTransformer.java:202)
    clojure.lang.LazyTransformer.equiv (LazyTransformer.java:185)
    clojure.lang.LazyTransformer.equals (LazyTransformer.java:202)
    clojure.lang.LazyTransformer.equiv (LazyTransformer.java:185)
    clojure.lang.LazyTransformer.equals (LazyTransformer.java:202)
    clojure.lang.LazyTransformer.equiv (LazyTransformer.java:185)
    clojure.lang.LazyTransformer.equals (LazyTransformer.java:202)
nil

The error persists, even if the sequence is forced with doall:

user=> (= (doall (sequence map-inc (range 10))) (doall (range 1 11)))

StackOverflowError   clojure.lang.LazyTransformer.equiv (LazyTransformer.java:185)

It does work as expected, however, if the sequence is converted to a vector:

user=> (= (vec (sequence map-inc (range 10))) (range 1 11))
true


 Comments   
Comment by Nicola Mometto [ 25/Aug/14 4:31 AM ]

Patch provides equiv/equals implementations for LazyTransform based on ASeq equiv/equals





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

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

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

OSX, lein repl



 Description   

Executing in lein repl either

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

Or

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

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



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

If instead of 'lein repl' you run:

java -cp clojure.jar clojure.main

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

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

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

Comment by Alex Miller [ 22/Aug/14 8:29 AM ]

Agreed with Andy - these examples work fine in base repl but other environments may be affecting your input upstream of clojure.

Comment by Andy Fingerhut [ 25/Aug/14 11:42 AM ]

Geoff, it looks like there have been recent changes committed to the tools.nrepl library related to this issue. tools.nrepl is used within Leiningen to implements its REPL.

https://github.com/clojure/tools.nrepl/commit/eb526fd8498ced1b4bd1555f8ff680f3ad65f1b4





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

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

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

Patch: Code
Approval: Triaged

 Description   

There is a list of namespaces to AOT in build.xml and several namespaces are missing from that list, thus no .class files for those namespaces are created or included in the standard clojure jar file as part of the build.

Missing namespaces include:

  • clojure.core.reducers
  • clojure.instant
  • clojure.parallel
  • clojure.uuid

Proposal: Attached patch sorts the ns list alphabetically (for easier maintenance) and adds clojure.instant and clojure.uuid to the compiled namespaces. clojure.parallel is deprecated and requires the JSR-166 jar so was not included (perhaps it's a separate ticket to remove this). clojure.core.reducers uses a compile-time check to choose the fork/join packages to use so cannot be compiled early.

Patch: clj-1509.diff

Screened by:



 Comments   
Comment by Alex Miller [ 20/Aug/14 1:06 PM ]

Looking at this a bit further, clojure.core.reducers uses the compile-if macro to determine what version of fork/join is available so AOT-compiling this namespace would fix that decision at build time rather than runtime, so it cannot be included.





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

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

Type: Enhancement Priority: Minor
Reporter: dennis zhuang Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: destructuring
Environment:

Mac OSX 10.9.4

java version "1.7.0_17"
Java(TM) SE Runtime Environment (build 1.7.0_17-b02)
Java HotSpot(TM) 64-Bit Server VM (build 23.7-b01, mixed mode)


Attachments: File supplied_p.diff    
Patch: Code and Test

 Description   

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

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

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

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

The patch is attached with code and test.



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

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

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




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

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

Type: Defect Priority: Minor
Reporter: dennis zhuang Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: eval-reader
Environment:

Mac OSX 10.9.4
java version "1.7.0_17"
Java(TM) SE Runtime Environment (build 1.7.0_17-b02)
Java HotSpot(TM) 64-Bit Server VM (build 23.7-b01, mixed mode)


Attachments: File fix_npe_eval_reader.diff    
Patch: Code

 Description   
Clojure 1.7.0-master-SNAPSHOT
user=> #=(var a)
NullPointerException   clojure.lang.Symbol.hashCode (Symbol.java:84)
user=> (.printStackTrace *e)
clojure.lang.LispReader$ReaderException: clojure.lang.LispReader$ReaderException: java.lang.NullPointerException
	at clojure.lang.LispReader.read(LispReader.java:218)
	at clojure.core$read.invoke(core.clj:3580)
	at clojure.core$read.invoke(core.clj:3578)
	at clojure.core$read.invoke(core.clj:3576)
	at clojure.core$read.invoke(core.clj:3574)
	at clojure.main$repl_read.invoke(main.clj:139)
	at clojure.main$repl$read_eval_print__6807$fn__6808.invoke(main.clj:237)
	at clojure.main$repl$read_eval_print__6807.invoke(main.clj:237)
	at clojure.main$repl$fn__6816.invoke(main.clj:257)
	at clojure.main$repl.doInvoke(main.clj:257)
	at clojure.lang.RestFn.invoke(RestFn.java:421)
	at clojure.main$repl_opt.invoke(main.clj:323)
	at clojure.main$main.doInvoke(main.clj:421)
	at clojure.lang.RestFn.invoke(RestFn.java:397)
	at clojure.lang.Var.invoke(Var.java:375)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)
Caused by: clojure.lang.LispReader$ReaderException: java.lang.NullPointerException
	at clojure.lang.LispReader.read(LispReader.java:218)
	at clojure.lang.LispReader$CtorReader.invoke(LispReader.java:1164)
	at clojure.lang.LispReader$DispatchReader.invoke(LispReader.java:609)
	at clojure.lang.LispReader.read(LispReader.java:183)
	... 17 more
Caused by: java.lang.NullPointerException
	at clojure.lang.Symbol.hashCode(Symbol.java:84)
	at java.util.concurrent.ConcurrentHashMap.hash(ConcurrentHashMap.java:332)
	at java.util.concurrent.ConcurrentHashMap.get(ConcurrentHashMap.java:987)
	at clojure.lang.Namespace.findOrCreate(Namespace.java:173)
	at clojure.lang.RT.var(RT.java:341)
	at clojure.lang.LispReader$EvalReader.invoke(LispReader.java:1042)
	at clojure.lang.LispReader$DispatchReader.invoke(LispReader.java:616)
	at clojure.lang.LispReader.read(LispReader.java:183)
	... 20 more

If the var symbol doesn't contains namespace ,it will throw the NPE exception in above code.Instead,i think it should use Compiler.currentNS() when doesn't find the var's namespace.

The patch is attached, after patched:

Clojure 1.7.0-master-SNAPSHOT
user=> #=(var a)
#'user/a





[CLJ-1506] A little improvement when reading syntax quote form Created: 16/Aug/14  Updated: 30/Aug/14

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

Type: Enhancement Priority: Minor
Reporter: dennis zhuang Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: syntax-quote
Environment:

Mac OSX 10.9.4
java version "1.7.0_17"
Java(TM) SE Runtime Environment (build 1.7.0_17-b02)
Java HotSpot(TM) 64-Bit Server VM (build 23.7-b01, mixed mode)


Attachments: File fast_syntax_quote_reader.diff    
Patch: Code

 Description   

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

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

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

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

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






[CLJ-1505] Sorry I have to enter this test bug Created: 15/Aug/14  Updated: 15/Aug/14  Resolved: 15/Aug/14

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

Type: Defect Priority: Major
Reporter: Karen Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

Interacting with this site caused FireFox to freak out when populating a field in our app so I need to enter a bug here to figure out why. See http://dev.clojure.org/jira/browse/CLJ-1378 We get the same error message in one of our summary field. It is a FireFox bug so I'll update this when I am done. Apologies.






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

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

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

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

 Description   

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



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

Related ticket CLJ-1227 and related quote from Alex:

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

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

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





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

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

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

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

 Description   

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

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



 Comments   
Comment by Nicola Mometto [ 24/Sep/14 5:04 AM ]

Duplicated of http://dev.clojure.org/jira/browse/CLJ-1425
Also I no longer think this is a good idea

Comment by Kevin Downey [ 14/Oct/14 1:27 PM ]

It seems like this is a bad idea, it sort of makes sense from purely a macro writing perspective, but syntax quote is used outside of macros, in which case this just becomes a circumvention of the duplicate key checks that were added, I think some time around 1.3 maybe 1.4





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

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

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

Windows 7 and 8, Java 7, Clojure repl


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

 Description   

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

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

and then you inspect the tree representing the object

(inspect-tree nst)

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

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

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

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



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

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

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

Thanks, Andy

  • DC




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

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

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

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

 Description   

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

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

Screened by: Alex Miller



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

Patch clj-1501.diff with tests added

Comment by Andy Fingerhut [ 08/Oct/14 5:50 PM ]

This ticket has no Fix Version/s, but is Screened, so at least in some code I have it is 'off the JIRA workflow state diagram'. Not sure if it shows up that way in your filters, Alex.

Comment by Alex Miller [ 08/Oct/14 7:45 PM ]

Yup thanks.





[CLJ-1500] Cache namespace env Created: 08/Aug/14  Updated: 08/Aug/14  Resolved: 08/Aug/14

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

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


 Description   

See: https://github.com/clojure/tools.analyzer.js/blob/master/src/main/clojure/clojure/tools/analyzer/js.clj#L524-L548






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

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

Type: Enhancement Priority: Critical
Reporter: Rich Hickey Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: File clj-1499-all.diff     File clj-1499-v2.diff     File clj-1499-v3.diff     Text File defrecord-iterator.patch     File defrecord-iterator-v2.diff    
Patch: Code and Test
Approval: Vetted

 Description   

Add support for direct iterators instead of seq-based iterators for non-seq collections that use SeqIterator.

Patch adds support for direct iterators on the following (removing use of SeqIterator):

  • PersistentHashMap - new internal iterator (~20% faster)
  • APersistentSet - use internal map impl iterator (~5% faster)
  • PersistentQueue
  • PersistentStructMap
  • records (in core_deftype.clj)

Patch does not change use of SeqIterator in:

  • LazyTransformer$MultiStepper (not sure if this could be changed)
  • ASeq
  • LazySeq

Patch: clj-1499-all.diff



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

The list of non-seqs that uses SeqIterator are:

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

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

  • ASeq
  • LazySeq.java

LazyTransformer$MultiStepper - not sure

Comment by Ghadi Shayban [ 27/Sep/14 2:16 PM ]

attached iterator impl for defrecords. ready to leverage iteration for extmap when PHM iteration lands.

Comment by Alex Miller [ 29/Sep/14 12:52 PM ]

PHM patch

Comment by Alex Miller [ 29/Sep/14 10:26 PM ]

New patch that fixes bugs with PHMs with null keys (and added tests to expose those issues), added support for PHS.

Comment by Ghadi Shayban [ 29/Sep/14 10:45 PM ]

Alex, the defrecord patch already uses the iterator for extmap. It's just made better by the PHM patch.

Comment by Alex Miller [ 29/Sep/14 10:47 PM ]

Comment by Ghadi Shayban [ 30/Sep/14 4:17 PM ]

Heh. Skate to where the puck is going to be – Gretzky

Re: defrecord iterator: As is, it propagates exceptions from reaching the end of the ExtMap's iterator. As noted in CLJ-1453, PersistentArrayMap's iterator improperly returns an ArrayIndexOutOfBoundsException, rather than NoSuchElementException.

Comment by Alex Miller [ 30/Sep/14 6:41 PM ]

Hey Ghadi, rather than rebuilding the case map to pass to the RecordIterator, why don't you just pass the fields in iteration order to it and leverage the case map via .valAt like everything else?

Comment by Ghadi Shayban [ 30/Sep/14 7:30 PM ]

defrecord-iterator-v2.diff reuses valAt and minimizes macrology.

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

Comments from Stu (found under the couch):

"1. some of the impls (e.g. queue manually concatenate two iters. Would implementing iter-cat and calling that be simpler and more robust?
2. I found this tweak to the generative testing more useful in reporting failure, non-dependent on clojure.test, and capable of expecting failures. Waddya think?

(defn seq-iter-match
  [seqable iterable]
  (let [i (.iterator iterable)]
    (loop [s (seq seqable)
           n 0]
      (if (seq s)
        (do
          (when-not (.hasNext i)
            (throw (ex-info "Iterator exhausted before seq"
                            {:pos n :seqable seqable :iterable iterable})))
          (when-not (= (.next i) (first s))
            (throw (ex-info "Iterator and seq did not match"
                            {:pos n :seqable seqable :iterable iterable})))
          (recur (rest s) (inc n)))
        (when (.hasNext i)
          (throw (ex-info "Seq exhausted before iterator"
                          {:pos n :seqable seqable :iterable iterable})))))))

		(defspec seq-and-iter-match-for-maps
  identity
  [^{:tag clojure.test-clojure.data-structures/gen-map} m]
  (seq-iter-match m m))

3. similar generative approach would be good for the other types (looks like we just do maps)"





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

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

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

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

 Description   

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

Current simple test:

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

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

After:

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

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

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

Patch: clj-1498-3.diff

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



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

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

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

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

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

New patch that replaces AtomicReference<Thread> with AtomicBoolean.

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

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

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

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

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

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





[CLJ-1497] sequence with transducers realizes n+2 elements Created: 08/Aug/14  Updated: 08/Aug/14  Resolved: 08/Aug/14

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: None

Attachments: File clj-1497.diff     File clj-1497v2.diff    
Approval: Ok

 Description   

The first element is realized at creation time:

user=> (def a (sequence (take 2) (map #(do (println (str "~" %)) %) (iterate inc 1))))
~1
#'user/a

Fully realizing the sequence realizes the other n-1 elements, and 2 more:

user=> a
(~2
~3
1 ~4
2)

Compare with version using seq operations:

user=> (def a (sequence (take 2 (map #(do (println (str "~" %)) %) (iterate inc 1)))))
#'user/a
user=> a
(~1
~2
1 2)

Transduce also doesn't seem to exhibit this issue:

user=> (def a (transduce (take 2) conj [] (map #(do (println (str "~" %)) %) (iterate inc 1))))
~1
~2
#'user/a
user=> a
[1 2]


 Comments   
Comment by Alex Miller [ 08/Aug/14 10:02 AM ]

Patch attached that improves the issue - will now only realize n+1 elements.

Comment by Nicola Mometto [ 08/Aug/14 10:16 AM ]

Nice, I added a commit on top of yours to delay the realization of the first element of the lazyseq to the first .next call instead of on SeqIteration creation

Comment by Alex Miller [ 08/Aug/14 11:12 AM ]

Fixed by Rich directly, not by patch.





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

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

Type: Enhancement Priority: Minor
Reporter: dennis zhuang Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: ex-info, exceptions
Environment:

java version "1.7.0_17"
Java(TM) SE Runtime Environment (build 1.7.0_17-b02)
Java HotSpot(TM) 64-Bit Server VM (build 23.7-b01, mixed mode)

Mac OSX 10.9.4


Attachments: File ex_info_arity.diff    
Patch: Code

 Description   

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

(ex-info "the exception message")

That created a ExceptionInfo instance carries empty data.

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

The patch is attached.






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

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

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


 Description   

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

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

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

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

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





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

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

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

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

 Description   

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

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



 Comments   
Comment by Alex Miller [ 03/Sep/14 11:02 AM ]

Done as a result of https://github.com/clojure/clojure/commit/7d84a9f6f35a503cddf98487b6544d18937c669e





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

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

Type: Enhancement Priority: Major
Reporter: dennis zhuang Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: keywords, performance
Environment:

Mac OS X 10.9.4 / 2.6 GHz Intel Core i5 / 8 GB 1600 MHz DDR3
java version "1.7.0_17"
Java(TM) SE Runtime Environment (build 1.7.0_17-b02)
Java HotSpot(TM) 64-Bit Server VM (build 23.7-b01, mixed mode)


Attachments: File fast_keyword_intern.diff    
Patch: Code
Approval: Triaged

 Description   

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

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

Before patched:

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

After patched:

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

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

The patch is attached and test.

Thanks.

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



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

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

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

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

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

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

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

Hi,andy

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

Comment by dennis zhuang [ 11/Sep/14 10:34 AM ]

I am glad to see it is helpful.I benchmark the patch with current master branch,it's fine too.





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

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

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

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


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

 Description   

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

The simplest case:

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

And you get the same exception when embedding a PersistentQueue:

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

Instead of the expected:

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

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

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

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

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

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






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

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

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

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

 Description   

Consider the following example.

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

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

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

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

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

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

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

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



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

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

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

Not challenging the premise at all but workaround:

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

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

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

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

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

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

Comment by Nicola Mometto [ 25/Sep/14 9:59 AM ]

The patch for http://dev.clojure.org/jira/browse/CLJ-1533?jwupdated=61127&focusedCommentId=35814 fixes this issue and more and should be preferred over this

Comment by Alex Miller [ 25/Sep/14 10:31 AM ]

Dupe of CLJ-1533





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

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

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

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

 Description   

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

However, the exception that gets generated can be confusing:

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

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

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

A patch will be ready shortly.



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

Patch with tests





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

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

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

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

 Description   

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

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

Depends on CLJ-1488



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

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

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





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

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

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

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

 Description   

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

The attached patch implements Named over Vars.

Example:

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

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



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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

Added patches explicitly handling the unnamed var cases.

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

Squashed all patches into a single diff and updated attachments.





[CLJ-1487] Variadic unrolling for partial Created: 01/Aug/14  Updated: 01/Aug/14  Resolved: 01/Aug/14

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

Type: Enhancement Priority: Minor
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Duplicate Votes: 0
Labels: None

Attachments: Text File unroll-partial.patch    
Patch: Code and Test

 Description   

Many of the functions in clojure.core are variadic-unrolled for performance. The implementation of juxt, for example, is 29 lines where it could be just 3 if we didn't care about performance. For the function "partial", this unrolling is, if you will excuse the pun, only partially done. This patch extends the unrolling for partial to mirror that for juxt and comp, and includes a test that partial works for any reasonable number of arguments (the existing test just spot-checks a few arities).

I've done some performance benchmarking, and we get about a 20% speedup on calling partial functions, with no performance penalty for creating them.

You may note that the n-ary case, ([arg1 arg2 arg3 & more] ...), is not unrolled in my patch. Doing so would have worsened performance: because we're having to call apply anyway, there's no real benefit to be had from unrolling, and it costs a little to rebuild the arglist before passing it to apply.



 Comments   
Comment by Ghadi Shayban [ 01/Aug/14 6:45 PM ]

Dupe of CLJ-1430

Comment by Alex Miller [ 01/Aug/14 11:10 PM ]

Dupe as noted in the comments





[CLJ-1486] Make fnil var-arg Created: 31/Jul/14  Updated: 18/Sep/14

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

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

Attachments: Text File 0001-make-fnil-vararg.patch    
Patch: Code and Test

 Description   

Currently fnil is defined only for 1 to 3 args, this patch makes it var-arg






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

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

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

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

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

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



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

Patch for issue





[CLJ-1483] Clarify the usage of replace(-first) with a function Created: 27/Jul/14  Updated: 29/Jul/14

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

Type: Enhancement Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring, string

Attachments: Text File 0001-Clarify-the-usage-of-replace-first-with-pattern-func.patch    
Patch: Code
Approval: Triaged

 Description   

The documentation of replace and replace-first didn't feature any example usage of the pattern + function combo so I've added one.






[CLJ-1482] Replace a couple of (filter (complement ...) ...) usages with (remove ...) Created: 27/Jul/14  Updated: 27/Jul/14

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

Type: Enhancement Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: enhancement

Attachments: Text File 0001-Replace-a-couple-of-filter-complement-usages-with-re.patch    
Patch: Code

 Description   

The title basically says it all - remove exists so we can express our intentions more clearly.






[CLJ-1481] Typo in type-reflect's docstring Created: 27/Jul/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

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

Type: Defect Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docstring, ft

Attachments: Text File 0001-Fix-a-typo.patch    
Patch: Code
Approval: Ok

 Description   

membrer -> member

Screened by: Alex Miller






[CLJ-1480] Incorrect param name reference in defmulti's docstring Created: 27/Jul/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

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

Type: Defect Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docstring, ft

Attachments: Text File 0001-Fix-param-name-reference-in-defmulti-s-docstring.patch    
Patch: Code
Approval: Ok

 Description   

attribute-map should actually be attr-map

Screened by: Alex Miller






[CLJ-1479] Typo in filterv example Created: 27/Jul/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

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

Type: Enhancement Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: documentation, ft

Attachments: Text File 0001-Fix-a-typo.patch    
Patch: Code
Approval: Ok

 Description   

filter -> filterv in changes.md

Screened by: Alex Miller






[CLJ-1478] Doc typo Created: 27/Jul/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

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

Type: Enhancement Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: docstring, ft

Attachments: Text File 0001-Fix-a-typo.patch    
Patch: Code
Approval: Ok

 Description   

Another small typo fix:
from from -> from

Screened by: Alex Miller






[CLJ-1477] Fixed a typo Created: 27/Jul/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

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

Type: Enhancement Priority: Trivial
Reporter: Bozhidar Batsov Assignee: Unassigned
Resolution: Completed Votes: 0
Labels: documentation, ft

Attachments: Text File 0001-Fix-a-typo.patch    
Patch: Code
Approval: Ok

 Description   

Just a simple typo fix - "directy" -> "directly".

Screened by: Alex Miller






[CLJ-1476] map-invert should use (empty m) instead of {} Created: 26/Jul/14  Updated: 27/Jul/14  Resolved: 27/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Gregory Schlomoff Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None


 Description   

clojure.set/map-invert should reduce with (empty m) instead of {} so that it returns a map of the same type as its argument.

This is a trivial change and I'm willing to submit a patch if nobody opposes.



 Comments   
Comment by Alex Miller [ 26/Jul/14 8:43 AM ]

I don't think that always makes sense. Say you had a map of string to integers with a custom comparator created by sorted-map-by. If you use empty, you'd still have a map with a custom comparator which you would pour integer keys into and would likely throw a ClassCastException.

What is the use case that led you to this ticket?

Comment by Gregory Schlomoff [ 26/Jul/14 9:14 AM ]

Hello Alex, thanks for commenting.

My use case is that I have a custom type that implements IPersistentMap. If I use map-invert over it, I get a regular map back, which is problematic because regular maps don't allow multiple values for the same key, unlike my multimap implementation, so I loose information.

(map-invert (my-multimap :a 1, :b 1))
=> {1 :b} ; lost the (1 :a) entry because regular maps don't allow duplicate keys

Maybe a solution would be to make a version of map-invert that takes a map to insert the inverted entries into?

I'm not adamant over this, if you think there is no elegant solution for this issue we can close it.

Comment by Alex Miller [ 27/Jul/14 7:28 AM ]

I don't think this enhancement makes sense as written - there are cases where it would be a breaking change for existing code.

I do think your specified problem makes sense though. One enhancement might be to have a variant of map-invert (different arity or map-invert-into that took an additional map target param).





[CLJ-1475] :post condition causes compiler error with recur Created: 25/Jul/14  Updated: 29/Jul/14

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

Type: Defect Priority: Minor
Reporter: Steve Miner Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler

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

 Description   

Michael O'Keefe <michael.p.okeefe@gmail.com> posted on the mailing list an example of code that causes a compiler error only if a :post condition is added. Here's my slightly modified version:

(defn g
  [xs acc]
  {:pre [(or (nil? xs) (sequential? xs))]
   :post [(number? %)]}
  (if (seq xs)
     (recur (next xs) (+ (first xs) acc))
     acc))

CompilerException java.lang.UnsupportedOperationException: Can only recur from tail position

The work-around is to wrap the body in a loop that simply rebinds the original args.



 Comments   
Comment by Steve Miner [ 25/Jul/14 9:53 AM ]

A macro expansion shows that body is placed in a let form to capture the result for later testing with the post condition, but the recur no longer has a proper target. The work-around of using a loop form is easy once you understand what's happening but it's a surprising limitation.

Comment by Steve Miner [ 25/Jul/14 9:55 AM ]

Use a local fn* around the body and call it with the original args so that the recur has a proper target. Update: not good enough for handling destructuring. Patch withdrawn.

Comment by Michael Patrick O'Keefe [ 25/Jul/14 10:37 AM ]

Link to the original topic discussion: https://groups.google.com/d/topic/clojure/Wb1Nub6wVUw/discussion

Comment by Steve Miner [ 25/Jul/14 1:42 PM ]

Patch withdrawn because it breaks on destructured args.

Comment by Steve Miner [ 25/Jul/14 5:27 PM ]

While working on a patch, I came up against a related issue: Should the :pre conditions apply to every recur "call". Originally, I thought the :pre conditions should be checked just once on the initial function call and never during a recur. People on the mailing list pointed out that the recur is semantically like calling the function again so the :pre checks are part of the contract. But no one seemed to want the :post check on every recursion, so the :post would happen only at the end.

That means automatically wrapping a loop (or nested fn* call) around the body is not going to work for the :pre conditions. A fix would have to bring the :pre conditions inside the loop.

Comment by Steve Miner [ 26/Jul/14 8:54 AM ]

I'm giving up on this bug. My approach was adding too much complexity to handle an edge case. I recommend the "loop" work-around to anyone who runs into this problem.

(defn g2
  [xs acc]
  {:pre [(or (nil? xs) (sequential? xs))]
   :post [(number? %)]}
  (loop [xs xs acc acc]
    (if (seq xs)
       (recur (next xs) (+ (first xs) acc))
       acc)))
Comment by Ambrose Bonnaire-Sergeant [ 26/Jul/14 10:29 AM ]

Add patch that handles rest arguments and destructuring.

Comment by Michael Patrick O'Keefe [ 26/Jul/14 10:57 AM ]

With regard to Steve's question on interpreting :pre, to me I would expect g to act like the case g3 below which uses explicit recursion (which does work and does appear to check the :pre conditions each time and :post condition once):

(defn g3
  [xs acc]
  {:pre [(or (sequential? xs) (nil? xs)) (number? acc)]
   :post [(number? %)]}
  (if (seq xs)
    (g3 (next xs) (+ (first xs) acc))
    acc))
Comment by Ambrose Bonnaire-Sergeant [ 26/Jul/14 11:42 AM ]

Patch clj-1475.diff handles destructuring, preconditions and rest arguments

Comment by Steve Miner [ 26/Jul/14 4:04 PM ]

The clj-1475.diff patch looks good to me.

Comment by Alex Miller [ 27/Jul/14 7:18 AM ]

Please don't use "patch" as a label - that is the purpose of the Patch field. There is a list of good and bad labels at http://dev.clojure.org/display/community/Creating+Tickets

Comment by Steve Miner [ 27/Jul/14 11:32 AM ]

More knowledgeable commenters might take a look at CLJ-701 just in case that's applicable to the proposed patch.

Comment by Kevin Downey [ 29/Jul/14 1:35 AM ]

re clj-701

it is tricky to express loop expression semantics in jvm byte code, so the compiler sort of punts, hoisting expression loops in to anonymous functions that are immediately invoked, closing over whatever is in scope that is required by the loop, this has some problems like those seen in CLJ-701, losing type data which the clojure compiler doesn't track across functions, the additional allocation of function objects (the jit may deal with that pretty well, I am not sure) etc.

where the world of clj-701 and this ticket collide is the patch on this ticket lifts the function body out as a loop expression, which without the patch in clj-701 will have the issues I listed above, but we already have those issues anywhere something that is difficult to express in bytecode as an expression (try and loop) is used as an expression, maybe it doesn't matter, or maybe clj-701 will get fixed in some way to alleviate those issues.

general musings

it seems like one feature people like from asserts is the ability to disable them in production (I have never actually seen someone do that with clojure), assert and :pre/:post have some ability to do that (it may only work at macroexpansion time, I don't recall) since the hoisting of the loop could impact performance it might be nice to have some mechanism to disable it (maybe using the same flag assert does?).





[CLJ-1474] `reduced` docstring should be more explicit Created: 25/Jul/14  Updated: 27/Jul/14  Resolved: 25/Jul/14

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

Type: Enhancement Priority: Major
Reporter: Jean Niklas L'orange Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: docstring


 Description   

The documentation for reduced is as follows:

Wraps x in a way such that a reduce will terminate with the value x

From what I gather, this does not specify whether the init value of a reduce could be a reduced value or not. As shown, the fact that the init value is a reduced value is ignored:

(reduce list (reduced 1) [2])
=> (#<Reduced@518a6aa: 1> 2)

The documentation should explicitly mention that a reduce call will not check if the initial value is reduced.



 Comments   
Comment by Alex Miller [ 25/Jul/14 9:09 AM ]

reduced creates a value that has special meaning as the output of invocation of the reducing function. Your example is about an input to that function. I don't see that this makes sense or needs documenting.

You can of course invent a situation where a (reduced 1) input is also the output but again, that seems like a pretty weird use case.

(reduce (fn [a v] a) (reduced 1) [2])
;; 1
Comment by Jean Niklas L'orange [ 25/Jul/14 12:10 PM ]

Right, that's my point. Nowhere in the documentation does it state that this does not apply to the initial value given to reduce. While you and I know this, I don't see how one can conclude this based on the current documentation.

Put differently, someone might wrongly assume that reduce is implemented as an optimised version of this:

(defn reduce [f init coll]
  (cond (reduced? init) (unreduced init)
        (empty? coll)    init
        :else           (recur f (f init (first coll))
                                 (rest coll))))

However, that's not the case, which I think is worth pointing out.

Comment by Alex Miller [ 25/Jul/14 5:01 PM ]

But it might apply to the initial value (as in my example where a reduced value is respected - note that doesn't return (reduced 1), just 1). Your suggested documentation change is talking about input values, but in my mind that leads to incorrect conclusions.

The only change that would make sense to me is clarifying where a "reduced" value is checked (on the result of applying the function passed to reduce). I think that's already implicit in the existing doc string myself. Since we have multiple implementations of "reduce", we have to tread carefully not to refer to explicitly to a particular one.

This use of a reduced initial value does not even make sense; why we would we confuse the docstring to warn about it?

Comment by Jean Niklas L'orange [ 27/Jul/14 7:20 AM ]

Ah, I get your point now, and I see how this would just create more confusion.

Thanks for the explanation.





[CLJ-1473] Bad pre/post conditions silently passed Created: 24/Jul/14  Updated: 11/Sep/14

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

Type: Enhancement Priority: Major
Reporter: Brandon Bloom Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: errormsgs

Attachments: Text File 0001-Validate-that-pre-and-post-conditions-are-vectors.patch    
Patch: Code and Test
Approval: Triaged

 Description   

Before:

user=> ((fn [x] {:pre (pos? x)} x) -5) ; ouch!
-5
user=> ((fn [x] {:pre [(pos? x)]} x) -5) ; meant this
AssertionError Assert failed: (pos? x)  user/eval4075/fn--4076 (form-init5464179453862723045.clj:1)

After:

user=> ((fn [x] {:pre (pos? x)} x) -5)
CompilerException java.lang.IllegalArgumentException: Pre and post conditions should be vectors, compiling:(NO_SOURCE_PATH:1:2) 
user=> ((fn [x] {:pre [(pos? x)]} x) -5)                                  
AssertionError Assert failed: (pos? x)  user/eval2/fn--3 (NO_SOURCE_FILE:2)
user=> ((fn [x] {:post (pos? x)} x) -5)
CompilerException java.lang.IllegalArgumentException: Pre and post conditions should be vectors, compiling:(NO_SOURCE_PATH:3:2) 
user=> ((fn [x] {:post [(pos? x)]} x) -5)              
AssertionError Assert failed: (pos? x)  user/eval7/fn--8 (NO_SOURCE_FILE:4)





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

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

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

Android ART runtime


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

 Description   

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

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

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



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

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

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

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

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

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

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

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





[CLJ-1471] Option to print type info Created: 21/Jul/14  Updated: 21/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Pascal Germroth Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: pprint


 Description   

I've had an issue with defrecord-types being converted into ordinary maps somewhere, which was relatively hard to track down inside a deep structure since they are pprinted as the same thing by default.
The following code patches into the pprint dispatch and prints the type around values; it turned out to be quite useful, but feels hackish.
Maybe something like that would be useful to integrate into clojure.pprint directly (there are a number of cosmetic options already), i.e. into clojure.pprint/write-out.

Only printing (type) may not be enough in some cases; so an option to print all metadata would be nice.
Maybe something like :metadata nil as default, :metadata :type to print types (but also for non-IMetas, using (type) and :metadata true to print metadata for IMetas using (meta).

(defn pprint-with-type
  ([object] (pprint object *out*))
  ([object writer]
   ; keep original dispatch.
   ; calling it directly will print only that object,
   ; but return to our dispatch for subobjects.
   (let [dispatch clojure.pprint/*print-pprint-dispatch*]
     (binding [clojure.pprint/*print-pprint-dispatch*
               (fn [obj]
                 (if (instance? clojure.lang.IMeta obj)
                   (do (print "^{:type ")
                       (dispatch (type obj))
                       (print "} ")
                       (clojure.pprint/pprint-newline :fill)
                       (dispatch obj))
                   (do (print "(^:type ")
                       (dispatch (type obj))
                       (print " ")
                       (clojure.pprint/pprint-newline :fill)
                       (dispatch obj)
                       (print ")"))))]
       (clojure.pprint/pprint object writer)))))





[CLJ-1470] Make Atom and ARef easy to subclass Created: 20/Jul/14  Updated: 23/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Aaron Craelius Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File CLJ-1470-v1.patch    
Patch: Code

 Description   

Atom is currently defined as final and ARef.validate() is package-private. This makes it impossible to define a subclass of an Atom and difficult subclass ARef (if validate() needs to be called).

I propose removing the final modifier from Atom, making ARef.validate() protected and also making Atom.state protected (it is currently package-private).

I'm not sure if there is a specific reason why Atom is final - if this is for performance reasons or to prevent someone from doing strange things with Atom's, but I can see a use case for sub-classing it.

One use-case is to create reactive Atom that allows derefs to be tracked (as in reagent). I have some Clojure (not Clojurescript) code where I'm trying to play with this idea and I've had to copy the entire Atom class (because it's sealed) and place it in the clojure.lang package (because ARef.validate() is package-private): https://github.com/aaronc/freactive/blob/master/src/java/clojure/lang/ReactiveAtom.java. In addition, I need to copy the defns for swap! and reset! into my own namespace. This seems a bit inconvenient.



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

related to http://dev.clojure.org/jira/browse/CLJ-803





[CLJ-1469] Emit KeywordInvoke callsites only when keyword is not namespaced Created: 18/Jul/14  Updated: 22/Oct/14

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

Type: Enhancement Priority: Trivial
Reporter: Ghadi Shayban Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File kwinvoke.patch    
Patch: Code

 Description   

Summary: Don't emit KeywordLookup thunks and machinery for namespaced keyword access

Description: When the compiler sees a keyword at the beginning of a sexpr, (:foo x), it emits some machinery that takes into account that 'x' could be a defrecord with a defined 'foo' field. This exists to fast-path it into a field lookup. Here is the supporting code from the target defrecord: https://github.com/clojure/clojure/blob/master/src/clj/clojure/core_deftype.clj#L185-L198
The compiler currently emits the same machinery for (:foo/bar x), a namespaced keyword access, but defrecords don't have any fast path field access for that. This trivial patch turns that scenario into a normal invocation.

Here is the disassembly for (fn [x] (:foo/bar x))
https://gist.github.com/anonymous/d94fc56fba4a1665f73f

There are two static fields on the IFn also for every kw access.

With the trivial patch, it turns into a normal invoke. (emit the fn aka the namespaced keyword, then the args Aka the target, and call IFn invoke: kw.invoke(target))






[CLJ-1468] Add deep-merge and deep-merge-with Created: 18/Jul/14  Updated: 18/Jul/14  Resolved: 18/Jul/14

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

Type: Enhancement Priority: Minor
Reporter: Stuart Sierra Assignee: Unassigned
Resolution: Declined Votes: 1
Labels: None

Attachments: Text File CLJ-1468-deep-merge-01.patch    
Patch: Code and Test
Approval: Triaged

 Description   

When dealing with nested map structures, one often wants to merge two maps recursively.

The deep-merge-with function was originally written by Chris Houser for clojure.contrib.map-utils but was not maintained after clojure-contrib was split into separate modules.

deep-merge and deep-merge-with are widely copied, usually with the same implementation, in utility libraries. For example:



 Comments   
Comment by Rich Hickey [ 18/Jul/14 11:42 AM ]

Vague semantics and docs strings





[CLJ-1467] Implement Comparable in PersistentList Created: 17/Jul/14  Updated: 17/Jul/14

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

Type: Enhancement Priority: Major
Reporter: Pascal Germroth Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

PersistentVector implements Comparable already.






[CLJ-1466] clojure.core/bean should implement Iterable Created: 16/Jul/14  Updated: 07/Oct/14  Resolved: 07/Oct/14

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

Type: Defect Priority: Major
Reporter: Ambrose Bonnaire-Sergeant Assignee: Unassigned
Resolution: Completed Votes: 2
Labels: ft, interop

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

 Description   

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

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

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

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

Screened by: Alex Miller



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

One workaround:

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

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

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

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

(iterator [] (.iterator pmap)

to the bean proxy definition?

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

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

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

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

Added new patch that just adds iterator to bean.





[CLJ-1465] SubVector leaks memory by design (?) Created: 14/Jul/14  Updated: 14/Jul/14  Resolved: 14/Jul/14

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

Type: Defect Priority: Minor
Reporter: Szymon Witamborski Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: memory


 Description   

While reading through the code of SubVector class, I've noticed that the only thing that it does is creating a "subview" of an existing vector without doing anything to elements that are not accessible any more. Please correct me if I'm wrong but I think this leads to memory leaks in this scenario:

(let [a [1 2 3 4 5]
      b (subvec a 2 3)]
  b)
[3]

In this example we no longer have any reference to a once the (let...) expression returned. Elements of a that are no longer accessible will not be garbage collected until b is garbage collected because b still holds a reference to a (the v field in SubVector class). Ideally, these elements should be garbage collectible as soon as a is gone.



 Comments   
Comment by Alex Miller [ 14/Jul/14 2:40 PM ]

The subvec docstring says:

"Returns a persistent vector of the items in vector from start (inclusive) to end (exclusive). If end is not supplied, defaults to (count vector). This operation is O(1) and very fast, as the resulting vector shares structure with the original and no trimming is done."

The implementation is intentional to make this a constant-time operation. If you are willing to make the tradeoff re shared structure and object retention, this constant operation has better performance. In other words: working as intended.





[CLJ-1464] Incorrectly named parameter to fold function in reducers.clj Created: 12/Jul/14  Updated: 10/Sep/14  Resolved: 10/Sep/14

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

Type: Defect Priority: Trivial
Reporter: Jason Jackson Assignee: Unassigned
Resolution: Declined Votes: 0
Labels: None

Attachments: Text File 0001-Rename-a-function-parameter-to-reflect-the-fold-func.patch    
Patch: Code
Approval: Triaged

 Description   

https://github.com/clojure/clojure/blob/master/src/clj/clojure/core/reducers.clj#L95
The 2-arity fold accepts reducef as parameter and then uses it as a combinef.
Instead it should accept combinef as parameter and then use it as a reducef, as every combine fn (monoid) is a reduce fn, but not every reduce fn is a combine fn (it's not associative).



 Comments   
Comment by Jason Jackson [ 12/Jul/14 2:58 PM ]

this is my first patch for clojure please double check everything. CA is done.

Comment by Andy Fingerhut [ 12/Jul/14 7:29 PM ]

Everything gets double checked whether it is your first patch or your 50th

At least as far as the format of the patch being correct, that it applies cleanly to the latest version of Clojure on the master branch, compiles and passes all tests cleanly, all of that is good.

Whether there is interest in taking your proposed change is to be decided by others. It may be some time before it is examined further.

Comment by Jozef Wagner [ 13/Jul/14 4:11 AM ]

This is not a defect. Quoting Rich, "If no combining fn is supplied, the reducing fn is used." (source)

There are three user supplied operations in fold: getting identity element (combinef :: -> T), reducing function (reducef :: T * E -> T) and combining function (combinef :: T * T -> T). For reduce, combining function is not needed but the rest two operations are needed. Thus reducing function (reducef) supplies identity element for reducers and only in folders the identity element is produced by combining function. In case where reducing fn is used for both reducing and combining, it must of course be associative and must handle objects of types T and E as a second argument.

Comment by Jason Jackson [ 14/Jul/14 12:14 AM ]

@Jozef I appreciate the feedback I still think my patch is correct, although I admit everyone's time is better spent not debating this small refactoring so feel free to close it.

One perspective to view my patch from is if we had protocols p/Monoid and p/Reducer. It's possible to reify any object that implements p/Monoid into p/Reducer, but not the other way around since not every p/Reducer is associative.

Comment by Jason Jackson [ 14/Jul/14 12:30 AM ]

From that perspective you could also say that in the 2-arity case the parameter "reducef" requires objects that implement both p/Monoid and p/Reducer, but in the 3-arity case the parameter "reducef" only requires p/Reducer

Comment by Jozef Wagner [ 14/Jul/14 1:38 AM ]

Note that reducef and combinef take different type of second argument, so not every combining function can be used as a reducing one. Your proposal is thus no better than the status quo. Consider following example:

(fold clojure.set/union conj [1 1 2 2 3 3 4 4])




[CLJ-1463] Providing own ClassLoader for eval is broken Created: 10/Jul/14  Updated: 10/Jul/14

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

Type: Defect Priority: Minor
Reporter: Volkert Oakley Jurgens Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler
Environment:

Clojure 1.6.0



 Description   

clojure.lang.Compiler has a method with the signature

public static Object eval(Object form, boolean freshLoader)

but the freshLoader argument is ignored since https://github.com/clojure/clojure/commit/2c2ed386ed0f6f875342721bdaace908e298c7f3

Is there a good reason this still needs to be "hotfixed" like this?

We would like to provide our own ClassLoader for eval to manage the lifecycle of the generated classes.






[CLJ-1462] cl-format throws ClassCastException: Writer cannot be cast to Future/IDeref Created: 07/Jul/14  Updated: 09/Jul/14

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

Type: Defect Priority: Minor
Reporter: Pascal Germroth Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: print


 Description   

Using ~I and ~_ etc fails in many situations, the most trivial one being:

Clojure 1.6.0 and 1.5.1:

user=> (clojure.pprint/cl-format true "~I")
ClassCastException java.io.PrintWriter cannot be cast to java.util.concurrent.Future  clojure.core/deref-future (core.clj:2180)
user=> (clojure.pprint/cl-format nil "~I")
ClassCastException java.io.StringWriter cannot be cast to java.util.concurrent.Future  clojure.core/deref-future (core.clj:2180)
user=> (clojure.pprint/cl-format nil "~_")
ClassCastException java.io.StringWriter cannot be cast to java.util.concurrent.Future  clojure.core/deref-future (core.clj:2180)

Clojure 1.4.0

user=> (clojure.pprint/cl-format true "~I")
ClassCastException java.io.OutputStreamWriter cannot be cast to clojure.lang.IDeref  clojure.core/deref (core.clj:2080)
user=> (clojure.pprint/cl-format nil "~I")
ClassCastException java.io.StringWriter cannot be cast to clojure.lang.IDeref  clojure.core/deref (core.clj:2080)
user=> (clojure.pprint/cl-format nil "~_")
ClassCastException java.io.StringWriter cannot be cast to clojure.lang.IDeref  clojure.core/deref (core.clj:2080)

These work in other implementations, i.e. clisp, creating empty output in these trivial cases:

> (format t "~I")
NIL
> (format nil "~I")
""
> (format nil "~_")
""


 Comments   
Comment by Andy Fingerhut [ 07/Jul/14 11:01 AM ]

The tilde-underscore sequence is for "conditional newline", according to the CLHS here: http://www.lispworks.com/documentation/lw51/CLHS/Body/22_cea.htm

Tilde-capital-letter-I is for indent: http://www.lispworks.com/documentation/lw51/CLHS/Body/22_cec.htm

Comment by Pascal Germroth [ 07/Jul/14 12:09 PM ]

Ah, didn't think to try that. It fails without cl-format as well:

user=> (clojure.pprint/pprint-newline :linear)
ClassCastException java.io.PrintWriter cannot be cast to java.util.concurrent.Future  clojure.core/deref-future (core.clj:2180)
user=> (clojure.pprint/pprint-indent :block 0)
ClassCastException java.io.PrintWriter cannot be cast to java.util.concurrent.Future  clojure.core/deref-future (core.clj:2180)

Manually creating a pretty writer does work though:

user=> (binding [*out* (clojure.pprint/get-pretty-writer *out*)] (clojure.pprint/pprint-newline :linear))
nil

In the get-pretty-writer doc it says:

Generally, it is unnecessary to call this function, since pprint,
write, and cl-format all call it if they need to.

Which appears to not be true for cl-format, and it would be nice if it would be applied automatically for all functions that need a pretty writer.

Comment by Pascal Germroth [ 09/Jul/14 6:37 PM ]

More bad news!
Manually creating a pretty-writer doesn't do the trick either, because it is not being properly flushed:

user=> (binding [*out* (get-pretty-writer *out*)] (cl-format true "hello ~_world~%"))
hello world
nil
user=> (binding [*out* (get-pretty-writer *out*)] (cl-format true "hello ~_world"))
hellonil
user=> (binding [*out* (get-pretty-writer *out*)] (cl-format true "hello ~_world") (.ppflush *out*))
hello worldnil

The ~% inserts an unconditional newline like \n, which also works as expected.

Insert ~_ before and it only prints up to that one. But I've also managed to get it to abort at other ~_ s, maybe because other commands flushed it.

Manually flushing it, like the inexplicably private with-pretty-writer macro does works though.
I don't understand why get-pretty-writer is exposed but not the macro that is needed to use it properly. Also all functions using pretty-writer facilities should use with-pretty-writer, that's what it appears to be specifically designed for. Then there's no need to expose it (or get-pretty-writer).





[CLJ-1461] print-dup is broken for some clojure collections Created: 06/Jul/14  Updated: 06/Jul/14

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

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections

Approval: Triaged

 Description   
user=> (print-dup (sorted-set 1) *out*)
#=(clojure.lang.PersistentTreeSet/create [1])nil
user=> (read-string (with-out-str (print-dup (sorted-set 1) *out*)))
ClassCastException Cannot cast clojure.lang.PersistentVector to clojure.lang.ISeq  java.lang.Class.cast (Class.java:3258)
user=> (print-dup (subvec [1] 0) *out*)
#=(clojure.lang.APersistentVector$SubVector/create [1])nil
user=> (read-string (with-out-str (print-dup (subvec [1] 0) *out*)))
IllegalArgumentException No matching method found: create  clojure.lang.Reflector.invokeMatchingMethod (Reflector.java:53)

print-dup assumes all IPersistentCollections not defined via defrecord have a static /create method that take an IPersistentCollection, but this is not true for many clojure collections






[CLJ-1460] Clojure transforms literals of custom IPersistentCollections not created via deftype/defrecord to their generic clojure counterpart Created: 06/Jul/14  Updated: 06/Jul/14

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

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

Approval: Triaged

 Description   
user=> (class (eval (sorted-map 1 1)))
clojure.lang.PersistentArrayMap ;; expected: clojure.lang.PersistentTreeMap


 Comments   
Comment by Alex Miller [ 06/Jul/14 5:35 PM ]

Seems related to CLJ-1093.

Comment by Nicola Mometto [ 06/Jul/14 5:51 PM ]

The symptoms are indeed similar but there are differences: CLJ-1093 affects all empty IPersistentCollections, this one affects all {ISeq,IPersistentList,IPersistentMap,IPersistentVector,IPersistentSet} collections that are not IRecord/IType.





[CLJ-1459] records should support transient Created: 05/Jul/14  Updated: 06/Jul/14

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

Type: Enhancement Priority: Major
Reporter: Yongqian Li Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: defrecord


 Description   

user=> (defrecord R [a])
user.R
user=> (transient (->R nil))
ClassCastException user.R cannot be cast to clojure.lang.IEditableCollection clojure.core/transient (core.clj:3060)






[CLJ-1458] Use transients in merge and merge-with Created: 04/Jul/14  Updated: 14/Sep/14

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

Type: Enhancement Priority: Minor
Reporter: Yongqian Li Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: newbie, performance

Attachments: File transient-merge.diff    
Patch: Code
Approval: Triaged

 Description   

It would be nice if merge used transients.



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

I will take a crack at a patch today.

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

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

Three potential issues:

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

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

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

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





[CLJ-1457] once the compiler pops the dynamic classloader from the stack, attempts to read record reader literals will fail Created: 30/Jun/14  Updated: 27/Oct/14

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

Type: Defect Priority: Minor
Reporter: Kevin Downey Assignee: Unassigned
Resolution: Unresolved Votes: 4
Labels: None

Approval: Triaged

 Description   

reproduction case

java -jar target/clojure-1.7.0-master-SNAPSHOT.jar -e "(do (ns foo.bar) (defrecord Foo []) (defn -main [] (prn (->Foo)) (read-string \"#foo.bar.Foo[]\")))" -m foo.bar

result

#'foo.bar/-main
#foo.bar.Foo{}
Exception in thread "main" java.lang.ClassNotFoundException: foo.bar.Foo
	at java.net.URLClassLoader$1.run(URLClassLoader.java:372)
	at java.net.URLClassLoader$1.run(URLClassLoader.java:361)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.net.URLClassLoader.findClass(URLClassLoader.java:360)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:424)
	at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:308)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
	at java.lang.Class.forName0(Native Method)
	at java.lang.Class.forName(Class.java:340)
	at clojure.lang.RT.classForNameNonLoading(RT.java:2076)
	at clojure.lang.LispReader$CtorReader.readRecord(LispReader.java:1195)
	at clojure.lang.LispReader$CtorReader.invoke(LispReader.java:1164)
	at clojure.lang.LispReader$DispatchReader.invoke(LispReader.java:609)
	at clojure.lang.LispReader.read(LispReader.java:183)
	at clojure.lang.RT.readString(RT.java:1737)
	at clojure.core$read_string.invoke(core.clj:3497)
	at foo.bar$_main.invoke(NO_SOURCE_FILE:1)
	at clojure.lang.Var.invoke(Var.java:375)
	at clojure.lang.AFn.applyToHelper(AFn.java:152)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.core$apply.invoke(core.clj:624)
	at clojure.main$main_opt.invoke(main.clj:315)
	at clojure.main$main.doInvoke(main.clj:420)
	at clojure.lang.RestFn.invoke(RestFn.java:457)
	at clojure.lang.Var.invoke(Var.java:394)
	at clojure.lang.AFn.applyToHelper(AFn.java:165)
	at clojure.lang.Var.applyTo(Var.java:700)
	at clojure.main.main(main.java:37)

what happens is the evaluator pushes a dynamicclassloader, evaluates some code, then -m foo.bar causes foo.bar/-main to be called, which tries to read in a literal for the just defined record, but it fails because when foo.bar/-main is called clojure.lang.Compiler/LOADER is unbound so RT uses the sun.misc classloader to try and find the class, which it knows nothing about



 Comments   
Comment by Kevin Downey [ 01/Jul/14 11:42 AM ]

this means that you cannot depend on ever being able to deserialize a record with read unless you are at the repl (the only place clojure.lang.Compiler/LOADER is guaranteed to be bound).

1. print/read support for records is broken
2. behavior is inconsistent between the repl and other environments
which will drive people crazy when the try to figure out why their code isn't working

Comment by Alex Miller [ 01/Jul/14 4:43 PM ]

I would appreciate more understanding about how this affects code run in a more normal scenario (than calling clojure.main with -e and -m).

Comment by Kevin Downey [ 22/Aug/14 4:24 PM ]

https://gist.githubusercontent.com/anonymous/bafde69c99e0be63988d/raw/736d14d98030f48b6a65ca0bfdc3c81fb44e1789/gistfile1.txt is an hour long irc log where someone was having a problem after they switched their app from aot compilation to launch via -m, which I tracked down to this issue.

Comment by David Pidcock [ 27/Oct/14 8:24 PM ]

This also affects me running
lein ring server

I threw this test code in my handler.clj

(defrecord Test [id])

(def example-test (pr-str (->Test 1)))

(defroutes app-routes
(GET "/test" [] (read-string example-test)))

When I run this in the REPL, it works.
But I get ClassNotFound when I browse to "/test".

I thought I'd done something stupid, until I found this bug. (Well - I could still be doing something stupid, of course).





[CLJ-1456] The compiler ignores too few or too many arguments to throw Created: 30/Jun/14  Updated: 03/Oct/14

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

Type: Enhancement Priority: Major
Reporter: Alf Kristian Støyle Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler, ft

Attachments: Text File 0001-CLJ-1456-counting-forms-to-catch-malformed-throw-for.patch     Text File 0001-CLJ-1456-counting-forms-to-catch-malformed-throw-for.patch     Text File v3_0001-CLJ-1456-counting-forms-to-catch-malformed-throw-for.patch    
Patch: Code and Test
Approval: Triaged

 Description   

The compiler does not fail on "malformed" throw forms:

user=> (defn foo [] (throw))
#'user/foo

user=> (foo)
NullPointerException   user/foo (NO_SOURCE_FILE:1)

user=> (defn bar [] (throw Exception baz))
#'user/bar

user=> (bar)
ClassCastException java.lang.Class cannot be cast to java.lang.Throwable  user/bar (NO_SOURCE_FILE:1)

; This one works, but ignored-symbol, should probably not be ignored
user=> (defn quux [] (throw (Exception. "Works!") ignored-symbol))
#'user/quux

user=> (quux)
Exception Works!  user/quux (NO_SOURCE_FILE:1)

The compiler can easily avoid these by counting forms.

Patch: v3_0001-CLJ-1456-counting-forms-to-catch-malformed-throw-for.patch

Screened by: Alex Miller



 Comments   
Comment by Alf Kristian Støyle [ 30/Jun/14 11:56 AM ]

Not sure how to create a test for the attached patch. Will happily do so if anyone has a suggestion.

Comment by Alex Miller [ 30/Jun/14 12:23 PM ]

Re testing, I think the examples you give are good - you should add tests to test/clojure/test_clojure/compilation.clj that eval the form and expect compilation errors. I'm sure you can find similar examples.

Comment by Alf Kristian Støyle [ 30/Jun/14 2:01 PM ]

Newest patch also contains a few tests.

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

All patches dated Jun 30 2014 and earlier no longer applied cleanly to latest master after some commits were made to Clojure on Aug 29, 2014. They did apply cleanly before that day.

I have not checked how easy or difficult it might be to update this patch. See section "Updating Stale Patches" on this wiki page for some tips on updating patches: http://dev.clojure.org/display/community/Developing+Patches

Alf, it can help avoid confusion if different patches have different file names. JIRA lets you create multiple attachments with the same name, but I wouldn't recommend it.

Comment by Alf Kristian Støyle [ 30/Aug/14 2:18 AM ]

It was easy to fix the patch. Uploaded the new patch v3_0001-CLJ-1456-counting-forms-to-catch-malformed-throw-for.patch, which applies cleanly to the current master.





[CLJ-1455] Postcondition in defrecord: Compiler unable to resolve symbol % Created: 28/Jun/14  Updated: 29/Jun/14

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

Type: Enhancement Priority: Minor
Reporter: Phill Wolf Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: defrecord


 Description   

Clojure's postconditions[1] are a splendiferous, notationally
idiot-proof way to scrutinize a function's return value without
inadvertently causing it to return something else.

Functions (implementing protocols) for a record type may be defined in
its defrecord or with extend-type. In functions defined in
extend-type, postconditions work as expected. Therefore, it is a
surprise that functions defined in defrecord cannot use
postconditions.

Actually it appears defrecord sees a pre/postcondition map as ordinary
code, so the postcondition runs at the beginning of the function (not
the end) and the symbol % (for return value) is not bound.

The code below shows a protocol and two record types that implement
it. Type "One" has an in-the-defrecord function definition where the
postcondition does not compile. Type "Two" uses extend-type and the
postcondition works as expected.

Unable to find source-code formatter for language: clojure. Available languages are: javascript, sql, xhtml, actionscript, none, html, xml, java
(defprotocol ITimesThree
  (x3 [a]))

;; defrecord with functions inside cannot use postconditions.
(defrecord One
    []
  ITimesThree
  (x3 [a]
    {:pre [(do (println "One x3 pre") 1)] ;; (works fine)
     :post [(do (println "One x3 post, %=" %) 1)]
     ;; Unable to resolve symbol: % in this context.
     ;; With % removed, it compiles but runs at start, not end.
     }
    (* 1 3)))

;; extend-type can add functions with postconditions to a record.
(defrecord Two
    [])
(extend-type Two
  ITimesThree
  (x3 [a]
    {:pre [(do (println "Two x3 pre") 1)] ;; (works fine)
     :post [(do (println "Two x3 post, %=" %) 1)] ;; (works fine)
     }
    (* 2 3)))

(defn -main
  "Main"
  []
  (println (x3 (->One)))
  (println (x3 (->Two))))

[1] http://clojure.org/special_forms, in the fn section.






[CLJ-1454] Companion to swap! which returns the old value Created: 28/Jun/14  Updated: 10/Sep/14

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

Type: Enhancement Priority: Major
Reporter: Philip Potter Ass