<< Back to previous view

[CLJ-47] GC Issue 43: Dead code in generated bytecode Created: 17/Jun/09  Updated: 26/Aug/13

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

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

Approval: Vetted

 Description   
Reported by Levente.Santha, Jan 11, 2009
The bug was described in detail in this thread: http://groups.google.com/
group/clojure/browse_thread/thread/81ba15d7e9130441

For clojure.core$last__2954.invoke the correct bytecode would be (notice 
the removed "goto    65" after "41:  goto    0"):

public java.lang.Object invoke(java.lang.Object)   throws 
java.lang.Exception;
  Code:
   0:   getstatic       #22; //Field const__0:Lclojure/lang/Var;
   3:   invokevirtual   #37; //Method clojure/lang/Var.get:()Ljava/lang/
Object;
   6:   checkcast       #39; //class clojure/lang/IFn
   9:   aload_1
   10:  invokeinterface #41,  2; //InterfaceMethod clojure/lang/IFn.invoke:
(Ljava/lang/Object;)Ljava/lang/Object;
   15:  dup
   16:  ifnull  44
   19:  getstatic       #47; //Field java/lang/Boolean.FALSE:Ljava/lang/
Boolean;
   22:  if_acmpeq       45
   25:  getstatic       #22; //Field const__0:Lclojure/lang/Var;
   28:  invokevirtual   #37; //Method clojure/lang/Var.get:()Ljava/lang/
Object;
   31:  checkcast       #39; //class clojure/lang/IFn
   34:  aload_1
   35:  invokeinterface #41,  2; //InterfaceMethod clojure/lang/IFn.invoke:
(Ljava/lang/Object;)Ljava/lang/Object;
   40:  astore_1
   41:  goto    0
   44:  pop
   45:  getstatic       #26; //Field const__1:Lclojure/lang/Var;
   48:  invokevirtual   #37; //Method clojure/lang/Var.get:()Ljava/lang/
Object;
   51:  checkcast       #39; //class clojure/lang/IFn
   54:  aload_1
   55:  aconst_null
   56:  astore_1
   57:  invokeinterface #41,  2; //InterfaceMethod clojure/lang/IFn.invoke:
(Ljava/lang/Object;)Ljava/lang/Object;
   62:  areturn

Our JIT reported incorrect stack size along the basic block introduced by 
the unneeded goto.
The bug was present in SVN rev 1205.


 Comments   
Comment by Assembla Importer [ 08/Oct/10 10:21 AM ]

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

Comment by Assembla Importer [ 08/Oct/10 10:21 AM ]

richhickey said: Updating tickets (#8, #19, #30, #31, #126, #17, #42, #47, #50, #61, #64, #69, #71, #77, #79, #84, #87, #89, #96, #99, #103, #107, #112, #113, #114, #115, #118, #119, #121, #122, #124)

Comment by Assembla Importer [ 08/Oct/10 10:21 AM ]

aredington said: This appears to still be a problem with the generated bytecode in 1.3.0. Examining the bytecode for last, the problem has moved to invokeStatic:

<pre>
public static java.lang.Object invokeStatic(java.lang.Object) throws java.lang.Exception;
Code:
0: aload_0
1: invokestatic #50; //Method clojure/core$next.invokeStatic:(Ljava/lang/Object;)Ljava/lang/Object;
4: dup
5: ifnull 25
8: getstatic #56; //Field java/lang/Boolean.FALSE:Ljava/lang/Boolean;
11: if_acmpeq 26
14: aload_0
15: invokestatic #50; //Method clojure/core$next.invokeStatic:(Ljava/lang/Object;)Ljava/lang/Object;
18: astore_0
19: goto 0
22: goto 30
25: pop
26: aload_0
27: invokestatic #59; //Method clojure/core$first.invokeStatic:(Ljava/lang/Object;)Ljava/lang/Object;
30: areturn
</pre>

Line number 22 is an unreachable goto given the prior goto on line 19.





[CLJ-415] smarter assert (prints locals) Created: 29/Jul/10  Updated: 03/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: errormsgs

Attachments: Text File clj-415-assert-prints-locals-v1.txt    
Approval: Vetted
Waiting On: Rich Hickey

 Description   

Here is an implementation you can paste into a repl. Feedback wanted:

(defn ^{:private true} local-bindings
  "Produces a map of the names of local bindings to their values."
  [env]
  (let [symbols (map key env)]
    (zipmap (map (fn [sym] `(quote ~sym)) symbols) symbols)))

(defmacro assert
  "Evaluates expr and throws an exception if it does not evaluate to
 logical true."
  {:added "1.0"}
  [x]
  (when *assert*
    (let [bindings (local-bindings &env)]
      `(when-not ~x
         (let [sep# (System/getProperty "line.separator")]
           (throw (AssertionError. (apply str "Assert failed: " (pr-str '~x) sep#
                                          (map (fn [[k# v#]] (str "\t" k# " : " v# sep#)) ~bindings)))))))))


 Comments   
Comment by Assembla Importer [ 24/Aug/10 5:41 PM ]

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

Comment by Assembla Importer [ 24/Aug/10 5:41 PM ]

alexdmiller said: A simple example I tried for illustration:

user=> (let [a 1 b 2] (assert (= a b)))
#<CompilerException java.lang.AssertionError: Assert failed: (= a b)
 a : 1
 b : 2
Comment by Assembla Importer [ 24/Aug/10 5:41 PM ]

fogus said: Of course it's weird if you do something like:

(let [x 1 y 2 z 3 a 1 b 2 c 3] (assert (= x y)))
java.lang.AssertionError: Assert failed: (= x y)
 x : 1
 y : 2
 z : 3
 a : 1
 b : 2
 c : 3
 (NO_SOURCE_FILE:0)
</code></pre>

So maybe it could be slightly changed to:
<pre><code>(defmacro assert
  "Evaluates expr and throws an exception if it does not evaluate to logical true."
  {:added "1.0"}
  [x]
  (when *assert*
    (let [bindings (local-bindings &env)]
      `(when-not ~x
         (let [sep#  (System/getProperty "line.separator")
               form# '~x]
           (throw (AssertionError. (apply str "Assert failed: " (pr-str form#) sep#
                                          (map (fn [[k# v#]] 
                                                 (when (some #{k#} form#) 
                                                   (str "\t" k# " : " v# sep#))) 
                                               ~bindings)))))))))
</code></pre>

So that. now it's just:
<pre><code>(let [x 1 y 2 z 3 a 1 b 2 c 3] (assert (= x y)))
java.lang.AssertionError: Assert failed: (= x y)
 x : 1
 y : 2
 (NO_SOURCE_FILE:0)

:f

Comment by Assembla Importer [ 24/Aug/10 5:41 PM ]

fogus said: Hmmm, but that fails entirely for: (let [x 1 y 2 z 3 a 1 b 2 c 3] (assert (= [x y] [a c]))). So maybe it's better just to print all of the locals unless you really want to get complicated.
:f

Comment by Assembla Importer [ 24/Aug/10 5:41 PM ]

jawolfe said: See also some comments in:

http://groups.google.com/group/clojure-dev/browse_frm/thread/68d49cd7eb4a4899/9afc6be4d3f8ae27?lnk=gst&q=assert#9afc6be4d3f8ae27

Plus one more suggestion to add to the mix: in addition to / instead of printing the locals, how about saving them somewhere. For example, the var assert-bindings could be bound to the map of locals. This way you don't run afoul of infinite/very large sequences, and allow the user to do more detailed interrogation of the bad values (especially useful when some of the locals print opaquely).

Comment by Assembla Importer [ 24/Aug/10 5:41 PM ]

stuart.sierra said: Another approach, which I wil willingly donate:
http://github.com/stuartsierra/lazytest/blob/master/src/main/clojure/lazytest/expect.clj

Comment by Jeff Weiss [ 15/Dec/10 1:33 PM ]

There's one more tweak to fogus's last comment, which I'm actually using. You need to flatten the quoted form before you can use 'some' to check whether the local was used in the form:

(defmacro assert
  "Evaluates expr and throws an exception if it does not evaluate to logical true."
  {:added "1.0"}
  [x]
  (when *assert*
    (let [bindings (local-bindings &env)]
      `(when-not ~x
         (let [sep#  (System/getProperty "line.separator")
               form# '~x]
           (throw (AssertionError. (apply str "Assert failed: " (pr-str form#) sep#
                                          (map (fn [[k# v#]] 
                                                 (when (some #{k#} (flatten form#)) 
                                                   (str "\t" k# " : " v# sep#))) 
                                               ~bindings)))))))))
Comment by Stuart Halloway [ 04/Jan/11 8:31 PM ]

I am holding off on this until we have more solidity around http://dev.clojure.org/display/design/Error+Handling. (Considering, for instance, having all exceptions thrown from Clojure provide access to locals.)

When my pipe dream fades I will come back and screen this before the next release.

Comment by Stuart Halloway [ 28/Jan/11 1:14 PM ]

Why try to guess what someone wants to do with the locals (or any other context, for that matter) when you can specify a callback (see below). This would have been useful last week when I had an assertion that failed only on the CI box, where no debugger is available.

Rich, at the risk of beating a dead horse, I still think this is a good idea. Debuggers are not always available, and this is an example of where a Lisp is intrinsically capable of providing better information than can be had in other environments. If you want a patch for the code below please mark waiting on me, otherwise please decline this ticket so I stop looking at it.

(def ^:dynamic *assert-handler* nil)

(defn ^{:private true} local-bindings
  "Produces a map of the names of local bindings to their values."
  [env]
  (let [symbols (map key env)]
    (zipmap (map (fn [sym] `(quote ~sym)) symbols) symbols)))

(defmacro assert
  [x]
  (when *assert*
    (let [bindings (local-bindings &env)]
      `(when-not ~x
         (let [sep#  (System/getProperty "line.separator")
               form# '~x]
           (if *assert-handler*
             (*assert-handler* form# ~bindings)
             (throw (AssertionError. (apply str "Assert failed: " (pr-str form#) sep#
                                            (map (fn [[k# v#]] 
                                                   (when (some #{k#} (flatten form#)) 
                                                     (str "\t" k# " : " v# sep#))) 
                                                 ~bindings))))))))))
Comment by Jeff Weiss [ 27/May/11 8:16 AM ]

A slight improvement I made in my own version of this code: flatten does not affect set literals. So if you do (assert (some #{x} [a b c d])) the value of x will not be printed. Here's a modified flatten that does the job:

(defn symbols [sexp]
  "Returns just the symbols from the expression, including those
   inside literals (sets, maps, lists, vectors)."
  (distinct (filter symbol? (tree-seq coll? seq sexp))))
Comment by Andy Fingerhut [ 18/Nov/12 1:06 AM ]

Attaching git format patch clj-415-assert-prints-locals-v1.txt of Stuart Halloway's version of this idea. I'm not advocating it over the other variations, just getting a file attached to the JIRA ticket.





[CLJ-445] Method/Constructor resolution does not factor in widening conversion of primitive args Created: 29/Sep/10  Updated: 27/Jul/13

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

Type: Enhancement Priority: Major
Reporter: Alexander Taggart Assignee: Alexander Taggart
Resolution: Unresolved Votes: 3
Labels: None

Attachments: Text File clj-445-prim-conversion-update-2-patch.txt     Text File prim-conversion.patch     Text File prim-conversion-update-1.patch     Text File reorg-reflector.patch    
Approval: Vetted

 Description   

Problem:
When making java calls (or inlined functions), if both args and param are primitive, no widening conversion is used to locate the proper overloaded method/constructor.

Examples:

user=> (Integer. (byte 0))
java.lang.IllegalArgumentException: No matching ctor found for class java.lang.Integer (NO_SOURCE_FILE:0)
</code></pre>
The above occurs because there is no Integer(byte) constructor, though it should match on Integer(int).
<pre><code>user=> (bit-shift-left (byte 1) 1)
Reflection warning, NO_SOURCE_PATH:3 - call to shiftLeft can't be resolved.
2

In the above, a call is made via reflection to Numbers.shiftLeft(Object, Object) and its associated auto-boxing, instead of directly to the perfectly adequate Numbers.shiftLeft(long, int).

Workarounds:
Explicitly casting to the formal type.

Ancillary benefits of fixing:
It would also reduce the amount of method overloading, e.g., RT.intCast(char), intCast(byte), intCast(short), could all be removed, since such calls would pass to RT.intCast(int).



 Comments   
Comment by Assembla Importer [ 23/Oct/10 6:43 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/445
Attachments:
fixbug445.diff - https://www.assembla.com/spaces/clojure/documents/b6gDSUZOur36b9eJe5cbCb/download/b6gDSUZOur36b9eJe5cbCb

Comment by Assembla Importer [ 23/Oct/10 6:43 PM ]

ataggart said: [file:b6gDSUZOur36b9eJe5cbCb]

Comment by Assembla Importer [ 23/Oct/10 6:43 PM ]

ataggart said: Also fixes #446.

Comment by Stuart Halloway [ 03/Dec/10 12:50 PM ]

The patch is causing a test failure

[java] Exception in thread "main" java.lang.IllegalArgumentException: 
     More than one matching method found: equiv, compiling:(clojure/pprint/cl_format.clj:428)

Can you take a look?

Comment by Stuart Halloway [ 28/Jan/11 12:30 PM ]

The failing test happens when trying to find the correct equiv for signature (Number, long). Is the compiler wrong to propose this signature, or is the resolution method wrong in not having an answer? (It thinks two signatures are tied: (Object, long) and (Number, Number).)

Exception in thread "main" java.lang.IllegalArgumentException: More than one matching method found: equiv, compiling:(clojure/pprint/cl_format.clj:428)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6062)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6050)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.access$100(Compiler.java:35)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5492)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2372)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$InvokeExpr.parse(Compiler.java:3277)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6057)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5231)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5527)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5231)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2385)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5231)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5527)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$IfExpr$Parser.parse(Compiler.java:2385)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5231)
	at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5527)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5231)
	at clojure.lang.Compiler$FnMethod.parse(Compiler.java:4667)
	at clojure.lang.Compiler$FnExpr.parse(Compiler.java:3397)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6053)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6043)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.access$100(Compiler.java:35)
	at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:480)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	at clojure.lang.Compiler.analyze(Compiler.java:5866)
	at clojure.lang.Compiler.analyze(Compiler.java:5827)
	at clojure.lang.Compiler.eval(Compiler.java:6114)
	at clojure.lang.Compiler.load(Compiler.java:6545)
	at clojure.lang.RT.loadResourceScript(RT.java:340)
	at clojure.lang.RT.loadResourceScript(RT.java:331)
	at clojure.lang.RT.load(RT.java:409)
	at clojure.lang.RT.load(RT.java:381)
	at clojure.core$load$fn__1427.invoke(core.clj:5308)
	at clojure.core$load.doInvoke(core.clj:5307)
	at clojure.lang.RestFn.invoke(RestFn.java:409)
	at clojure.pprint$eval3969.invoke(pprint.clj:46)
	at clojure.lang.Compiler.eval(Compiler.java:6110)
	at clojure.lang.Compiler.load(Compiler.java:6545)
	at clojure.lang.RT.loadResourceScript(RT.java:340)
	at clojure.lang.RT.loadResourceScript(RT.java:331)
	at clojure.lang.RT.load(RT.java:409)
	at clojure.lang.RT.load(RT.java:381)
	at clojure.core$load$fn__1427.invoke(core.clj:5308)
	at clojure.core$load.doInvoke(core.clj:5307)
	at clojure.lang.RestFn.invoke(RestFn.java:409)
	at clojure.core$load_one.invoke(core.clj:5132)
	at clojure.core$load_lib.doInvoke(core.clj:5169)
	at clojure.lang.RestFn.applyTo(RestFn.java:143)
	at sun.reflect.GeneratedMethodAccessor11.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
	at java.lang.reflect.Method.invoke(Method.java:597)
	at clojure.lang.Reflector.invokeMatchingMethod(Reflector.java:77)
	at clojure.lang.Reflector.invokeInstanceMethod(Reflector.java:28)
	at clojure.core$apply.invoke(core.clj:602)
	at clojure.core$load_libs.doInvoke(core.clj:5203)
	at clojure.lang.RestFn.applyTo(RestFn.java:138)
	at sun.reflect.GeneratedMethodAccessor11.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
	at java.lang.reflect.Method.invoke(Method.java:597)
	at clojure.lang.Reflector.invokeMatchingMethod(Reflector.java:77)
	at clojure.lang.Reflector.invokeInstanceMethod(Reflector.java:28)
	at clojure.core$apply.invoke(core.clj:604)
	at clojure.core$use.doInvoke(core.clj:5283)
	at clojure.lang.RestFn.invoke(RestFn.java:409)
	at clojure.main$repl.doInvoke(main.clj:196)
	at clojure.lang.RestFn.invoke(RestFn.java:422)
	at clojure.main$repl_opt.invoke(main.clj:267)
	at clojure.main$main.doInvoke(main.clj:362)
	at clojure.lang.RestFn.invoke(RestFn.java:409)
	at clojure.lang.Var.invoke(Var.java:401)
	at clojure.lang.AFn.applyToHelper(AFn.java:163)
	at clojure.lang.Var.applyTo(Var.java:518)
	at clojure.main.main(main.java:37)
Caused by: java.lang.IllegalArgumentException: More than one matching method found: equiv
	at clojure.lang.Reflector.getMatchingParams(Reflector.java:639)
	at clojure.lang.Reflector.getMatchingParams(Reflector.java:578)
	at clojure.lang.Reflector.getMatchingMethod(Reflector.java:569)
	at clojure.lang.Compiler$StaticMethodExpr.<init>(Compiler.java:1439)
	at clojure.lang.Compiler$HostExpr$Parser.parse(Compiler.java:896)
	at clojure.lang.Compiler.analyzeSeq(Compiler.java:6055)
	... 115 more
Comment by Alexander Taggart [ 08/Feb/11 6:27 PM ]

In working on implementing support for vararg methods, I found a number of flaws with the previous solutions. Please disregard them.

I've attached a single patch (reflector-compiler-numbers.diff) which is a rather substantial overhaul of the Reflector code, with some enhancements to the Compiler and Numbers code.

The patch notes:

  • Moved reflection functionality from Compiler to Reflector.
  • Reflector supports finding overloaded methods by widening conversion, boxing conversion, and casting.
  • During compilation Reflector attempts to find best wildcard match.
  • Reflector refers to *unchecked-math* when reflectively invoking methods and constructors.
  • Both Reflector and Compiler support variable arity java methods and constructor; backwards compatible with passing an array or nil in the vararg slot.
  • Added more informative error messages to Reflector.
  • Added tests to clojure.test-clojure.reflector.
  • Altered overloaded functions of clojure.lang.Numbers to service Object/double/long params; fixes some ambiguity issues and avoids unnecessary boxing in some cases.
  • Patch closes issues 380, 440, 445, 666, and possibly 259 (not enough detail provided).
Comment by Alexander Taggart [ 10/Feb/11 7:35 PM ]

Updated patch to fix a bug where a concrete class with multiple identical Methods (e.g., one from an interface, another from an abstract class) would result in ambiguity. Now resolved by arbitrary selection (this is what the original code did as well albeit not explicitly).

Comment by Alexander Taggart [ 25/Feb/11 9:29 PM ]

Updated patch to work with latest master branch.

Comment by Stuart Halloway [ 06/Mar/11 1:54 PM ]

patch appears to be missing test file clojure/test_clojure/reflector.clj.

Comment by Alexander Taggart [ 06/Mar/11 2:39 PM ]

Bit by git.

Patch corrected to contain clojure.test-clojure.reflector.

Comment by Stuart Halloway [ 11/Mar/11 10:30 AM ]

Rich: I verified that the patch applied but reviewed only briefly, knowing you will want to go over this one closely.

Comment by Stuart Halloway [ 11/Mar/11 10:55 AM ]

After applying this patch, I am getting method missing errors:

java.lang.NoSuchMethodError: clojure.lang.Numbers.lt(JLjava/lang/Object;)

but only when using compiled code, e.g. the same code works in the REPL and then fails after compilation. Haven't been able to isolate an example that I can share here yet, but hoping this will cause someone to have an "a, hah" moment...

Comment by Alexander Taggart [ 02/Apr/11 12:55 PM ]

The patch now contains only the minimal changes needed to support widening conversion. Cleanup of Numbers overloads, etc., can wait until this patch gets applied. The vararg support is in a separate patch on CLJ-440.

Comment by Christopher Redinger [ 15/Apr/11 12:50 PM ]

Please test patch

Comment by Christopher Redinger [ 21/Apr/11 11:02 AM ]

FYI: Patch applies cleanly on master and all tests pass as of 4/21 (2011)

Comment by Christopher Redinger [ 22/Apr/11 9:57 AM ]

This work is too big to take into the 1.3 beta right now. We'll revisit for a future release.

Comment by Alexander Taggart [ 28/Apr/11 1:19 PM ]

To better facilitate understanding of the changes, I've broken them up into two patches, each with a number of isolable, incremental commits:

reorg-reflector.patch: Moves the reflection/invocation code from Compiler to Reflector, and eliminates redundant code. The result is a single code base for resolving methods/constructors, which will allow for altering that mechanism without excess external coordination. This contains no behaviour changes.

prim-conversion.patch: Depends on the above. Alters the method/constructor resolution process:

  • more consistent with java resolution, especially when calling pre-1.5 APIs
  • adds support for widening conversion of primitive numerics of the same category (this is more strict than java, and more clojuresque)
  • adds support for wildcard matches at compile-time (i.e., you don't need to type-hint every arg to avoid reflection).

This also provides a base to add further features, e.g., CLJ-666.

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

It's documented in situ, but here are the conversion rules that the reflector uses to find methods:

  1. By Type:
    • object to ancestor type
    • primitive to a wider primitive of the same numeric category (new)
  2. Boxing:
    • boxed number to its primitive
    • boxed number to a wider primitive of the same numeric category (new for Short and Byte args)
    • primitive to its boxed value
    • primitive to Number or Object (new)
  3. Casting:
    • long to int
    • double to float
Comment by Alexander Taggart [ 10/May/11 3:13 PM ]

prim-conversion-update-1.patch applies to current master.

Comment by Alexander Taggart [ 11/May/11 2:15 PM ]

Created CLJ-792 for the reflector reorg.

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

prim-conversion-update-1.patch does not apply as of f5bcf64.

Is CLJ-792 now a prerequisite of this ticket?

Comment by Alexander Taggart [ 17/Feb/12 3:15 PM ]

Yes, after the original patch was deemed "too big".

After this much time with no action from TPTB, feel free to kill both tickets.

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

Again, not sure if this is any help, but I've tested starting from Clojure head as of Feb 20, 2012, applying clj-792-reorg-reflector-patch2.txt attached to CLJ-792, and then applying clj-445-prim-conversion-update-2-patch.txt attached to this ticket, and the result compiles and passes all but 2 tests. I don't know whether those failures are easy to fix or not, or whether issues may have been introduced with these patches.





[CLJ-274] cannot close over mutable fields (in deftype) Created: 23/Feb/10  Updated: 03/Sep/13

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

Type: Defect Priority: Major
Reporter: Anonymous Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: deftype

Approval: Vetted

 Description   

Simplest case:

user=>
(deftype Bench [#^{:unsynchronized-mutable true} val]
Runnable
(run [_]
(fn [] (set! val 5))))

java.lang.IllegalArgumentException: Cannot assign to non-mutable: val (NO_SOURCE_FILE:5)

Functions should be able to mutate mutable fields in their surrounding deftype (just like inner classes do in Java).

Filed as bug, because the loop special form expands into a fn form sometimes:

user=>
(deftype Bench [#^{:unsynchronized-mutable true} val]
Runnable
(run [_]
(let [x (loop [] (set! val 5))])))
java.lang.IllegalArgumentException: Cannot assign to non-mutable: val (NO_SOURCE_FILE:9)



 Comments   
Comment by Assembla Importer [ 01/Oct/10 9:35 AM ]

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

Comment by Assembla Importer [ 01/Oct/10 9:35 AM ]

donmullen said: Updated each run to [_] for new syntax.

Now gives exception listed.

Comment by Assembla Importer [ 01/Oct/10 9:35 AM ]

richhickey said: We're not going to allow closing over mutable fields. Instead we'll have to generate something other than fn for loops et al used as expressions. Not going to come before cinc





[CLJ-348] reify allows use of qualified name as method parameter Created: 13/May/10  Updated: 26/Jul/13

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

Type: Defect Priority: Minor
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Approval: Vetted

 Description   

This should complain about using a fully-qualified name as a parameter:

(defmacro lookup []
`(reify clojure.lang.ILookup
(valAt [_ key])))

Instead it simply ignores that parameter in the method body in favour of clojure.core/key.



 Comments   
Comment by Assembla Importer [ 24/Aug/10 8:03 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/348
Attachments:
0001-Add-a-test-for-348-reify-shouldn-t-accept-qualified-.patch - https://www.assembla.com/spaces/clojure/documents/d2xUJIxTyr36fseJe5cbLA/download/d2xUJIxTyr36fseJe5cbLA

Comment by Assembla Importer [ 24/Aug/10 8:03 AM ]

technomancy said: [file:d2xUJIxTyr36fseJe5cbLA]: A test to expose the unwanted behaviour

Comment by Assembla Importer [ 24/Aug/10 8:03 AM ]

richhickey said: I'm not sure the bug is what you say it is, or the resolution should be what you suggest. The true problem is the resolution of key when qualified. Another possibility is to ignore the qualifier there.

Comment by Assembla Importer [ 24/Aug/10 8:03 AM ]

technomancy said: Interesting. So it's not appropriate to require auto-gensym here? Why are the rules different for reify methods vs proxy methods?

> (defmacro lookup []
`(proxy [clojure.lang.ILookup] []
(valAt [key] key)))
> (lookup)

Can't use qualified name as parameter: clojure.core/key
[Thrown class java.lang.Exception]





[CLJ-326] add :as-of option to refer Created: 30/Apr/10  Updated: 26/Jul/13

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

Type: Enhancement Priority: Minor
Reporter: Christophe Grand Assignee: Christophe Grand
Resolution: Unresolved Votes: 0
Labels: None

Approval: Vetted

 Description   

Discussed here: http://groups.google.com/group/clojure-dev/msg/74af612909dcbe56

:as-of allows library authors to specify a known subset of vars to refer from clojure (or any other library which would use :added metadata).

(ns foo (:refer-clojure :as-of "1.1")) is equivalent to (ns foo (:refer-clojure :only [public-documented-vars-which-already-existed-in-1.1]))



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

Converted from http://www.assembla.com/spaces/clojure/tickets/326
Attachments:
add-as-of-to-refer.patch - https://www.assembla.com/spaces/clojure/documents/a8SumUvcOr37SmeJe5cbLA/download/a8SumUvcOr37SmeJe5cbLA

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

cgrand said: [file:a8SumUvcOr37SmeJe5cbLA]: requires application of #325

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

richhickey said: Do we still need this?





[CLJ-731] Create macro to variadically unroll a combinator function definition Created: 26/Jan/11  Updated: 26/Jul/13

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

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

Approval: Vetted

 Description   

Clojure contains a set of combinators that are implemented in a similar, but slightly different way. That is, they are implemented as a complete set of variadic overloads on both the call-side and also on the functions that they return. Visually, they all tend to look something like:

(defn foo
  ([f]
     (fn
       ([] do stuff...)
       ([x] do stuff...)
       ([x y] do stuff...)
       ([x y z] do stuff...)
       ([x y z & args] do variadic stuff...)))
  ([f1 f2]
     (fn
       ([] do stuff...)
       ([x] do stuff...)
       ([x y] do stuff...)
       ([x y z] do stuff...)
       ([x y z & args] do variadic stuff...)))
  ([f1 f2 f3]
     (fn
       ([] do stuff...)
       ([x] do stuff...)
       ([x y] do stuff...)
       ([x y z] do stuff...)
       ([x y z & args] do variadic stuff...)))
  ([f1 f2 f3 & fs]
     (fn
       ([] do stuff...)
       ([x] do stuff...)
       ([x y] do stuff...)
       ([x y z] do stuff...)
       ([x y z & args] do variadic stuff...))))

To build this type of function for each combinator is tedious and error-prone.

There must be a way to implement a macro that takes a "specification" of a combinator including:

1. name
2. docstring
3. do stuff template
4. do variadic stuff template

And builds something like the function foo above. This macro should be able to implement the current batch of combinators (assuming that http://dev.clojure.org/jira/browse/CLJ-730 is completed first for the sake of verification).



 Comments   
Comment by Stuart Halloway [ 28/Jan/11 9:03 AM ]

This seems useful. Rich, would you accept a patch?

Comment by Stuart Halloway [ 28/Jan/11 9:40 AM ]

Nevermind, just saw that Rich already suggested this on the dev list. Patch away.





[CLJ-1104] Concurrent with-redefs do not unwind properly, leaving a var permanently changed Created: 07/Nov/12  Updated: 26/Jul/13

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

Type: Enhancement Priority: Minor
Reporter: Jason Wolfe Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Mac OS, Java 6


Attachments: Text File clj-1104-doc-unsafety-of-concurrent-with-redefs-v1.txt    
Patch: Code
Approval: Vetted

 Description   

On 1.4 and latest master:

user> (defn ten [] 10)
#'user/ten
user> (doall (pmap #(with-redefs [ten (fn [] %)] (ten)) (range 20 100)))
(20 21 22 23 24 25 34 27 28 29 30 31 32 33 34 35 36 37 38 39 39 35 42 43 44 45 48 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 79 87 88 89 90 91 92 93 94 95 97 92 98 99)
user> (ten)
79

Not sure if this is a bug per se, but the doc doesn't mention lack of concurrency safety and my expectation was that the original value would always be restored after any (arbitrarily interleaved) sequence of with-redefs calls.



 Comments   
Comment by Tim McCormack [ 07/Nov/12 8:50 PM ]

The with-redefs doc (v1.4.0) says "These temporary changes will be visible in all threads." That sounds non-thread-safe to me.

In general, changes to var root bindings are not thread safe.

Comment by Herwig Hochleitner [ 08/Nov/12 9:17 AM ]

As I understand it, with-redefs is mainly used in test suites to mock out vars. It was introduced when vars became static by default and a lot of testsuites had been using binding for mocking. Maybe the docstring should be amended with something along the lines of: When using this you have to ensure that only a single thread is interacting with redef'd vars.

Comment by Stuart Halloway [ 25/Nov/12 6:41 PM ]

Behavior find as is, doc string change would be fine.

Comment by Andy Fingerhut [ 25/Nov/12 6:57 PM ]

Patch clj-1104-doc-unsafety-of-concurrent-with-redefs-v1.txt dated Nov 25 2012 updates doc string of with-redefs to make it clear that concurrent use is unsafe.





[CLJ-84] GC Issue 81: compile gen-class fail when class returns self Created: 17/Jun/09  Updated: 27/Jul/13

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

Type: Defect Priority: Minor
Reporter: Assembla Importer Assignee: Rich Hickey
Resolution: Unresolved Votes: 0
Labels: None

Approval: Vetted
Waiting On: Chouser

 Description   
Reported by davidhaub, Feb 14, 2009

When attempting to compile the following program, clojure fails with a
ClassNotFoundException.  It occurs because one of the methods returns the
same class that is being generated.  If the returnMe method below is
changed to return an Object, the compile succeeds.

Beware when testing! If the classpath contains a class file (say from a
prior successful build when the returnMe method was changed to return an
object), the compile will succeed.  Always clear out the
clojure.compile.path prior to compiling.

;badgenclass.clj
(ns badgenclass
  (:gen-class
     :state state
     :methods
     [[returnMe [] badgenclass]]
     :init init))
(defn -init []
  [[] nil])

(defn -returnMe [this]
  this)

#!/bin/sh
rm -rf classes
mkdir classes
java -cp lib/clojure.jar:classes:. -Dclojure.compile.path=classes \
clojure.lang.Compile badgenclass


Comment 1 by chouser, Mar 07, 2009

Attached is a patch that accepts strings or symbols for parameter and return class
names, and generates the appropriate bytecode without calling Class/forName.  It
fixes this issue, but because 'ns' doesn't resolve :gen-class's arguments, class
names aren't checked as early anymore.  :gen-class-created classes with invalid
parameter or return types can even be instantiated, and no error will be reported
until the broken method is called.

One possible alternative would be to call Class/forName on any symbols given, but
allow strings to use the method given by this patch.  To return your own type, you'd
need a method defined like:

  [returnMe [] "badgenclass"]

Any thoughts?


 Comments   
Comment by Assembla Importer [ 28/Sep/10 5:09 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/84
Attachments:
genclass-allow-unresolved-classname.patch - https://www.assembla.com/spaces/clojure/documents/cWS6Aww30r3RbzeJe5afGb/download/cWS6Aww30r3RbzeJe5afGb

Comment by Assembla Importer [ 28/Sep/10 5:09 PM ]

oranenj said: [file:cWS6Aww30r3RbzeJe5afGb]: on comment 1

Comment by Assembla Importer [ 28/Sep/10 5:09 PM ]

richhickey said: Updating tickets (#8, #19, #30, #31, #126, #17, #42, #47, #50, #61, #64, #69, #71, #77, #79, #84, #87, #89, #96, #99, #103, #107, #112, #113, #114, #115, #118, #119, #121, #122, #124)

Comment by Stuart Halloway [ 30/Oct/10 11:58 AM ]

The approach take in the initial patch (delaying resolution of symbols into classes) is fine: gen-class makes no promise about when this happens, and the dynamic approach feels more consistent with Clojure. I think the proposed (but not implemented) use of string/symbol to control when class names are resolved is a bad idea: magical and not implied by the types.

Needed:

  • update the patch to apply cleanly
  • consider whether totype could live in clojure.reflect.java. (Beware load order dependencies.)
Comment by Chouser [ 30/Oct/10 9:29 PM ]

Wow, 18-month-old patch, back to haunt me for Halloway'een

So what does it mean that the assignee is Rich, but it's waiting on me?

Comment by Stuart Halloway [ 01/Nov/10 9:17 AM ]

I am using Approval = Incomplete plus Waiting On = Someone to let submitters know that there is feedback waiting, and that they can move the ticket forward by acting on it. The distinction is opportunity to contribute (something has been provided to let you move forward) vs. expectation of contribution.





[CLJ-291] (take-nth 0 coll) redux... Created: 08/Apr/10  Updated: 27/Jul/13

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

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

Approval: Vetted

 Description   

I dont seem to be able make the old ticket uninvalid so here goes
(take-nth 0 coll) causes (at least on Solaris) infinite space and time consumption
It's not a printout error as the following code causes the problem too

(let [j 0
firstprod (apply * (doall (map #(- 1 %) (take-nth j (:props mix)))))]) ; from my parameter update function

I used jvisualvm and the jvm is doing some RNI call - no clojure code is running at all
If left alone it will crash the jvm with all heap space consumed
0 is an InvalidArgument for take-nth
I wouldnt mind if it produced an infinite lazy sequence of nils even though thats wrong
It doesnt do this though it actively destroys the JVM
Its a bug nasty destructive and it took me half a day to figure out what was going on
please let someone fix it!



 Comments   
Comment by Assembla Importer [ 17/Oct/10 9:47 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/291
Attachments:
fixbug291.diff - https://www.assembla.com/spaces/clojure/documents/dfNhoS2Cir3543eJe5cbLA/download/dfNhoS2Cir3543eJe5cbLA

Comment by Assembla Importer [ 17/Oct/10 9:47 AM ]

bhurt said: Before this bug gets marked as invalid as well, let me point out that the problem here is that (take-nth 0 any-list) is a meaningless construction- the only question is what to do when this happens. IMHO, the correct behavior is to throw an exception.

Comment by Assembla Importer [ 17/Oct/10 9:47 AM ]

ataggart said: [file:dfNhoS2Cir3543eJe5cbLA]: throws IllegalArgumentException on negative step size

Comment by Stuart Halloway [ 29/Oct/10 10:36 AM ]

Does calling (take-nth 0 ...) cause the problem, or only realizing the result?

Comment by Chouser [ 29/Oct/10 11:06 AM ]

I'm not seeing a problem. Calling take-nth and even partially consuming the seq it returns works fine for me:

(take 5 (take-nth 0 [1 2 3]))
;=> (1 1 1 1 1)

Note however that it is returning an infinite lazy seq. The example in the issue description seems to include essentially (doall <infinite-lazy-seq>) which does blow the heap:

(doall (range))

This issue still strikes me as invalid.

Comment by Rich Hickey [ 29/Oct/10 11:06 AM ]

(take-nth 0 ...) returns an infinite sequence of the first item:

(take 12 (take-nth 0 [1 2 3]))
=> (1 1 1 1 1 1 1 1 1 1 1 1)

Is something other than this happening on Solaris?





[CLJ-69] GC Issue 66: Add "keyset" to Clojure; make .keySet for APersistentMap return IPersistentSet Created: 17/Jun/09  Updated: 27/Jul/13

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

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

Approval: Vetted

 Description   
Reported by wolfe.a.jason, Feb 04, 2009

Describe the feature/change.

Add "keyset" to Clojure; make .keySet for APersistentMap return an
IPersistentSet

Was this discussed on the group? If so, please provide a link to the
discussion:

http://groups.google.com/group/clojure/browse_thread/thread/66e708e477ae992f/ff3d8d588068b60e?hl=en#ff3d8d588068b60e

-----------------------------------------------------

A patch is attached.  Some notes:

I chose to add a "keyset" function, rather than change the existing "keys",
so as to avoid breaking anything.

The corresponding RT.keyset function just calls .keySet on the argument.
I would have liked to have "keyset" return an IPersistentSet even when
passed a (non-Clojure) java.util.Map, but this seems impossible to do in
sublinear time because of essentially the same limitation mentioned in the
above thread (the Map interface does not support getKey() or entryAt()) --
assuming, again, that "get" is supposed to return the actual (identical?)
key in a set, and not just an .equal key.

I then changed the implementation of .keySet for APersistentMap to
essentially copy APersistentSet.  A more concise alternative would have
been to extend APersistentSet and override the .get method, but that made
me a bit nervous (since if APeristentSet changed this could break). 

Anyway, this is my first patch for the Java side of Clojure, and I'm not
yet solid on the conventions and aesthetics, so
comments/questions/criticisms/requests for revisions are very welcome.


 Comments   
Comment by Assembla Importer [ 28/Sep/10 7:00 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/69
Attachments:
keyset.patch - https://www.assembla.com/spaces/clojure/documents/dKgE6mw3Gr3O2PeJe5afGb/download/dKgE6mw3Gr3O2PeJe5afGb

Comment by Assembla Importer [ 28/Sep/10 7:00 AM ]

oranenj said: [file:dKgE6mw3Gr3O2PeJe5afGb]

Comment by Assembla Importer [ 28/Sep/10 7:00 AM ]

richhickey said: Updating tickets (#8, #19, #30, #31, #126, #17, #42, #47, #50, #61, #64, #69, #71, #77, #79, #84, #87, #89, #96, #99, #103, #107, #112, #113, #114, #115, #118, #119, #121, #122, #124)

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

patch not in correct format





[CLJ-211] Support arbitrary functional destructuring via -> and ->> Created: 17/Nov/09  Updated: 27/Jul/13

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

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

Approval: Vetted

 Description   

Support arbitrary functional destructuring, that is the use of
any function in any destructuring form to help unpack data in
arbitrary ways.

The discussion began here:
http://clojure-log.n01se.net/date/2009-11-17.html#09:31c

The attached patch implements the spec described here:
http://clojure-log.n01se.net/date/2009-11-17.html#10:50a

That is, the following examples would now work:

user=> (let [(-> str a) 1] a)
"1"

user=> (let [[a (-> str b) c] [1 2]] (list a b c))
(1 "2" nil)

user=> (let [(->> (map int) [a b]) "ab"] (list a b))
(97 98)



 Comments   
Comment by Assembla Importer [ 28/Sep/10 6:57 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/211
Attachments:
destructuring-fns.diff - https://www.assembla.com/spaces/clojure/documents/aHWQ_W06Kr3O89eJe5afGb/download/aHWQ_W06Kr3O89eJe5afGb

Comment by Assembla Importer [ 28/Sep/10 6:57 AM ]

chouser@n01se.net said: [file:aHWQ_W06Kr3O89eJe5afGb]: [PATCH] Support -> and ->> in destructuring forms.

Comment by Assembla Importer [ 28/Sep/10 6:57 AM ]

cgrand said: I think the current patch suffers from the problem described here http://groups.google.com/group/clojure-dev/msg/80ba7fad2ff04708 too.

Comment by Assembla Importer [ 28/Sep/10 6:57 AM ]

richhickey said: so, don't use syntax-quote, just use clojure.core/->

Comment by Assembla Importer [ 28/Sep/10 6:57 AM ]

chouser@n01se.net said: Only -> and ->> are actually legal here anyway – if you've locally bound foo to -> there's not really any good reason to think (fn [(foo inc a)] a) should work. And if you've redefined -> or ->> to mean something else in your ns, do we need to catch that at compile time, or is it okay to emit the rearranged code and see what happens?

In short, would '#{> ->> clojure.core/> clojure.core/->>} be sufficient?

Comment by Assembla Importer [ 28/Sep/10 6:57 AM ]

cgrand said: Only -> and ->> are legal here but what if they are aliased or shadowed? Instead of testing the symboil per se I would check, if:

  • the symbol is not in &env
  • the symbol resolve to #'clojure.core/> or #'clojure.core/>>
(when-not (&env (first b)) (#{#'clojure.core/-> #'clojure.core/->>} (resolve (first b))))

but it requires to change destructure's sig to pass the env around

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

Rich: Are you assigned to this by accident? If so, please deassign yourself.





[CLJ-771] Move unchecked-prim casts to clojure.unchecked Created: 07/Apr/11  Updated: 03/Sep/13

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

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

Attachments: Text File clj-771-move-unchecked-casts-patch-v5.txt     Text File move-unchecked-casts.patch     Text File move-unchecked-casts-v2.patch    
Patch: Code and Test
Approval: Vetted
Waiting On: Rich Hickey

 Description   

Per Rich's comment in CLJ-767:

Moving unchecked coercions into unchecked ns is ok



 Comments   
Comment by Alexander Taggart [ 29/Apr/11 3:41 PM ]

Requires that patch on CLJ-782 be applied first.

Comment by Stuart Sierra [ 31/May/11 10:43 AM ]

Applies on master as of commit 66a88de9408e93cf2b0d73382e662624a54c6c86

Comment by Rich Hickey [ 09/Dec/11 8:40 AM ]

still considering when to incorporate this

Comment by John Szakmeister [ 19/May/12 9:36 AM ]

v2 of the patch applies to master as of commit eccde24c7fb63679f00c64b3c70c03956f0ce2c3

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

Patch clj-771-move-unchecked-casts-patch-v3.txt dated Sep 6 2012 is the same as Alexander Taggart's patch move-unchecked-casts.patch except that it has been updated to apply cleanly to latest Clojure master.

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

Patch clj-771-move-unchecked-casts-patch-v4.txt dated Oct 20 2012 is the same as Alexander Taggart's patch move-unchecked-casts.patch except that it has been updated to apply cleanly to latest Clojure master.

Comment by Andy Fingerhut [ 01/Jan/13 11:37 AM ]

The patch clj-771-move-unchecked-casts-patch-v4.txt applies cleanly to latest master and passes all tests. Rich marked this ticket as Incomplete on Dec 9 2011 with the comment "still considering when to incorporate this" above. Is it reasonable to change it back to Vetted or Screened so it can be considered again, perhaps after Release 1.5 is made?

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

Patch clj-771-move-unchecked-casts-patch-v5.txt dated Feb 12 2013 is the same as Alexander Taggart's patch move-unchecked-casts.patch except that it has been updated to apply cleanly to latest Clojure master.





[CLJ-346] (pprint-newline :fill) is not handled correctly Created: 12/May/10  Updated: 03/Sep/13

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

Type: Defect Priority: Minor
Reporter: Assembla Importer Assignee: Tom Faulhaber
Resolution: Unresolved Votes: 0
Labels: print

Approval: Vetted
Waiting On: Tom Faulhaber

 Description   

Filled pretty printing (where we try to fit as many elements on a line as possible) is being too aggressive as we can see when we try to print the following array:

user> (binding [*print-right-margin* 20] (pprint (int-array (range 10))))

Produces:

[0,
1,
2,
3,
4, 5, 6, 7, 8, 9]

Rather than

[0, 1, 2, 3, 4,
5, 6, 7, 8, 9]

or something like that. (I haven't worked through the exact correct representation for this case).

We currently only use :fill style newlines for native java arrays.



 Comments   
Comment by Assembla Importer [ 24/Aug/10 8:01 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/346
Attachments:
0347-pprint-update-2.diff - https://www.assembla.com/spaces/clojure/documents/diLxv6y4Sr35GVeJe5cbLr/download/diLxv6y4Sr35GVeJe5cbLr

Comment by Assembla Importer [ 24/Aug/10 8:01 AM ]

stu said: [file:diLxv6y4Sr35GVeJe5cbLr]

Comment by Assembla Importer [ 24/Aug/10 8:01 AM ]

stu said: The second patch includes the first, and adds another test.

Comment by Assembla Importer [ 24/Aug/10 8:01 AM ]

tomfaulhaber said: This patch was attached to the wrong bug. It should be attached to bug #347. There is no fix for this bug yet.

Comment by Rich Hickey [ 05/Nov/10 8:07 AM ]

Is this current?

Comment by Stuart Halloway [ 29/Nov/10 8:48 PM ]

Tom, this patch doesn't apply, and I am not sure why. Can you take a look?





[CLJ-250] debug builds Created: 27/Jan/10  Updated: 23/Oct/13

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

Type: Enhancement Priority: Minor
Reporter: Stuart Halloway Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: build

Approval: Vetted
Waiting On: Rich Hickey

 Description   

This ticket includes two patches:

  1. a patch to set assert when clojure.lang.RT loads, based on the presence of system property clojure.debug
  2. expand error messages in assert to include local-bindings</code> (a new macro which wraps the implicit <code>&env)

Things to consider before approving these patches:

  1. should there be an easy Clojure-level way to query if debug is enabled? (checking assert isn't the same, as debug should eventually drive other features)
  2. assertions will now be off by default – this is a change!
  3. is the addition of the name local-bindings to clojure.core cool?


 Comments   
Comment by Assembla Importer [ 24/Aug/10 6:05 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/250
Attachments:
add-clojure-debug-flag.patch - https://www.assembla.com/spaces/clojure/documents/aUWn50c64r35E-eJe5aVNr/download/aUWn50c64r35E-eJe5aVNr
assert-report-locals.patch - https://www.assembla.com/spaces/clojure/documents/aUWqLSc64r35E-eJe5aVNr/download/aUWqLSc64r35E-eJe5aVNr

Comment by Stuart Halloway [ 07/Dec/10 8:23 PM ]

Ignore the old patches. Considering the following implementation, please review and then move ticket to waiting on Stu:

  1. RT will check system property "clojure.debug", default to false
  2. property will set the root binding for the current *assert*, plus a new *debug* flag. (Debug builds can and will drive other things than just asserts.)
  3. does Compile.java need to push *assert* or *debug* as thread local bindings, or can they be root-bound only when compiling clojure?
  4. will add *debug* binding to clojure.main/with-bindings. Anywhere else?
  5. build.xml should not have to change – system properties will flow through (and build.xml may not be around much longer anyway)
  6. once we agree on the approach, I will ping maven plugin and lein owners so that they flow the setting through
  7. better assertion messages will be a separate ticket
  8. what is the interaction between *debug* and *unchecked-math*? Change checks to (and *unchecked-math* (not *debug*))}?
Comment by Rich Hickey [ 08/Dec/10 11:00 AM ]

#3 - root bound only
#4 - should not be in with-bindings for same reason as #3 - we don't want people to set! *debug* nor *assert*
#8 - yes, wrapping that in a helper fn

#6 - my biggest reservation is that this isn't yet informed by maven best practices

Comment by Stuart Sierra [ 08/Dec/10 2:09 PM ]

System properties can be passed through Maven, so I do not anticipate this being a problem.

However, I would prefer *assert* to remain true by default.

Comment by Chas Emerick [ 09/Dec/10 7:19 AM ]

SS is correct about this approach not posing any issue for Maven. In addition, the build could easily be set up to always emit two jars, one "normal", one "debug".

I'd suggest that, while clojure.debug might have broad effect, additional properties should be available to provide fine-grained control over each of the additional "debug"-related parameterizations that might become available in the future.


I'd like to raise a couple of potentially tangential concerns (after now thinking about assertions for a bit in the above context), some or all of which may simply be a result of my lack of understanding in various areas.

Looking at where assert is used in core.clj (only two places AFAICT: validating arguments to derive and checking pre- and post-conditions in fn), it would seem unwise to make it false by default. i.e. non-Named values would be able to get into hierarchies, and pre- and post-conditions would simply be ignored.

It's my understanding that assertions (talking here of the JVM construct, from which Clojure reuses AssertionError) should not be used to validate arguments to public API functions, or used to validate any aspect of a function's normal operation (i.e. "where not to use assertions"). That would imply that derive should throw IllegalArugmentException when necessary, and fn pre- and post-conditions should perhaps throw IllegalStateException – or, in any case, something other than AssertionError via assert. This would match up far better with most functions in core using assert-args rather than assert, the former throwing IllegalArgumentException rather than AssertionError.

That leads me to the question: is assert (and *assert*) intended to be a Clojure construct, or a quasi-interop form?

If the former, then it can roughly have whatever semantics we want, but then it seems like it should not be throwing AssertionError.

If the latter, then AssertionError is appropriate on the JVM, but then we need to take care that assertions can be enabled and disabled at runtime (without having to switch around different builds of Clojure), ideally using the host-defined switches (e.g. -ea and friends) and likely not anything like *assert*. I don't know if this is possible or practical at this point (I presume this would require nontrivial compiler changes).


Hopefully the above is not water under the bridge at this point. Thank you in advance for your forbearance.

Comment by Rich Hickey [ 09/Dec/10 8:08 AM ]

Thanks for the useful input Chas. Nothing is concluded yet. I think we should step back and look at the objective here, before moving forward with a solution. Being a dynamic language, there are many things we might want to validate about our programs, where the cost of checking is something we are unwilling to pay in production.

Being a macro, assert has the nice property that, should *assert* not be true during compilation, it generates nil, no conditional test at all. Thus right now it is more like static conditional compilation.

Java assert does have runtime toggle-ability, via -ea as you say. I haven't looked at the bytecode generated for Java asserts, but it might be possible for Clojure assert to do something similar, if the runtime overhead is negligible. It is quite likely that HotSpot has special code for eliding the assertion bytecode given a single check of some flag. I'm just not sure that flag is Class.desiredAssertionStatus.

Whether this turns into changes in assert or pre/post conditions, best practices etc is orthogonal and derived. Currently we don't have a facility to run without the checks. We need to choose between making them disappear during compilation (debug build) or runtime (track -ea) or both. Then we can look at how that is reflected in assert/pre-post and re-examine existing use of both. The "where not to use assertions" doc deals with them categorically, but in not considering their cost, seems unrealistic IMO.

I'd appreciate it if someone could look into how assert is generated and optimized by Java itself.

Comment by Chas Emerick [ 09/Dec/10 5:04 PM ]

Bytecode issues continue to be above my pay grade, unfortunately…

A few additional thoughts in response that you may or may not be juggling already:

assert being a macro makes total sense for what it's doing. Trouble is, "compile-time" is a tricky concept in Clojure: there's code-loading-time, AOT-compile-time, and transitively-AOT-compile-time. Given that, it's entirely possible for an application deployed to a production environment that contains a patchwork of code or no code produced by assert usages in various libraries and namespaces depending upon when those libs and ns' were loaded, AOT-compiled, or their dependents AOT-compiled, and the value of *assert* at each of those times. Of course, this is the case for all such macros whose results are dependent upon context-dependent state (I think this was a big issue with clojure.contrib.logging, making it only usable with log4j for a while).

What's really attractive about the JVM assertion mechanism is that it can be parameterized for a given runtime on a per-package basis, if desired. Reimplementing that concept so that assert can be *ns*-sensitive seems like it'd be straightforward, but the compile-time complexity already mentioned remains, and the idea of having two independently-controlled assertion facilities doesn't sound fun.

I know nearly nothing about the CLR, but it would appear that it doesn't provide for anything like runtime-controllable assertions.

Comment by Stuart Halloway [ 29/Dec/10 3:17 PM ]

The best (dated) evidence I could find says that the compiler sets a special class static final field $assertionsDisabled based on the return of desiredAssertionStatus. HotSpot doesn't do anything special with this, dead code elimination simply makes it go away. The code indeed compiles this way:

11: getstatic #6; //Field $assertionsDisabled:Z
14: ifne 33
17: lload_1
18: lconst_0
19: lcmp
20: ifeq 33
23: new #7; //class java/lang/AssertionError
26: dup
27: ldc #8; //String X should be zero
29: invokespecial #9; //Method java/lang/AssertionError."<init>":(Ljava/lang/Object;)V
32: athrow

Even if we were 100% sure that assertion removal was total, I would still vote for a separate Clojure-level switch, for the following reasons:

  1. I have a real and pressing need to disable some assertions, and I don't need the Java interop at all. Arguably others will be in the same boat.
  2. there will be multiple debugging facilities over time, and having a top-level debug switch is convenient for Clojure users.
  3. Java dis/enabling via command line flags is still possible as a separate feature. We could add this later as a (small) breaking change to our assert, or have a separate java-assert interop form. I am on the fence about which way to go here.
  4. I believe it is perfectly fine to throw an AssertionError from a non-Java-assertion-form. We don't believe in a world of a static exception hierarchy, and an assertion in production is a critical failure no matter what you call it. Even Scala does it http://daily-scala.blogspot.com/2010/03/assert-require-assume.html

Rich: awaiting your blessing to move forward on this.

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

The compiler sets $assertionsDisabled when, in static init code? Is there special support in the classloader for it? Is there a link to the best dated evidence you found?

Comment by Stuart Halloway [ 07/Jan/11 9:51 AM ]
  1. Yes, in static init code
  2. There is no special support in the classloader, per Brian Goetz (private correspondence) last week. But dead code elimination is great: "The run-time cost of disabled assertions should indeed be zero for compiled code"
Comment by Stuart Halloway [ 07/Jan/11 9:57 AM ]

Link: Google "java assert shirazi". (Not posting link because I can't tell in 10 secs whether it includes my session information.)

Comment by Alexander Kiel [ 14/Mar/13 1:28 PM ]

Is there anything new on this issue? I also look for a convenient way to disable assertions in production.

Comment by Michał Łopuszyński [ 11/Oct/13 4:21 AM ]

I am also interested in any news on this issue.
Convenient way to enable/disable assertions at runtime (preferably via -ea/-da options) would be a great feature!

Comment by Michał Łopuszyński [ 23/Oct/13 3:12 AM ]

Btw. there is a library for runtime-toggable assertions available via clojars
https://github.com/pjstadig/assertions
This helped me a great deal.





[CLJ-5] Unintuitive error response in clojure 1.0 Created: 17/Jun/09  Updated: 18/Apr/14

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

Type: Enhancement Priority: Minor
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs

Attachments: File clj-5-destructure-error.diff     Text File CLJ-5.patch    
Patch: Code and Test
Approval: Vetted

 Description   

The following broken code:

(let [[x y] {}] x)

provides the following stack trace:

Exception in thread "main" java.lang.UnsupportedOperationException: nth not supported on this type: PersistentArrayMap (test.clj:0)
at clojure.lang.Compiler.eval(Compiler.java:4543)
at clojure.lang.Compiler.load(Compiler.java:4857)
at clojure.lang.Compiler.loadFile(Compiler.java:4824)
at clojure.main$load_script__5833.invoke(main.clj:206)
at clojure.main$script_opt__5864.invoke(main.clj:258)
at clojure.main$main__5888.doInvoke(main.clj:333)
at clojure.lang.RestFn.invoke(RestFn.java:413)
at clojure.lang.Var.invoke(Var.java:346)
at clojure.lang.AFn.applyToHelper(AFn.java:173)
at clojure.lang.Var.applyTo(Var.java:463)
at clojure.main.main(main.java:39)
Caused by: java.lang.UnsupportedOperationException: nth not supported on this type: PersistentArrayMap
at clojure.lang.RT.nth(RT.java:800)
at clojure.core$nth__3578.invoke(core.clj:873)
at user$eval__1.invoke(test.clj:1)
at clojure.lang.Compiler.eval(Compiler.java:4532)
... 10 more

The message "nth not supported on this type" while correct doesn't make the cause of the error very clear. Better error messages when destructuring would be very helpful.



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

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

Comment by Eugene Koontz [ 11/Nov/11 7:36 PM ]

Please see the attached patch which produces a (hopefully more clear) error message as shown below (given the broken code shown in the original bug report):

Clojure 1.4.0-master-SNAPSHOT
user=> (let [x 42 y 43] (+ x y))
85
user=> (let [[x y] {}] x)
UnsupportedOperationException left side of binding must be a symbol (found a PersistentVector instead).  clojure.lang.Compiler.checkLet (Compiler.java:6545)
user=>

In addition, this patch checks the argument of (let) as shown below:

user=> (let 42)
UnsupportedOperationException argument to (let)  must be a vector (found a Long instead).  clojure.lang.Compiler.checkLet (Compiler.java:6553)
Comment by Eugene Koontz [ 11/Nov/11 7:38 PM ]

Patch produced by doing git diff against commit ba930d95fc (master branch).

Comment by Eugene Koontz [ 13/Nov/11 11:24 PM ]

Sorry, this patch is wrong: it assumes that the left side of the binding is wrong - the [x y] in :

(let [[x y] {}] x)

because [x y] is a vector, when in fact, the left side is fine (per http://clojure.org/special_forms#let : "Clojure supports abstract structural binding, often called destructuring, in let binding lists".)

So it's the right side (the {}) that needs to be checked and flagged as erroneous, not the [x y].

Comment by Carin Meier [ 30/Nov/11 12:15 PM ]

Add patch better-error-for-let-vector-map-binding

This produces the following:

(let [[x y] {}] x)
Exception map binding to vector is not supported

There are other cases that are not handled by this though — like binding vector to a set

user=> (let [[x y] #{}] x)
UnsupportedOperationException nth not supported on this type: PersistentHashSet

Wondering if it might be better to try convert the map to a seq to support? Although this might be another issue.

Thoughts?

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

This seems too specific. Is this issue indicative of a larger problem that should be addressed? Even if this is the only case where bindings produce poor error messages, all the cases described above should be addressed in the patch.

Comment by Carin Meier [ 16/Dec/11 7:47 AM ]

Unfortunately, realized that this still does not cover the nested destructuring cases. Coming to the conclusion, that my approach above is not going to work for this.

Comment by Carin Meier [ 28/Apr/12 10:46 PM ]

File: clj-5-destructure-error.diff

Added support for nested destructuring errors

let [[[x1 y1][x2 y2]] [[1 2] {}]]
;=> UnsupportedOperationException let cannot destructure class clojure.lang.PersistentArrayMap.
Comment by Kevin Downey [ 18/Apr/14 1:45 AM ]

I am not wild about that error message, let can destructure a map fine.

If there error message were to change, I would prefer to get something like "sequential destructing not supported on maps".

I actually like the "nth not supported" error message, because it is exactly the problem, nth, used by sequential destructuring, doesn't work on maps.

it conveys exactly what the problem is if you know how destructing works and what nth means, where as "UnsupportedOperationException let cannot destructure class clojure.lang.PersistentArrayMap" seems misleading when you are in the know





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

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

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

Approval: Vetted

 Description   

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

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

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

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

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

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

Multimethod leak

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

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

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

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

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

(future (run-loop))

(reset! sleep-duration 0)

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

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

These lines will stop once the PermGen space fills up.

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

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

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

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

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

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

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

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

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

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

Protocol leak

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

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

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

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

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

(future (run-loop))

(reset! sleep-duration 0)

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

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

(-reset-methods ITake)

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



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

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

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

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

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

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

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

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

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

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

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

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





[CLJ-1250] Reducer (and folder) instances hold onto the head of seqs Created: 03/Sep/13  Updated: 18/Apr/14

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

Type: Defect Priority: Critical
Reporter: Christophe Grand Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: memory, reducers

Attachments: Text File after-change.txt     Text File before-change.txt     Text File CLJ-1250-20131211.patch     Text File CLJ-1250-AllInvokeSites-20140204.patch     Text File CLJ-1250-AllInvokeSites-20140320.patch    
Patch: Code
Approval: Vetted

 Description   

Problem Statement
A shared function #'clojure.core.reducers/reducer holds on to the head of a reducible collection, causing it to blow up when the collection is a lazy sequence.

Cause: #'reducer closes over a collection when in order to reify CollReduce, and the closed-over is never cleared. When code attempts to reduce over this anonymous transformed collection, it will realize the tail while the head is stored in the closed-over.

Reproduction steps:
Compare the following calls:

(time (reduce + 0 (map identity (range 1e8))))
(time (reduce + 0 (r/map identity (range 1e8))))

The second call should fail on a normal or small heap.

(If reducers are faster than seqs, increase the range.)

Approaches:

1) Reimplement the #'reducer (and #'folder) transformation fns similar to the manner that Christophe proposes here:

(defrecord Reducer [coll xf])

(extend-protocol 
  clojure.core.protocols/CollReduce
  Reducer
      (coll-reduce [r f1]
                   (clojure.core.protocols/coll-reduce r f1 (f1)))
      (coll-reduce [r f1 init]
                   (clojure.core.protocols/coll-reduce (:coll r) ((:xf r) f1) init)))

(def rreducer ->Reducer) 

(defn rmap [f coll]
  (rreducer coll (fn [g] 
                   (fn [acc x]
                     (g acc (f x))))))

Advantages: Relatively non-invasive change.
Disadvantages: Not evident code. Additional protocol dispatch, though only incurred once

2) Clear the reference to 'this' on the stack just before a tail call occurs

When a callee is in return position, clear the local variable reference to 'this' in the caller's stack frame, which will make the caller and all its closed-overs eligible for reclamation.

Patch 1211 takes this approach for InvokeExpr call sites in return position. Patch 1214 takes the same approach for InvokeExprs and also static and instance interop calls.

Here is the code that performs the clearing excerpted from the 1214 patch:

void emitClearThis(GeneratorAdapter gen) {
		gen.visitInsn(Opcodes.ACONST_NULL);
		gen.visitVarInsn(Opcodes.ASTORE, 0);
	}

Tail calls wrapped inside a try/catch/finally clause cannot have 'this' cleared, because closed-overs/locals may need to be emitted for exception handling blocks. Both patches consider and handle this edge case.

Advantages: Fixes this case with no user code changes. Enables GC to do reclaim closed-overs references earlier.
Disadvantages: A compiler change.

3) Alternate approach

from Christophe Grand:
Another way would be to enhance the local clearing mechanism to also clear "this" but it's complex since it may be needed to keep a reference to "this" for caches long after obvious references to "this" are needed.

Advantages: Fine-grained
Disadvantages: Complex, invasive, and the compiler is hard to hack on.

Mitigations
Avoid reducing on lazy-seqs and instead operate on vectors / maps, or custom reifiers of CollReduce or CollFold. This could be easier with some implementations of common collection functions being available (like iterate and partition).

See https://groups.google.com/d/msg/clojure-dev/t6NhGnYNH1A/2lXghJS5HywJ for previous discussion.



 Comments   
Comment by Gary Fredericks [ 03/Sep/13 8:53 AM ]

Fixed indentation in description.

Comment by Ghadi Shayban [ 11/Dec/13 11:08 PM ]

Adding a patch that clears "this" before tail calls. Verified that Christophe's repro case is fixed.

Will upload a diff of the bytecode soon.

Any reason this juicy bug was taken off 1.6?

Comment by Ghadi Shayban [ 11/Dec/13 11:17 PM ]

Here's the bytecode for the clojure.core.reducers/reducer reify before and after the change... Of course a straight diff isn't useful because all the line numbers changed. Kudos to Gary Trakhman for the no.disassemble lein plugin.

Comment by Christophe Grand [ 12/Dec/13 6:58 AM ]

Ghadi, I'm a bit surprised by this part of the patch: was the local clearing always a no-op here?

-		if(context == C.RETURN)
+		if(shouldClear)
 			{
-			ObjMethod method = (ObjMethod) METHOD.deref();
-			method.emitClearLocals(gen);
+                            gen.visitInsn(Opcodes.ACONST_NULL);
+                            gen.visitVarInsn(Opcodes.ASTORE, 0);
 			}

The problem with this approach (clear this on tail call) is that it adds yet another special case. To me the complexity stem from having to keep this around even if the user code doesn't refer to it.

Comment by Ghadi Shayban [ 12/Dec/13 7:19 AM ]

Thank you - I failed to mention this in the commit message: it appears that emitClearLocals() belonging to both ObjMethod and FnMethod (its child) are empty no-ops. I believe the actual local clearing is on line 4855.

I agree re: another special case in the compiler.

Comment by Alex Miller [ 12/Dec/13 8:56 AM ]

Ghadi re 1.6 - this ticket was never in the 1.6 list, it has not yet been vetted by Rich but is ready to do so when we open up again after 1.6.

Comment by Ghadi Shayban [ 12/Dec/13 8:59 AM ]

Sorry I confused the critical list with the Rel1.6 list.

Comment by Ghadi Shayban [ 14/Dec/13 11:16 AM ]

New patch 20131214 that handles all tail invoke sites (InvokeExpr + StaticMethodExpr + InstanceMethodExpr). 'StaticInvokeExpr' seems like an old remnant that had no active code path, so that was left as-is.

The approach taken is still the same as the original small patch that addressed only InvokeExpr, except that it is now using a couple small helpers. The commit message has more details.

Also a 'try' block with no catch or finally clause now becomes a BodyExpr. Arguably a user error, historically accepted, and still accepted, but now they are a regular BodyExpr, instead of being wrapped by a the no-op try/catch mechanism. This second commit can be optionally discarded.

With this patch on my machine (4/8 core/thread Ivy Bridge) running on bare clojure.main:
Christophe's test cases both run i 3060ms on a artificially constrained 100M max heap, indicating a dominant GC overhead. (But they now both work!)

When max heap is at a comfortable 2G the reducers version outpaces the lazyseq at 2100ms vs 2600ms!

Comment by Ghadi Shayban [ 13/Jan/14 10:48 AM ]

Updating stale patch after latest changes to master. Latest is CLJ-1250-AllInvokeSites-20140113

Comment by Ghadi Shayban [ 04/Feb/14 3:50 PM ]

Updating patch after murmur changes

Comment by Tassilo Horn [ 13/Feb/14 4:52 AM ]

Ghadi, I suffer from the problem of this issue. Therefore, I've applied your patch CLJ-1250-AllInvokeSites-20140204.patch to the current git master. However, then I get lots of "java.lang.NoSuchFieldError: array" errors when the clojure tests are run:

     [java] clojure.test-clojure.clojure-set
     [java] 
     [java] java.lang.NoSuchFieldError: array
     [java] 	at clojure.core.protocols$fn__6026.invoke(protocols.clj:123)
     [java] 	at clojure.core.protocols$fn__5994$G__5989__6003.invoke(protocols.clj:19)
     [java] 	at clojure.core.protocols$fn__6023.invoke(protocols.clj:147)
     [java] 	at clojure.core.protocols$fn__5994$G__5989__6003.invoke(protocols.clj:19)
     [java] 	at clojure.core.protocols$seq_reduce.invoke(protocols.clj:31)
     [java] 	at clojure.core.protocols$fn__6017.invoke(protocols.clj:48)
     [java] 	at clojure.core.protocols$fn__5968$G__5963__5981.invoke(protocols.clj:13)
     [java] 	at clojure.core$reduce.invoke(core.clj:6213)
     [java] 	at clojure.set$difference.doInvoke(set.clj:61)
     [java] 	at clojure.lang.RestFn.invoke(RestFn.java:442)
     [java] 	at clojure.test_clojure.clojure_set$fn__1050$fn__1083.invoke(clojure_set.clj:109)
     [java] 	at clojure.test_clojure.clojure_set$fn__1050.invoke(clojure_set.clj:109)
     [java] 	at clojure.test$test_var$fn__7123.invoke(test.clj:704)
     [java] 	at clojure.test$test_var.invoke(test.clj:704)
     [java] 	at clojure.test$test_vars$fn__7145$fn__7150.invoke(test.clj:721)
     [java] 	at clojure.test$default_fixture.invoke(test.clj:674)
     [java] 	at clojure.test$test_vars$fn__7145.invoke(test.clj:721)
     [java] 	at clojure.test$default_fixture.invoke(test.clj:674)
     [java] 	at clojure.test$test_vars.invoke(test.clj:718)
     [java] 	at clojure.test$test_all_vars.invoke(test.clj:727)
     [java] 	at clojure.test$test_ns.invoke(test.clj:746)
     [java] 	at clojure.core$map$fn__2665.invoke(core.clj:2515)
     [java] 	at clojure.lang.LazySeq.sval(LazySeq.java:40)
     [java] 	at clojure.lang.LazySeq.seq(LazySeq.java:49)
     [java] 	at clojure.lang.Cons.next(Cons.java:39)
     [java] 	at clojure.lang.RT.boundedLength(RT.java:1655)
     [java] 	at clojure.lang.RestFn.applyTo(RestFn.java:130)
     [java] 	at clojure.core$apply.invoke(core.clj:619)
     [java] 	at clojure.test$run_tests.doInvoke(test.clj:761)
     [java] 	at clojure.lang.RestFn.applyTo(RestFn.java:137)
     [java] 	at clojure.core$apply.invoke(core.clj:617)
     [java] 	at clojure.test.generative.runner$run_all_tests$fn__527.invoke(runner.clj:255)
     [java] 	at clojure.test.generative.runner$run_all_tests$run_with_counts__519$fn__523.invoke(runner.clj:251)
     [java] 	at clojure.test.generative.runner$run_all_tests$run_with_counts__519.invoke(runner.clj:251)
     [java] 	at clojure.test.generative.runner$run_all_tests.invoke(runner.clj:253)
     [java] 	at clojure.test.generative.runner$test_dirs.doInvoke(runner.clj:304)
     [java] 	at clojure.lang.RestFn.applyTo(RestFn.java:137)
     [java] 	at clojure.core$apply.invoke(core.clj:617)
     [java] 	at clojure.test.generative.runner$_main.doInvoke(runner.clj:312)
     [java] 	at clojure.lang.RestFn.invoke(RestFn.java:408)
     [java] 	at user$eval564.invoke(run_tests.clj:3)
     [java] 	at clojure.lang.Compiler.eval(Compiler.java:6657)
     [java] 	at clojure.lang.Compiler.load(Compiler.java:7084)
     [java] 	at clojure.lang.Compiler.loadFile(Compiler.java:7040)
     [java] 	at clojure.main$load_script.invoke(main.clj:274)
     [java] 	at clojure.main$script_opt.invoke(main.clj:336)
     [java] 	at clojure.main$main.doInvoke(main.clj:420)
     [java] 	at clojure.lang.RestFn.invoke(RestFn.java:408)
     [java] 	at clojure.lang.Var.invoke(Var.java:379)
     [java] 	at clojure.lang.AFn.applyToHelper(AFn.java:154)
     [java] 	at clojure.lang.Var.applyTo(Var.java:700)
     [java] 	at clojure.main.main(main.java:37)
Comment by Ghadi Shayban [ 13/Feb/14 8:23 AM ]

Can you give some details about your JVM/environment that can help reproduce? I'm not encountering this error.

Comment by Tassilo Horn [ 13/Feb/14 9:41 AM ]

Sure. It's a 64bit ThinkPad running GNU/Linux.

% java -version
java version "1.7.0_51"
OpenJDK Runtime Environment (IcedTea 2.4.5) (ArchLinux build 7.u51_2.4.5-1-x86_64)
OpenJDK 64-Bit Server VM (build 24.51-b03, mixed mode)
Comment by Ghadi Shayban [ 13/Feb/14 10:19 AM ]

Strange, that is exactly my mail env, OpenJDK7 on Arch, 64-bit. I have also tested on JDK 6/7/8 on OSX mavericks. Are you certain that the git tree is clean besides the patch? (Arch users unite!)

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

Yes, the tree is clean. But now I see that I get the same error also after resetting to origin/master, so it's not caused by your patch at all. Oh, now the error vanished after doing a `mvn clean`! So problem solved.

Comment by Nicola Mometto [ 19/Feb/14 12:32 PM ]

Ghandi, FnExpr.parse should bind IN_TRY_BLOCK to false before analyzing the fn body, consider the case

(try (do something (fn a [] (heap-consuming-op a))) (catch Exception e ..))

Here in the a function the this local will never be cleared even though it's perfectly safe to.
Admittedly this is an edge case but we should cover this possibility too.

Comment by Ghadi Shayban [ 19/Feb/14 2:06 PM ]

You may have auto-corrected my name to Ghandi instead of Ghadi. I wish I were that wise =)

I will update the patch for FnExpr (that seems reasonable), but maybe after 1.6 winds down and the next batch of tickets get scrutiny. It would be nice to get input on a preferred approach from Rich or core after it gets vetted – or quite possibly not vetted.

Comment by Nicola Mometto [ 19/Feb/14 6:11 PM ]

hah, sorry for the typo on the name

Seems reasonable to me, in the meantime I just pushed to tools.analyzer/tools.emitter complete support for "this" clearing, I'll test this a bit in the next few days to make sure it doesn't cause unexpected problems.

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

Patch CLJ-1250-AllInvokeSites-20140204.patch no longer applies cleanly to latest master as of Feb 23, 2014. It did on Feb 14, 2014. Most likely some of its context lines are changed by the commit to Clojure master on Feb 23, 2014 – I haven't checked in detail.

Comment by Ghadi Shayban [ 20/Mar/14 4:39 PM ]

Added a patch that 1) applies cleanly, 2) binds the IN_TRY_EXPR to false initially when analyzing FnExpr and 3) uses RT.booleanCast





[CLJ-1330] Class name clash between top-level functions and defn'ed ones Created: 22/Jan/14  Updated: 18/Apr/14

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

Type: Defect Priority: Critical
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 6
Labels: aot, compiler

Attachments: Text File 0001-Fix-CLJ-1330-make-top-level-named-functions-classnam.patch     File demo1.clj    
Patch: Code
Approval: Vetted

 Description   

Named anonymous fn's are not guaranteed to have unique class names when AOT-compiled.

For example:

(defn g [])
(def xx (fn g []))

When AOT-compiled both functions will emit user$g.class, the latter overwriting the former.

Demonstration script: demo1.clj

Patch: 0001-Fix-CLJ-1330-make-top-level-named-functions-classnam.patch

Approach: Generate unique class names for named fn's the same way as for unnamed anonymous fn's.

See also: This patch also fixes the issue reported in CLJ-1227.



 Comments   
Comment by Ambrose Bonnaire-Sergeant [ 22/Jan/14 11:12 AM ]

This seems like the reason why jvm.tools.analyzer cannot analyze clojure.core. On analyzing a definline, there is an "attempted duplicate class definition" error.

This doesn't really matter, but I thought it may or may not be useful information to someone.

Comment by Nicola Mometto [ 22/Jan/14 11:35 AM ]

Attached a fix.

This also fixes AOT compiling of code like:

(def x (fn foo []))
(fn foo [])
Comment by Nicola Mometto [ 22/Jan/14 11:39 AM ]

Cleaned up patch

Comment by Alex Miller [ 22/Jan/14 12:43 PM ]

It looks like the patch changes indentation of some of the code - can you fix that?

Comment by Nicola Mometto [ 22/Jan/14 3:57 PM ]

Updated patch without whitespace changes

Comment by Alex Miller [ 22/Jan/14 4:15 PM ]

Thanks, that's helpful.

Comment by Alex Miller [ 24/Jan/14 10:03 AM ]

There is consensus that this is a problem, however this is an area of the code with broad impacts as it deals with how classes are named. To that end, there is some work that needs to be done in understanding the impacts before we can consider it.

Some questions we would like to answer:

1) According to Rich, naming of (fn x []) function classes used to work in the manner of this patch - with generated names. Some code archaeology needs to be done on why that was changed and whether the change to the current behavior was addressing problems that we are likely to run into.

2) Are there issues with recursive functions? Are there impacts either in AOT or non-AOT use cases? Need some tests.

3) Are there issues with dynamic redefinition of functions? With the static naming scheme, redefinition causes a new class of the same name which can be picked up by reload of classes compiled to the old definition. With the dynamic naming scheme, redefinition will create a differently named class so old classes can never pick up a redefinition. Is this a problem? What are the impacts with and without AOT? Need some tests.

Comment by Nicola Mometto [ 24/Jan/14 11:39 AM ]

Looks like the current behaviour has been such since https://github.com/clojure/clojure/commit/4651e60808bb459355a3a5d0d649c4697c672e28

My guess is that Rich simply forgot to consider the (def y (fn x [] ..)) case.

Regarding 2 and 3, the dynamic naming scheme is no different than what happens for anonymous functions so I don't see how this could cause any issue.

Recursion on the fn arg is simply a call to .invoke on "this", it's classname unaware.

I can add some tests to test that

(def y (fn x [] 1))
and
(fn x [] 2)
compile to different classnames but other than that I don't see what should be tested.





[CLJ-1325] Add *warn-on-boxed-math* warning Created: 16/Jan/14  Updated: 15/Apr/14

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

Type: Enhancement Priority: Major
Reporter: Alex Miller Assignee: Alex Miller
Resolution: Unresolved Votes: 1
Labels: errormsgs, math

Attachments: File boxed.diff     Text File boxedmath.txt    
Patch: Code
Approval: Vetted

 Description   

Currently, it is difficult to tell that the compiler is using boxed math unless you look at the generated bytecode. Adding warn-on-boxed-math warning (similar in use to warn-on-reflection) to warn when these calls are being made.

Approach: In the compiler, when compiling a StaticMethodExpr that is specifying a call into certain boxed math methods on Number, and if the warning is enabled, print the warning. The list of calls would need to be in a boxed math "black list" which would probably be created manually.

Patch: The attached patch is an incomplete sketch of the solution. It is not yet specific enough in determining whether the particular call to Numbers is doing boxed math - it merely looks for an Object argument, which does catch a large set of the actual calls, but may also ensnare some calls that should not be included. One option would be to mark the offending methods in Numbers with an annotation that could be checked in isBoxedMath.

Screened by:



 Comments   
Comment by Alex Miller [ 14/Apr/14 10:56 PM ]

Moving to 1.7.

Comment by Alex Miller [ 15/Apr/14 10:17 AM ]

List of methods in Numbers and whether they should be considered "boxed math" or not, with some questions.





[CLJ-701] Compiler loses 'loop's return type in some cases Created: 03/Jan/11  Updated: 18/Apr/14

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

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

Clojure commit 9052ca1854b7b6202dba21fe2a45183a4534c501, version 1.3.0-master-SNAPSHOT


Approval: Vetted

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

Generates the following warnings:

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

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

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

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

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

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

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

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



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

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

so

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

gets converted into

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

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

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

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

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

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

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

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

[1] beware of CLJ-1111 when testing.

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

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





[CLJ-787] transient blows up when passed a vector created by subvec Created: 03/May/11  Updated: 18/Apr/14

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

Type: Defect Priority: Major
Reporter: Alexander Redington Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File CLJ-787-p1.patch    
Patch: Code and Test
Approval: Vetted

 Description   

Subvectors created with subvec from a PersistentVector cannot be made transient:

user=> (transient (subvec [1 2 3 4] 2))
ClassCastException clojure.lang.APersistentVector$SubVector cannot be cast to clojure.lang.IEditableCollection  clojure.core/transient (core.clj:2864)

Cause: APersistentVector$SubVector does not implement IEditableCollection

Patch: CLJ-787-p1.patch

Approach: Create a TransientSubVector based on an underlying TransientVector.

Two assumptions:

  • It's okay for TransientSubVector to delegate the ensureEditable functionality to the underlying TransientVector (sometimes explicitly, sometimes implicitly) - calling ensureEditable explicitly also requires that the field for the underlying vector be the concrete TransientVector type rather than the ITransientVector interface.
  • When an operation that would throw an exception on a PersistentVector happens from the wrong thread (or after persistent!), we throw that exception rather than the IllegalAccessError that transients throw when accessed inappropriately.


 Comments   
Comment by Stuart Sierra [ 31/May/11 9:28 AM ]

Confirmed. APersistentVector$SubVector does not implement IEditableCollection.

The current implementation of TransientVector depends on implementation details of PersistentVector, so it is not a trivial fix. The simplest fix might be to implement IEditableCollection.asTransient in SubVector by creating a new PersistentVector, but I do not know the performance implications.

Comment by Gary Fredericks [ 25/May/13 8:11 PM ]

We could get the same performance characteristics as SubVector by creating a TransientSubVector based on an underlying TransientVector, right?

Preparing a patch to that effect.

Comment by Gary Fredericks [ 25/May/13 10:58 PM ]

Text from the commit msg:

Made two assumptions:

  • It's okay for TransientSubVector to delegate the ensureEditable
    functionality to the underlying TransientVector (sometimes
    explicitely, sometimes implicitely) – calling ensureEditable
    explicitely also requires that the field for the underlying vector
    be the concrete TransientVector type rather than the
    ITransientVector interface.
  • When an operation that would throw an exception on a
    PersistentVector happens from the wrong thread (or after
    persistent!), we throw that exception rather than the
    IllegalAccessError that transients throw when accessed
    inappropriately.
Comment by Alex Miller [ 11/Oct/13 4:17 PM ]

I think there are some assumptions being made in this patch about the class structure here that do not hold. The structure is, admittedly, quite twisty.

A counter-example that highlights one of a few subtypes of APersistentVector that are not PersistentVector (like MapEntry):

user=> (transient (subvec (first {:a 1}) 0 1))
ClassCastException clojure.lang.MapEntry cannot be cast to clojure.lang.IEditableCollection  clojure.lang.APersistentVector$TransientSubVector.<init> (APersistentVector.java:592)

PersistentVector.SubVector expects to work on anything that implements IPersistentVector. Note that this includes concrete types such as MapEntry and LazilyPersistentVector, but could also be any user-implemented type IPersistentVector type too. TransientSubVector is making the assumption that the IPersistentVector in a SubVector question is also an IEditableCollection (that can be converted to be transient). Note that while PersistentVector implements TransientVector (and IEditableCollection), APersistentVector does not. To really implement this in tandem with SubVector, I think you would need to guarantee that IPersistentVector extended IEditableCollection and I don't think that's something we want to do.

I don't see an easy solution. Any time I see all these modifiers (Transient, Sub, etc) being created in different combinations, it is a clear sign that independent kinds of functionality are being remixed into single inheritance OO trees. You can see the same thing in most collection libraries (even Java's - need a ConcurrentIdentitySortedMap? too bad!).

Needs more thought.

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

Patch CLJ-787-p1.patch no longer applies cleanly to latest master, but it is only because of some new tests added to the transients.clj file since the patch was created, so it is trivial to update in that sense. Not updating it now due to other more significant issues with the patch described above.

Comment by Alex Miller [ 17/Jan/14 10:19 AM ]

No good solution to consider right now, removing from 1.6.





[CLJ-700] contains? broken for transient collections Created: 01/Jan/11  Updated: 18/Apr/14

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

Type: Defect Priority: Major
Reporter: Herwig Hochleitner Assignee: Unassigned
Resolution: Unresolved Votes: 8
Labels: None

Attachments: Java Source File 0001-Refactor-of-some-of-the-clojure-.java-code-to-fix-CL.patch     File clj-700-7.diff     File clj-700.diff     Text File clj-700-patch4.txt     Text File clj-700-patch6.txt    
Patch: Code and Test
Approval: Vetted

 Description   

(contains? (transient {:x "fine"}) :x)

false

also

(contains? (transient (hash-map :x "fine")) :x)

false

Patch: clj-700-7.diff



 Comments   
Comment by Herwig Hochleitner [ 01/Jan/11 8:01 PM ]

the same is also true for TransientVectors

{{(contains? (transient [1 2 3]) 0)}}

false

Comment by Herwig Hochleitner [ 01/Jan/11 8:25 PM ]

As expected, TransientSets have the same issue; plus an additional, probably related one.

(:x (transient #{:x}))

nil

(get (transient #{:x}) :x)

nil

Comment by Alexander Redington [ 07/Jan/11 2:07 PM ]

This is caused by expectations in clojure.lang.RT regarding the type of collections for some methods, e.g. contains() and getFrom(). Checking for contains looks to see if the instance passed in is Associative (a subinterface of PersistentCollection), or IPersistentSet.

This patch refactors several of the Clojure interfaces so that logic abstract from the issue of immutability is pulled out to a general interface (e.g. ISet, IAssociative), but preserves the contract specified (e.g. Associatives only return Associatives when calling assoc()).

With more general interfaces in place the contains() and getFrom() methods were then altered to conditionally use the general interfaces which are agnostic of persistence vs. transience. Includes tests in transients.clj to verify the changes fix this problem.

Comment by Stuart Halloway [ 28/Jan/11 10:35 AM ]

Rich: Patch doesn't currently apply, but I would like to get your take on approach here. In particular:

  1. this represents working back from the defect to rethinking abstractions (good!). Does it go far enough?
  2. what are good names for the interfaces introduced here?
Comment by Alexander Redington [ 25/Mar/11 7:44 AM ]

Rebased the patch off the latest pull of master as of 3/25/2011, it should apply cleanly now.

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

Latest patch does not apply as of f5bcf647

Comment by Andy Fingerhut [ 17/Feb/12 5:59 PM ]

clj-700-patch2.txt does patch cleanly to latest Clojure head as of a few mins ago. No changes to patch except in context around changed lines.

Comment by Andy Fingerhut [ 07/Mar/12 3:23 AM ]

Sigh. Git patches applied via 'git am' are fragile beasts indeed. Look at them the wrong way and they fail to apply.

clj-700-patch3.txt applies cleanly to latest master as of Mar 7, 2012, but not if you use this command:

git am -s < clj-700-patch3.txt

I am pretty sure this is because of DOS CR/LF line endings in the file src/jvm/clojure/lang/Associative.java. The patch does apply cleanly if you use this command:

git am --keep-cr -s < clj-700-patch3.txt

Comment by Andy Fingerhut [ 23/Mar/12 6:34 PM ]

This ticket was changed to Incomplete and waiting on Rich when Stuart Halloway asked for feedback on the approach on 28/Jan/2011. Stuart Sierra changed it to not waiting on Rich on 17/Feb/2012 when he noted the patch didn't apply cleanly. Latest patch clj-700-patch3.txt does apply cleanly, but doesn't change the approach used since the time Stuart Halloway's concern was raised. Should it be marked as waiting on Rich again? Something else?

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

Patch 4 incorporates patch 3, and brings it up to date on hashing (i.e. uses hasheq).

Comment by Andy Fingerhut [ 08/Jun/12 12:52 PM ]

Removed clj-700-patch3.txt in favor of Stuart Halloway's improved clj-700-patch4.txt dated June 8, 2012.

Comment by Andy Fingerhut [ 18/Jun/12 3:06 PM ]

clj-700-patch5.txt dated June 18, 2012 is the same as Stuart Halloway's clj-700-patch4.txt, except for context lines that have changed in Clojure master since Stuart's patch was created. clj-700-patch4.txt no longer applies cleanly.

Comment by Andy Fingerhut [ 19/Aug/12 4:47 AM ]

Adding clj-700-patch6.txt, which is identical to Stuart Halloway's clj-700-patch4.txt, except that it applies cleanly to latest master as of Aug 19, 2012. Note that as described above, you must use the --keep-cr option to 'git am' when applying this patch for it to succeed. Removing clj-700-patch5.txt, since it no longer applies cleanly.

Comment by Stuart Sierra [ 24/Aug/12 1:08 PM ]

Patch fails as of commit 1c8eb16a14ce5daefef1df68d2f6b1f143003140

Comment by Andy Fingerhut [ 24/Aug/12 1:53 PM ]

Which patch did you try, and what command did you use? I tried applying clj-700-patch6.txt to the same commit, using the following command, and it applied, albeit with the warning messages shown:

% git am --keep-cr -s < clj-700-patch6.txt
Applying: Refactor of some of the clojure .java code to fix CLJ-700.
/Users/jafinger/clj/latest-clj/clojure/.git/rebase-apply/patch:29: trailing whitespace.
public interface Associative extends IPersistentCollection, IAssociative{
warning: 1 line adds whitespace errors.
Applying: more CLJ-700: refresh to use hasheq

Note the --keep-cr option, which is necessary for this patch to succeed. It is recommended in the "Screening Tickets" section of the JIRA workflow wiki page here: http://dev.clojure.org/display/design/JIRA+workflow

Comment by Andy Fingerhut [ 28/Aug/12 5:48 PM ]

Presumptuously changing Approval from Incomplete back to None, since the latest patch does apply cleanly if the --keep-cr option is used. It was in Screened state recently, but I'm not so presumptuous as to change it to Screened

Comment by Alex Miller [ 19/Aug/13 12:26 PM ]

I think through a series of different hands on this ticket it got knocked way back in the list. Re-marking vetted as it's previously been all the way up through screening. Should also keep an eye on CLJ-787 as it may have some collisions with this one.

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

clj-700-7.diff is identical to clj-700-patch6.txt, except it applies cleanly to latest master. Only some lines of context in a test file have changed.

When I say "applies cleanly", I mean that there is one warning when using the proper "git am" command from the dev wiki page. This is because one line replaced in Associative.java has a CR/LF at the end of the line, because all lines in that file do.

Comment by Herwig Hochleitner [ 17/Feb/14 9:54 AM ]

Since clojure 1.5, contains? throws an IllegalArgumentException on transients.
In 1.6.0-beta1, transients are no longer marked as alpha.

Does this mean, that we won't be able to distinguish between a nil value and no value on a transient?





[CLJ-1241] NPE when AOTing overrided clojure.core functions Created: 30/Jul/13  Updated: 18/Apr/14

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

Type: Defect Priority: Major
Reporter: Phil Hagelberg Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: aot, compiler

Attachments: Text File 0001-fix-CLJ-1241.patch    
Patch: Code
Approval: Vetted

 Description   

When performing AOT compilation on a namespace that overrides a clojure.core function without excluding the original clojure.core function from the ns, you get a NullPointerException.

To reproduce:

$ lein new aot-get
$ cd aot-get
$ sed -i s/foo/get/
$ lein compile :all
WARNING: get already refers to: #'clojure.core/get in namespace: aot-get.core, being replaced by: #'aot-get.core/get
Exception in thread "main" java.lang.NullPointerException
	at clojure.lang.Compiler$ObjExpr.emitVar(Compiler.java:4858)
	at clojure.lang.Compiler$DefExpr.emit(Compiler.java:428)
	at clojure.lang.Compiler.compile1(Compiler.java:7152)
	at clojure.lang.Compiler.compile(Compiler.java:7219)
	at clojure.lang.RT.compile(RT.java:398)
	at clojure.lang.RT.load(RT.java:438)
	at clojure.lang.RT.load(RT.java:411)

Cause: DefExpr.parse does not call registerVar for vars overridding clojure.core ones, thus when AOT compiling the var is not registered in the constant table.

Proposed: The attached patch makes DefExpr.parse call registerVar for vars overridding clojure.core ones.

Patch: 0001-fix-CLJ-1241.patch



 Comments   
Comment by Nicola Mometto [ 30/Jul/13 7:29 PM ]

DefExpr.parse was not calling registerVar for vars overridding clojure.core ones.

Comment by Alex Miller [ 31/Jul/13 12:25 AM ]

Verified on Clojure 1.5.1.

Comment by Javier Neira Sanchez [ 27/Aug/13 8:34 AM ]

Reproduced with `key` function without `(:refer-clojure :exclude [key])`

Comment by Rich Hickey [ 05/Sep/13 8:32 AM ]

This doesn't meet triage guidelines - i.e. there is this problem, therefore we will fix it by _____ so it then does _____

Comment by Aaron Cohen [ 26/Mar/14 12:52 PM ]

This is still present in the 1.6 release. I think it's mis-classified as low priority.

Comment by Aaron Cohen [ 26/Mar/14 12:52 PM ]

See for instance the cascalog mailing list: https://groups.google.com/forum/#!topic/cascalog-user/Pe5QIpmU0vA

Comment by Andy Fingerhut [ 26/Mar/14 1:07 PM ]

It may help if someone could clarify Rich's comment.

Does it mean that the ticket should include a plan of the form "therefore we will fix it by _____ so it then does _____", but this ticket doesn't have that?

Or perhaps it means that the ticket should not include a plan of that form, but this ticket does? If so, I don't see it, except perhaps the very last sentence of the description. If that is a problem for vetting a ticket, perhaps we could just delete that sentence and proceed from there?

Something else?

Comment by Nicola Mometto [ 26/Mar/14 1:13 PM ]

Andy, I added the two last lines in the description after reading Rich's comment to explain why this bug happens and how the patch I attached works around this.

I don't know if this is what he was asking for though.

Comment by Alex Miller [ 27/Mar/14 11:00 AM ]

I think Rich meant that a ticket should have a plan of that form but does not. My own take on "triaged" is that it should state actual and expected results demonstrating a problem - I don't think it needs to actually describe the solution (as that can happen later in development). It is entirely possible that Rich and I differ in our interpretation of that. I will see if I can rework the description a bit to match what I've been doing elsewhere.

Comment by Andy Fingerhut [ 31/Mar/14 9:34 AM ]

Alex, I have looked through the existing wiki pages on the ticket tracking process, and do not recall seeing anything about this desired aspect of a triaged ticket. Is it already documented somewhere and I missed it? Not that it has to be documented, necessarily, but Rich saying "triage guidelines" makes it sound like a filter he applies that ticket creators and screeners maybe should know about.

Comment by Alex Miller [ 31/Mar/14 11:57 AM ]

To me, Triage (and Vetting) is all about having good problem statements. For a defect, it is most important to demonstrate the problem (what happens now) and what you expect to happen instead. I do not usually expect there to necessarily be "by ____" in the ticket - to me that is part of working through the solution (although it is typical to have this in an enhancement). This ticket, as it stands now, seems to have both a good problem statement and a good cause/solution statement so seems to exceed Triaging standards afaik.

Two places where I have tried to write about these things in the past are http://dev.clojure.org/display/community/Creating+Tickets and in the Triage process on the workflow page http://dev.clojure.org/display/community/JIRA+workflow.





[CLJ-971] Jar within a jar throws a runtime error Created: 10/Apr/12  Updated: 18/Apr/14

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

Type: Defect Priority: Major
Reporter: Ron Romero Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: None
Environment:

Maven using the one-jar plugin


Attachments: Text File clj-971-1.patch     Text File clj-971-2.patch    
Patch: Code
Approval: Vetted

 Description   

I've created two jar files in my multi-project Maven setup. The first jar is the "engine", and it includes the clojure jar in it. The other jar is the "application". It includes the engine and then packages itself into a one-jar jar file. This means we have a jar within a jar: The "onejar" contains the engine jar, which in turn contains that clojure jar.

I then get an error in the runtime:

Exception in thread "main" java.lang.reflect.InvocationTargetException
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:616)
at com.simontuffs.onejar.Boot.run(Boot.java:340)
at com.simontuffs.onejar.Boot.main(Boot.java:166)
Caused by: java.lang.ExceptionInInitializerError
at com.ziroby.clojure.App.main(App.java:14)
... 6 more
Caused by: java.lang.NullPointerException
at clojure.lang.RT.lastModified(RT.java:374)
at clojure.lang.RT.load(RT.java:408)
at clojure.lang.RT.load(RT.java:398)
at clojure.lang.RT.doInit(RT.java:434)
at clojure.lang.RT.<clinit>(RT.java:316)
... 7 more

See also my Stack Overflow question on this at http://stackoverflow.com/questions/7763480/making-an-executable-jar-that-evals-clojure-strings

In researching it, I've found the problem lies in RT.lastModified, where it tries to determine last modified time by looking at the modified time on the jar file for Clojure. But there's not actually a jar file, since it's embedded in another.

I've found that adding a null check solves the problem. My lastModified looks like this now:

static public long lastModified(URL url, String libfile) throws Exception{
if(url.getProtocol().equals("jar")) { ZipEntry entry = ((JarURLConnection) url.openConnection()).getJarFile().getEntry(libfile); if (entry != null) return entry.getTime(); }

return url.openConnection().getLastModified();
}

This runs successfully.

If you'd prefer, I can submit a patch, or commit directly.



 Comments   
Comment by Anders Sveen [ 11/Nov/13 3:29 AM ]

Would be awesome if you could get this working. Wanting to use som Clojure libs in Java so Leiningen uberjar is not an option right now.

Comment by Paavo Parkkinen [ 24/Nov/13 7:11 AM ]

Took the code change in the description and rolled it into a patch to hopefully push this forward a little bit.

Comment by Alex Miller [ 24/Nov/13 10:06 AM ]

Thanks Paavo - on a quick scan, it looks like in the case you're interested in the updated code would now call url.openConnection() twice - perhaps that could be factored out?

Comment by Paavo Parkkinen [ 25/Nov/13 6:19 AM ]

New patch with duplicate calls to url.openConnection() factored out.

Comment by Gleb Kanterov [ 13/Dec/13 4:30 AM ]

I had the same issue, adding following line to manifest file worked for me

One-Jar-URL-Factory: com.simontuffs.onejar.JarClassLoader$OneJarURLFactory
Comment by Stuart Halloway [ 31/Jan/14 1:04 PM ]

Can we get a test showing the normal path and the can't-read-inside path?

Comment by Sean Shubin [ 17/Apr/14 9:34 PM ]

I notice this solution falls back on the last modified date for the entire jar, while it is still possible to get the date of the individual file, albeit less efficiently.
I posted a way to get the individual file date in CLJ-1405.
Perhaps you don't need the date of the individual file, but I am having a hard time groking the calling code so I am not sure what its intent is.
I did notice in the calling code that the if statement
(classURL != null && (cljURL == null || lastModified(classURL, classfile) > lastModified(cljURL, cljfile))) || classURL == null
Is logically equivalent to the much simpler
classURL == null || cljURL == null || lastModified(classURL, classfile) > lastModified(cljURL, cljfile)





[CLJ-1274] Unable to set compiler options via system properties except for AOT compilation Created: 02/Oct/13  Updated: 18/Apr/14

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

Type: Defect Priority: Major
Reporter: Howard Lewis Ship Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: compiler

Attachments: Text File CLJ-1274.patch    
Patch: Code
Approval: Vetted

 Description   

The code that converts JVM system properties into keys under the *compiler-options* var is present only inside the clojure.lang.Compile class. This is a problem when using a debugger inside an IDE and not AOT compiling; specifying -Dclojure.compiler.disable-locals-clearing=true has no effect here when it would be most useful!



 Comments   
Comment by Howard Lewis Ship [ 02/Oct/13 4:52 PM ]

Obviously, that's supposed to be *compiler-options*.

Comment by Howard Lewis Ship [ 02/Dec/13 4:03 PM ]

Changes initialization of *compiler-options* to occur statically inside Compiler; now available to all forms of Clojure, not just AOT compilation; however, the initial *compiler-options* value is now defined as a root binding, rather than a per-thread binding, which has slightly different semantics.





[CLJ-1362] Reduce broken on some primitive vectors Created: 18/Feb/14  Updated: 18/Apr/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: Release 1.7

Type: Defect Priority: Major
Reporter: Nathan Davis Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: collections

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

 Description   

In some cases, reduce over a sequence from a primitive vector created with vector-of will return incorrect answers:

user=> (into [] (drop 32 (into [] (range 33))))
[32]
user=> (into [] (drop 32 (into (vector-of :int) (range 33))))
[0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32]

Second call should return [32] just like the first one.

Cause: VecSeq (seq on primitive Vec obtained with vector-of) maintains two flags: i is the total number of elements prior to the current node in this seq. offset is the offset in the current anode. When using internal-reduce on a VecSeq, the starting index for the reduce was using offset and ignoring i.

Solution: Use (+ i offset) as the starting index.

Patch: clj-1362-v1.patch

Screened by:



 Comments   
Comment by Alex Miller [ 18/Feb/14 10:18 PM ]

We did some debugging on this at the St. Louis Clojure Meetup tonight and suspect the problem is happening when drop walks through the chunked seq over the vector. Specifically, in the VecSeq's implementation of IChunkedSeq.chunkedNext() at https://github.com/clojure/clojure/blob/master/src/clj/clojure/gvec.clj#L116 particularly the offset 0 at the end.

Comment by Alex Miller [ 19/Feb/14 2:41 PM ]

Upon further review, the VecSeq seems to be created properly during chunking. The real issue is in internal-reduce where the starting index is improperly computed.





[CLJ-1192] vec function is substantially slower than into function Created: 06/Apr/13  Updated: 18/Apr/14

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

Type: Defect Priority: Major
Reporter: Luke VanderHart Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: performance

Approval: Vetted

 Description   

(vec coll) and (into [] coll) do exactly the same thing. However, due to into using transients, it is substantially faster. On my machine:

(time (dotimes [_ 100] (vec (range 100000))))
"Elapsed time: 732.56 msecs"

(time (dotimes [_ 100] (into [] (range 100000))))
"Elapsed time: 491.411 msecs"

This is consistently repeatable.

Since vec's sole purpose is to transform collections into vectors, it should do so at the maximum speed available.



 Comments   
Comment by Andy Fingerhut [ 07/Apr/13 5:50 PM ]

I am pretty sure that Clojure 1.5.1 also uses transient vectors for (vec (range n)) (probably also some earlier versions of Clojure, too).

Look at vec in core.clj. It checks whether its arg is a java.util.Collection, which lazy seqs are, so calls (clojure.lang.LazilyPersistentVector/create coll).

LazilyPersistentVector's create method checks whether its argument is an ISeq, which lazy seqs are, so it calls PersistentVector.create(RT.seq(coll)).

All 3 of PersistentVector's create() methods use transient vectors to build up the result.

I suspect the difference in run times are not because of transients or not, but because of the way into uses reduce, and perhaps may also have something to do with the perhaps-unnecessary call to RT.seq in LazilyPersistentVector's create method (in this case, at least – it is likely needed for other types of arguments).

Comment by Alan Malloy [ 14/Jun/13 2:17 PM ]

I'm pretty sure the difference is that into uses reduce: since reducers were added in 1.5, chunked sequences know how to reduce themselves without creating unnecessary cons cells. PersistentVector/create doesn't use reduce, so it has to allocate a cons cell for each item in the sequence.

Comment by Gary Fredericks [ 08/Sep/13 1:55 PM ]

Is there any downside to (defn vec [coll] (into [] coll)) (or the inlined equivalent)?

Comment by Ghadi Shayban [ 11/Apr/14 5:13 PM ]

While I agree that there are improvements and possibly low-hanging fruit, FWIW https://github.com/clojure/tools.analyzer/commit/cf7dda81a22f4c9c1fe64c699ca17e7deed61db4#commitcomment-5989545

showed a 5% slowdown from a few callsites in tools.analyzer.

This ticket's benchmark is incomplete in that it covers a single type of argument (chunked range), and flawed as it timing the expense of realizing the range. (That could be a legit benchmark case, but it shouldn't be the only one).

Sorry to rain on a parade. I promise like speed too!





[CLJ-887] Error when calling primitive functions with destructuring in the arg vector Created: 29/Nov/11  Updated: 18/Apr/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: Alexander Taggart Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: compiler

Attachments: Text File 0001-don-t-remove-meta-from-arg-vector-in-maybe-destructu.patch    
Patch: Code
Approval: Vetted

 Description   

If one defines a primitive-taking function with destructuring, calling that function will result in a ClassCastException, IFF the primitive return-type hint is present.

Clojure 1.4.0-master-SNAPSHOT
user=> (defn foo [[a b] ^long x ^long y] 0)
#'user/foo
user=> (foo [1 2] 3 4)
0
user=> (defn foo ^long [[a b] ^long x ^long y] 0)
#'user/foo
user=> (foo [1 2] 3 4)
ClassCastException user$foo cannot be cast to clojure.lang.IFn$OLLL  user/eval9 (NO_SOURCE_FILE:4)
user=> (pst)
ClassCastException user$foo cannot be cast to clojure.lang.IFn$OLLL
	user/eval9 (NO_SOURCE_FILE:4)
	clojure.lang.Compiler.eval (Compiler.java:6493)
	clojure.lang.Compiler.eval (Compiler.java:6459)
	clojure.core/eval (core.clj:2796)
	clojure.main/repl/read-eval-print--5967 (main.clj:244)
	clojure.main/repl/fn--5972 (main.clj:265)
	clojure.main/repl (main.clj:265)
	clojure.main/repl-opt (main.clj:331)
	clojure.main/main (main.clj:427)
	clojure.lang.Var.invoke (Var.java:397)
	clojure.lang.Var.applyTo (Var.java:518)
	clojure.main.main (main.java:37)
nil


 Comments   
Comment by Nicola Mometto [ 03/Apr/14 1:35 PM ]

This was happening because maybe-destructured returned the arg vector without the type hint, so the function was getting compiled to a IFn$OLLO rather than a IFn$OLLL but the :arglists vector in the var meta was still tagged, so the compiler thought that foo was a IFn$OLLL.

This patch addresses this by preserving the original meta on the fn arglist

Comment by Alex Miller [ 03/Apr/14 1:40 PM ]

Weirdly, I saw this happen today in my own code.





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

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

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

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


Approval: Vetted

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

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



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

reproduces with jdk 1.8.0 and clojure 1.6





[CLJ-1388] equality bug on records created with nested calls to map->record Created: 18/Mar/14  Updated: 18/Apr/14

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

Type: Defect Priority: Major
Reporter: Nicola Mometto Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: defrecord

Attachments: Text File 0001-FIX-CLJ-1388.patch    
Patch: Code and Test
Approval: Screened

 Description   
user> (defrecord a []) 
user.a
user> (def r1 (map->a {:a 1}))
nil
user> (def r2 (map->a r1))
nil
user> (= r1 r2)  ;; expected => true
false
user> (.__extmap r1)
{:a 1}
user> (.__extmap r2)  ;; expected => {:a 1}
#user.a{:a 1}

This bug was described in this post: https://groups.google.com/forum/#!topic/clojure/iN-SPBaTFUw

Approach: Clean the extmap before putting it into the record constructor.

Patch: 0001-FIX-CLJ-1388.patch

Screened by: Alex Miller






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

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

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

Windows 7; JVM 1.6; Clojure 1.3 beta 1


Approval: Vetted

 Description   

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

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

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

(future (send-off agt fill))

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



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

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

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

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

release-pending-sends?





[CLJ-1039] Using 'def with metadata {:type :anything} throws ClassCastException during printing Created: 09/Aug/12  Updated: 18/Apr/14

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

Type: Defect Priority: Minor
Reporter: Gunnar Völkel Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None
Environment:

Ubuntu, lein 1.7.1 - lein repl


Attachments: Text File CLJ-1039-tolerate-misleading-type-metadata-on-var-wh.patch    
Patch: Code and Test
Approval: Vetted

 Description   

Specific to setting :type meta on a var:

user=> (def ^{:type :anything} mydef 1)
#<main$repl clojure.main$repl@6193b845>
CompilerException java.lang.ClassNotFoundException: main.clj:257, compiling:(NO_SOURCE_PATH:13:20)
ClassCastException clojure.lang.Var cannot be cast to clojure.lang.IObj  clojure.core/with-meta (core.clj:214)
user=> (pst *e)
ClassCastException clojure.lang.Var cannot be cast to clojure.lang.IObj
	clojure.core/with-meta (core.clj:214)
	clojure.core/vary-meta (core.clj:640)
	clojure.core/fn--5420 (core_print.clj:76)
	clojure.lang.MultiFn.invoke (MultiFn.java:232)
	clojure.core/pr-on (core.clj:3392)
	clojure.core/pr (core.clj:3404)
	clojure.core/apply (core.clj:624)
	clojure.core/prn (core.clj:3437)
	clojure.main/repl/read-eval-print--6627 (main.clj:241)
	clojure.main/repl/fn--6636 (main.clj:258)
	clojure.main/repl (main.clj:258)
	clojure.main/repl-opt (main.clj:324)

If it is intended to forbid setting the :type metadata, then there should be an appropriate error message instead of the ClassCastException.

Cause: This is caused by the printer dispatch function

(defmulti print-method (fn [x writer]
                         (let [t (get (meta x) :type)]
                           (if (keyword? t) t (class x)))))

which ends up calling the default dispatch, which tries to vary-meta.

Solution:

Patch:

Screened by:



 Comments   
Comment by Stuart Halloway [ 10/Aug/12 1:40 PM ]

This is caused by the printer dispatch function

(defmulti print-method (fn [x writer]
                         (let [t (get (meta x) :type)]
                           (if (keyword? t) t (class x)))))

which ends up calling the default dispatch, which tries to vary-meta.

Comment by Steve Miner [ 14/Apr/14 10:13 AM ]

The :type metadata is used internally by Clojure. For the situation in this bug report, you have to take responsibility for providing a print-method if you put :type metatdata on your var.

This is not a good example, but it shows one way to work around the bug:

(defmethod print-method :anything [obj w] (print-method {:anything @obj} w))
Comment by Steve Miner [ 14/Apr/14 10:21 AM ]

On the other hand, the :default print-method probably should be more robust. I think a check for (instance? clojure.lang.IObj o) before calling vary-meta would be appropriate. Added a patch that calls print-simple if the o isn't an IObj. That works around the issue for Var, and seems reasonable for other exotic types. The only downside I can imagine is if someone had a custom print-method but accidentally had a typo in their :type metadata, they will no longer get an error. This was an edge case to begin with so that probably doesn't matter.





[CLJ-1187] Clojure loses quoted metadata on empty literals Created: 22/Mar/13  Updated: 18/Apr/14

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

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

Attachments: Text File 001-CLJ-1187.patch    
Patch: Code and Test
Approval: Vetted

 Description   

meta on empty collections is lost:

user=> (meta '^:foo [])
nil   ;; expected {:foo true} as in:
user=> (meta '^:foo [1])
{:foo true}

This bug propagates to ^:const vars:

user=> (def ^:const foo ^:foo [])
#'user/foo
user=> (meta foo)
nil
user=> (meta @#'foo)
{:foo true}

Cause: As in CLJ-1093, empty collections are replaced with an EmptyExpr that loses meta

Proposed: Don't replace with EmptyExpr if meta is present.

Patch: 001-CLJ-1187.patch (NOTE: this change overlaps with CLJ-1093 and the patch there may be preferred)

Screened by:



 Comments   
Comment by Nicola Mometto [ 22/Mar/13 2:12 PM ]

After patch:

user=> (meta '^:foo [])
{:foo true}
user=> (meta '^:foo [:a])
{:foo true}
user=> (def ^:const foo ^:foo [])
#'user/foo
user=> (meta foo)
{:foo true}

Comment by Stuart Halloway [ 29/Mar/13 6:13 AM ]

I believe the title should read "Clojure loses quoted metdata on empty literals".

Comment by Stuart Halloway [ 29/Mar/13 6:16 AM ]

At first glance, the implementation looks wrong in that it blocks non-IObjs ever getting to EmptyExpr. Probably all persistent collections are IObjs, but would not want to bake this in.

Comment by Nicola Mometto [ 29/Mar/13 7:00 AM ]

You're right, I've updated my patch, it should work as expected now

Comment by Andy Fingerhut [ 04/Apr/13 9:44 PM ]

Nicola: Your updated patch 001-CLJ-1187.patch dated Mar 29, 2013 gives syntax errors when I try to compile it.

Comment by Nicola Mometto [ 05/Apr/13 9:06 AM ]

Oh, the irony, I messed up some parentheses writing java.

Sorry for it, here's the correct patch, it applies on upstream/master.

Comment by Gabriel Horner [ 24/May/13 9:14 AM ]

Looks good

Comment by Nicola Mometto [ 26/Jun/13 8:08 PM ]

CLJ-1093 contains a patch that fixes this issue aswell and should be preferred

Comment by Alex Miller [ 18/Aug/13 2:46 PM ]

Marking un-Screened due to the note about CLJ-1093. Want to assess this more before going through Rich.

Comment by Alex Miller [ 18/Oct/13 8:49 AM ]

Switching to Incomplete pending CLJ-1093 which I hope to pull into 1.6.

Comment by Alex Miller [ 17/Jan/14 10:17 AM ]

Pulling out of 1.6, will consider in next release.





[CLJ-1100] Reader literals cannot contain periods Created: 02/Nov/12  Updated: 18/Apr/14

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

Type: Defect Priority: Minor
Reporter: Kevin Lynagh Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: reader

Approval: Vetted

 Description   

The reader tries to read a record instead of a literal if the tag contains periods.

user> (binding [*data-readers* {'foo/bar #'identity}] (read-string "#foo/bar 1"))
1
user> (binding [*data-readers* {'foo/bar.x #'identity}] (read-string "#foo/bar.x 1"))
ClassNotFoundException foo/bar.x  java.lang.Class.forName0 (Class.java:-2)

Summary of reader forms:

Kind Example Constraint Status
Record #user.Foo[1] record class name OK
Class #java.lang.String["abc"] class name OK
Clojure reader tag #uuid "c48d7d6e-f3bb-425a-abc5-44bd014a511d" not a class name, no "/" OK
Library reader tag #my/card "5H" not a class name, has "/" OK
  #my.ns/card "5H" not a class name, has "/" OK
  #my/playing.card "5H" not a class name, has "/" BROKEN - read as record

Note: reader tags should not be allowed to override the record reader.

Cause: In LispReader, CtorReader.invoke() decides between record and tagged literal based on whether the tag has a ".".

Proposed: Change the discriminator in CtorReader.

Alternative 1 (purely string inspection):

  • If name has a "/" -> readTagged (not a legal class name)
  • If name has no "/" or "." -> readTagged (records must have qualified names)
  • Else -> readRecord (also covers Java classes)

Tradeoffs: Clojure-defined data reader tags must not contain periods. Not possible to read a Java class with no package. Avoids unnecessary class loading/construction for all tags.

Alternative 2 (prioritize Class check):

  • Attempt readRecord (also covers Java classes)
  • If failed, attempt readTagged

Tradeoffs: Clojure tags could not override Java/record constructors - I'm not sure that's something we'd ever want to do, but this would cut that off. This alternative may attempt classloading when it would not have before.

Hybrids of these are also possible.

Patch:

Screened by:



 Comments   
Comment by Steve Miner [ 06/Nov/12 9:41 AM ]

The suggested patch (clj-1100-reader-literal-periods.patch) will break reading records when *default-data-reader-fn* is set. Try adding a test like this:

(deftest tags-containing-periods-with-default
      ;; we need a predefined record for this test so we (mis)use clojure.reflect.Field for convenience
      (let [v "#clojure.reflect.Field{:name \"fake\" :type :fake :declaring-class \"Fake\" :flags nil}"]
        (binding [*default-data-reader-fn* nil]
          (is (= (read-string v) #clojure.reflect.Field{:name "fake" :type :fake :declaring-class "Fake" :flags nil})))
        (binding [*default-data-reader-fn* (fn [tag val] (assoc val :meaning 42))]
          (is (= (read-string v) #clojure.reflect.Field{:name "fake" :type :fake :declaring-class "Fake" :flags nil})))))
Comment by Rich Hickey [ 29/Nov/12 9:36 AM ]

The problem assessment is ok, but the resolution approach may not be. What happens should be based not upon what is in data-readers but whether or not the name names a class.

Is the intent here to allow readers to circumvent records? I'm not in favor of that.

Comment by Steve Miner [ 29/Nov/12 4:01 PM ]

New patch following Rich's comments. The decision to read a record is now based on the symbol containing periods and not having a namespace. Otherwise, it is considered a data reader tag. User
defined tags are required to be qualified but they may now have periods in the name. Tests added to show that
data readers cannot override record classes. Note: Clojure-defined data reader tags may be unqualified, but they should not contain periods in order to avoid confusion with record classes.

Comment by Steve Miner [ 29/Nov/12 4:17 PM ]

I deleted my old patch and some comments referring to it to avoid confusion.

In Clojure 1.5 beta 1, # followed by a qualified symbol with a period in the name is considered a record and causes an exception for the missing record class. With the patch, only non-qualified symbols containing periods are considered records. That allows user-defined qualified symbols with periods in their names to be used as data reader tags.

Comment by Andy Fingerhut [ 07/Feb/13 9:05 AM ]

clj-1100-periods-in-data-reader-tags-patch-v2.txt dated Feb 7 2013 is identical to CLJ-1100-periods-in-data-reader-tags.patch dated Nov 29 2012, except it applies cleanly to latest master. The only change appears to be in some white space in the context lines.

Comment by Andy Fingerhut [ 07/Feb/13 12:53 PM ]

I've removed clj-1100-periods-in-data-reader-tags-patch-v2.txt mentioned in the previous comment, because I learned that CLJ-1100-periods-in-data-reader-tags.patch dated Nov 29 2012 applies cleanly to latest master and passes all tests if you use this command to apply it.

% git am --keep-cr -s --ignore-whitespace < CLJ-1100-periods-in-data-reader-tags.patch

I've already updated the JIRA workflow and screening patches wiki pages to mention this --ignore-whitespace option.

Comment by Andy Fingerhut [ 13/Feb/13 11:31 AM ]

Both of the current patches, CLJ-1100-periods-in-data-reader-tags.patch dated Nov 29 2012, and clj-1100-reader-literal-periods.patch dated Nov 6 2012, fail to apply cleanly to latest master (1.5.0-RC15) as of today, although they did last week. Given all of the changes around read / read-string and edn recently, they should probably be evaluated by their authors to see how they should be updated.

Comment by Steve Miner [ 14/Feb/13 12:23 PM ]

I deleted my patch: CLJ-1100-periods-in-data-reader-tags.patch. clj-1100-reader-literal-periods.patch is clearly wrong, but the original author or an administrator has to delete that.

Comment by Kevin Lynagh [ 14/Feb/13 1:28 PM ]

I cannot figure out how to remove my attachment (clj-1100-reader-literal-periods.patch) in JIRA.

Comment by Steve Miner [ 14/Feb/13 1:43 PM ]

Downarrow (popup) menu to the right of the "Attachments" section. Choose "manager attachments".

Comment by Kevin Lynagh [ 14/Feb/13 2:02 PM ]

Great, thanks Steve. Are you going to take another pass at this issue, or should I give it a go?

Comment by Steve Miner [ 14/Feb/13 3:04 PM ]

Kevin, I'm not planning to work on this right now as 1.5 is pretty much done. It might be worthwhile discussing the issue a bit on the dev mailing list before working on a patch, but that's up to you. I think my approach was correct, although now changes would have to be applied to both LispReader and EdnReader.

Comment by Alex Miller [ 09/Apr/14 10:29 AM ]

Updated description based on my understanding.





[CLJ-1157] Classes generated by gen-class aren't loadable from remote codebase for mis-implementation of static-initializer Created: 04/Feb/13  Updated: 18/Apr/14

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

Type: Defect Priority: Minor
Reporter: Tsutomu Yano Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: gen-class
Environment:

Tested on Mac OS X 10.9.1 and Oracle JVM 1.7.0_51 with Clojure 1.6 master SNAPSHOT


Attachments: File 20140121_fix_classloader.diff    
Patch: Code
Approval: Vetted

 Description   

When a genclass'ed object is serialized and sent to a remote system, the remote system throws an exception loading the object:

Exception in thread "main" java.lang.ExceptionInInitializerError
        at java.io.ObjectStreamClass.hasStaticInitializer(Native Method)
        at java.io.ObjectStreamClass.computeDefaultSUID(ObjectStreamClass.java:1723)
        at java.io.ObjectStreamClass.access$100(ObjectStreamClass.java:69)
        at java.io.ObjectStreamClass$1.run(ObjectStreamClass.java:247)
        at java.io.ObjectStreamClass$1.run(ObjectStreamClass.java:245)
        at java.security.AccessController.doPrivileged(Native Method)
        at java.io.ObjectStreamClass.getSerialVersionUID(ObjectStreamClass.java:244)
        at java.io.ObjectStreamClass.initNonProxy(ObjectStreamClass.java:600)
        at java.io.ObjectInputStream.readNonProxyDesc(ObjectInputStream.java:1601)
        at java.io.ObjectInputStream.readClassDesc(ObjectInputStream.java:1514)
        at java.io.ObjectInputStream.readOrdinaryObject(ObjectInputStream.java:1750)
        at java.io.ObjectInputStream.readObject0(ObjectInputStream.java:1347)
        at java.io.ObjectInputStream.readObject(ObjectInputStream.java:369)
        at sun.rmi.server.UnicastRef.unmarshalValue(UnicastRef.java:324)
        at sun.rmi.server.UnicastRef.invoke(UnicastRef.java:173)
        at java.rmi.server.RemoteObjectInvocationHandler.invokeRemoteMethod(RemoteObjectInvocationHandler.java:194)
        at java.rmi.server.RemoteObjectInvocationHandler.invoke(RemoteObjectInvocationHandler.java:148)
        at $Proxy0.getResult(Unknown Source)
        at client.SampleClient$_main.doInvoke(SampleClient.clj:12)
        at clojure.lang.RestFn.invoke(RestFn.java:397)
        at clojure.lang.AFn.applyToHelper(AFn.java:159)
        at clojure.lang.RestFn.applyTo(RestFn.java:132)
        at client.SampleClient.main(Unknown Source)
 Caused by: java.io.FileNotFoundException: Could not locate remoteserver/SampleInterfaceImpl__init.class or remoteserver/SampleInterfaceImpl.clj on classpath: 
        at clojure.lang.RT.load(RT.java:434)
        at clojure.lang.RT.load(RT.java:402)
        at clojure.core$load$fn__5039.invoke(core.clj:5520)
        at clojure.core$load.doInvoke(core.clj:5519)
        at clojure.lang.RestFn.invoke(RestFn.java:408)
        at clojure.lang.Var.invoke(Var.java:415)
        at remoteserver.SampleInterfaceImpl.<clinit>(Unknown Source)
        ... 23 more

Reproduce:

// build
git clone git://github.com/tyano/clojure_genclass_fix.git
cd clojure_genclass_fix
sh build.sh
// start rmiregistry
rmiregistry -J-Djava.rmi.server.useCodebaseOnly=false &
// start server
cd remoteserver
sh start.sh
// Start client
cd ../client
sh start.sh

Cause:

A gen-classed class (in this case, SampleInterfaceImpl.class) uses a static-initializer for loading SampleInterfaceImpl__init.class like:

static
  {
    RT.var("clojure.core", "load").invoke("/remoteserver/SampleInterfaceImpl");
  }

RT.load in default uses a context-classloader for loading __init.class but all classes depending on a gen-classed class must be loaded from the same classloader. In this case, RT.load must use a remote URLClassLoader which loads the main class.

Proposed:

Instead produce the equivalent of this in the static initializer:

static
  {
    Var.pushThreadBindings(RT.map(new Object[] { Compiler.LOADER, SampleInterfaceImpl.class.getClassLoader() }));
    try {
        RT.var("clojure.core", "load").invoke("/remoteserver/SampleInterfaceImpl");
    }
    finally 
    {
        Var.popThreadBindings();
    }
  }

With this code, RT.load will uses a same classloader which load SampleInterfaceImpl.class.

Patch: 20140121_fix_classloader.diff

Screened by:



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

This sounds reasonable, but anything touching classloaders must be considered very carefully.

Comment by Stuart Halloway [ 01/Mar/13 12:12 PM ]

It seems overly complex to have the patch do so much code generation. Why not implement a method that does this job, and have the generated code call that?

Comment by Andy Fingerhut [ 11/Jan/14 2:47 PM ]

Patch 20130204_fix_classloader.diff dated Feb 3, 2013 no longer applies cleanly as of the latest commits to Clojure master on Jan 11, 2014. The only conflict in applying the patch appears to be in the file src/jvm/clojure/asm/commons/GeneratorAdapter.java. This is probably due to the commit for ticket CLJ-713 that was committed today, updating the ASM library.

Comment by Tsutomu Yano [ 21/Jan/14 3:01 AM ]

I put a new patch applicable on the latest master branch.
This new patch is simpler and robust because the code-generation becomes very simple. Now It just call a method implemented with Java.

And I fixed my sample program and the 'HOW TO REPRODUCT THIS ISSUE' section of this ticket, because old description is not runnable on newest JVM. It is because the specification of remote method call of the newest JVM was changed from the old one. In the newest JVM, we need a 'java.rmi.server.useCodebaseOnly=false' option for making the behavior of remote call same as old one.
pull the newest sample.





[CLJ-1093] Empty PersistentCollections get incorrectly evaluated as their generic clojure counterpart Created: 24/Oct/12  Updated: 18/Apr/14

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

Type: Defect Priority: Minor
Reporter: Nicola Mometto Assignee: Timothy Baldridge
Resolution: Unresolved Votes: 2
Labels: None

Attachments: Text File 0001-CLJ-1093-fix-empty-records-literal-v2.patch     Text File clj-1093-fix-empty-record-literal-patch-v2.txt    
Patch: Code and Test
Approval: Vetted

 Description   
user> (defrecord x [])
user.x
user> #user.x[]
{}
user> #user.x{}
{}
user> #clojure.lang.PersistentTreeMap[]
{}
user> (class *1)
clojure.lang.PersistentArrayMap

Cause: Compiler's ConstantExpr parser returns an EmptyExpr for all empty persistent collections, even if they are of types other than the core collections (for example: records, sorted collections, custom collections). EmptyExpr reports its java class as one the classes - IPersistentList/IPersistentVector/IPersistentMap/IPersistentSet rather than the original type.

Proposed: If one of the Persistent* classes, then create EmptyExpr as before, otherwise retain the ConstantExpression of the original collection.

Patch: 0001-CLJ-1093-fix-empty-records-literal-v2.patch

Screened by:



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

Unable to reproduce this bug on latest version of master. Most likely fixed by some of the recent changes to data literal readers.

Marking Not-Approved.

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

Could not reproduce in master.

Comment by Nicola Mometto [ 01/Mar/13 1:23 PM ]

I just checked, and the problem still exists for records with no arguments:

Clojure 1.6.0-master-SNAPSHOT
user=> (defrecord a [])
user.a
user=> #user.a[]
{}

Admittedly it's an edge case and I see little usage for no-arguments records, but I think it should be addressed aswell since the current behaviour is not what one would expect

Comment by Herwig Hochleitner [ 02/Mar/13 8:14 AM ]

Got the following REPL interaction:

% java -jar ~/.m2/repository/org/clojure/clojure/1.5.0/clojure-1.5.0.jar
user=> (defrecord a [])
user.a
user=> (a.)
#user.a{}
user=> #user.a{}
{}
#user.a[]
{}

This should be reopened or declined for another reason than reproducability.

Comment by Nicola Mometto [ 10/Mar/13 2:18 PM ]

I'm reopening this since the bug is still there.

Comment by Andy Fingerhut [ 13/Mar/13 2:04 PM ]

Patch clj-1093-fix-empty-record-literal-patch-v2.txt dated Mar 13, 2013 is identical to Bronsa's patch 001-fix-empty-record-literal.patch dated Oct 24, 2012, except that it applies cleanly to latest master. I'm not sure why the older patch doesn't but git doesn't like something about it.

Comment by Nicola Mometto [ 26/Jun/13 8:06 PM ]

Patch 0001-CLJ-1093-fix-empty-records-literal-v2.patch solves more issues than the previous patch that was not evident to me at the time.

Only collections that are either PersistentList or PersistentVector or PersistentHash[Map|Set] or PersistentArrayMap can now be EmptyExpr.
This is because we don't want every IPersistentCollection to be emitted as a generic one eg.

user=> (class #clojure.lang.PersistentTreeMap[])
clojure.lang.PersistentArrayMap

Incidentally, this patch also solves CLJ-1187
This patch should be preferred over the one on CLJ-1187 since it's more general

Comment by Jozef Wagner [ 09/Aug/13 2:08 AM ]

Maybe this is related:

user=> (def x `(quote ~(list 1 (clojure.lang.PersistentTreeMap/create (seq [1 2 3 4])))))
#'user/x
user=> x
(quote (1 {1 2, 3 4}))
user=> (class (second (second x)))
clojure.lang.PersistentTreeMap
user=> (eval x)
(1 {1 2, 3 4})
user=> (class (second (eval x)))
clojure.lang.PersistentArrayMap

Even if the collection is not evaluated, it is still converted to the generic clojure counterpart.





[CLJ-1261] Invalid defrecord results in exception attributed to namespace that imports namespace with defrecord Created: 12/Sep/13  Updated: 18/Apr/14

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

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

Attachments: File clj-1261-2.diff    
Patch: Code
Approval: Incomplete

 Description   

I was introducing a namespace that included a defrecord.

My defrecord was wrong; it used a keyword to define a field, not a symbol. Minimal test case:

% cat src/useclj16/init.clj
(ns useclj16.init)

(defrecord Application [:shutdown-fn])
% cat src/useclj16/app.clj 
(ns useclj16.app
  (:require [useclj16.init :as init]))

However, the exception was perplexing:

% java -cp clojure-1.6.0-master-SNAPSHOT.jar:src clojure.main
user=> (require 'useclj16.app)
ClassCastException clojure.lang.Keyword cannot be cast to clojure.lang.IObj  clojure.core/with-meta (core.clj:214)

user=> (pst *e 100)
ClassCastException clojure.lang.Keyword cannot be cast to clojure.lang.IObj
        clojure.core/with-meta (core.clj:214)
        clojure.core/defrecord/fn--147 (core_deftype.clj:362)
        clojure.core/map/fn--4210 (core.clj:2494)
        clojure.lang.LazySeq.sval (LazySeq.java:42)
        clojure.lang.LazySeq.seq (LazySeq.java:60)
        clojure.lang.RT.seq (RT.java:484)
        clojure.lang.LazilyPersistentVector.create (LazilyPersistentVector.java:31)
        clojure.core/vec (core.clj:354)
        clojure.core/defrecord (core_deftype.clj:362)
        clojure.lang.Var.invoke (Var.java:427)
        clojure.lang.Var.applyTo (Var.java:532)
        clojure.lang.Compiler.macroexpand1 (Compiler.java:6483)
        clojure.lang.Compiler.macroexpand (Compiler.java:6544)
        clojure.lang.Compiler.eval (Compiler.java:6618)
        clojure.lang.Compiler.load (Compiler.java:7079)
        clojure.lang.RT.loadResourceScript (RT.java:370)
        clojure.lang.RT.loadResourceScript (RT.java:361)
        clojure.lang.RT.load (RT.java:440)
        clojure.lang.RT.load (RT.java:411)
        clojure.core/load/fn--5024 (core.clj:5546)
        clojure.core/load (core.clj:5545)
        clojure.core/load-one (core.clj:5352)
        clojure.core/load-lib/fn--4973 (core.clj:5391)
        clojure.core/load-lib (core.clj:5390)
        clojure.core/apply (core.clj:619)
        clojure.core/load-libs (core.clj:5429)
        clojure.core/apply (core.clj:619)
        clojure.core/require (core.clj:5512)
        useclj16.app/eval322/loading--4916--auto----323 (app.clj:1)
        useclj16.app/eval322 (app.clj:1)
        clojure.lang.Compiler.eval (Compiler.java:6634)
        clojure.lang.Compiler.eval (Compiler.java:6623)
        clojure.lang.Compiler.load (Compiler.java:7079)
        clojure.lang.RT.loadResourceScript (RT.java:370)
        clojure.lang.RT.loadResourceScript (RT.java:361)
        clojure.lang.RT.load (RT.java:440)
        clojure.lang.RT.load (RT.java:411)
        clojure.core/load/fn--5024 (core.clj:5546)
        clojure.core/load (core.clj:5545)
        clojure.core/load-one (core.clj:5352)
        clojure.core/load-lib/fn--4973 (core.clj:5391)
        clojure.core/load-lib (core.clj:5390)
        clojure.core/apply (core.clj:619)
        clojure.core/load-libs (core.clj:5429)
        clojure.core/apply (core.clj:619)
        clojure.core/require (core.clj:5512)
        user/eval318 (NO_SOURCE_FILE:1)
        clojure.lang.Compiler.eval (Compiler.java:6634)
        clojure.lang.Compiler.eval (Compiler.java:6597)
        clojure.core/eval (core.clj:2864)
        clojure.main/repl/read-eval-print--6594/fn--6597 (main.clj:260)
        clojure.main/repl/read-eval-print--6594 (main.clj:260)
        clojure.main/repl/fn--6603 (main.clj:278)
        clojure.main/repl (main.clj:278)
        clojure.main/repl-opt (main.clj:344)
        clojure.main/main (main.clj:442)
        clojure.lang.Var.invoke (Var.java:411)
        clojure.lang.Var.applyTo (Var.java:532)
        clojure.main.main (main.java:37)
nil

The error was attributed to app.clj (useclj16.app), a namespace which requires useclj16.init, the namespace containing the defrecord.

No indication that this concerned a defrecord, or even what namespace contained the error, was present in the exception.

Patch: clj-1261-v1.txt

Approach: Check explicitly that the fields are all symbols, for both defrecord and deftype, and throw a CompilerException with file, line, and column number if not. Example of exception after patch is applied, in the case give above:

user=> (require 'useclj16.app)
CompilerException java.lang.AssertionError: defrecord and deftype fields must be symbols.  These are not allowed: (:shutdown-fn), compiling:(useclj16/init.clj:3:1)


 Comments   
Comment by Alex Miller [ 12/Sep/13 8:58 PM ]

Can you include an example of the defrecord definition just so we're clear what it looks like?

Comment by Alex Miller [ 12/Sep/13 8:59 PM ]

Also, "feedback" is not a useful label. Please use "errormsgs" for stuff like this. See the list of many commonly used labels here: http://dev.clojure.org/display/community/Creating+Tickets

Comment by Howard Lewis Ship [ 13/Sep/13 10:42 AM ]

"Feedback" is my own personal crusade http://tapestryjava.blogspot.com/2013/05/once-more-feedback-please.html

In my case, my invalid code was:

(defrecord Application [:shutdown-fn])

And the mistake was that :shutdown-fn should be a symbol, not a keyword.

Here it is, more completely:

(ns novate.services.initialization
  "Infrastructure for system-as-transient state.")

(defrecord Application [:shutdown-fn])

and

(ns novate.services.activator
  "Responsible for bootstrapping the application by loading certain namespaces and invoking certain functions, guided by data in JAR manifests."
  (:gen-class)
  (:require [clojure.edn :as edn]
            [clojure.java.io :as io]
            [novate.util.logging :as l]
            [novate.services
             [initialization :as init]
             [ordering :as o]])
  (:import [java.io PushbackReader]))

The error was attributed to this file.

Comment by Andy Fingerhut [ 13/Sep/13 11:48 AM ]

Patch clj-1261-v1.txt throws an exception if any fields given to defrecord or deftype are not symbols. They are CompilerExceptions, so include an accurate file, line, and column number.

Comment by Andy Fingerhut [ 13/Sep/13 11:57 AM ]

Updated description to give minimal test case.

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

Patch clj-1261-2.diff is identical to clj-1261-v1.txt except that it applies cleanly to latest master. The only change was to some lines of context due to recent commits to Clojure.

Comment by Alex Miller [ 18/Apr/14 1:16 PM ]

I think the patch is ok but I have two suggestions in the error message - first, include the record/type ns+name (I think the classname in the patched fn is what you want). Second, I think the wording could be adjusted a bit and the parens should go away - those look like but don't actually have meaning in the original context (since you are filtering out the symbols). Maybe something like:

"defrecord and deftype fields must be symbols, useclj16.init.Application had: :shutdown-fn, :foo-bar"





[CLJ-738] <= is incorrect when args include Double/NaN Created: 14/Feb/11  Updated: 18/Apr/14

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

Type: Defect Priority: Trivial
Reporter: Jason Wolfe Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: math
Environment:

Mac OS X, Java 6


Attachments: File 738.diff     File 738-tests.diff     File clj-738-v2.diff    
Patch: Code and Test
Approval: Vetted

 Description   
user=> (<= (long Double/NaN) 1)
true     
user=> (<= Double/NaN 1)
false  ;; should match primitive version

Cause: The problem was that the logic for lte/gte depended on the fact that lte is equivalent to !gt.
However, in Java, this assumption is invalid - any comparison involving NaN always yields false.

Solution: The fix was to adding lte and gte methods to Numbers.Ops directly, rather than implementing everything in terms of lt. This was the only fix I could see that didn't incur the cost of runtime checks for NaN.

Patch: 738.diff, 738-tests.diff

Screened by:



 Comments   
Comment by Jason Wolfe [ 14/Feb/11 7:18 PM ]

The source of the issue seems to be incorrect treatment of boxed NaN:

user> (<= 1000 (Double. Double/NaN))
true
user> (<= 1000 (double Double/NaN))
false

Comment by Alexander Taggart [ 28/Feb/11 11:14 PM ]

Primitive comparisons use java's primitive operators directly, which always return false for NaN, even when testing equality between two NaNs.

In clojure, Number comparisons are all logical variations around calls to Numbers.Ops.lt(Number, Number). So a call to (<= x y) is actually a call to (not (< y x)), which eventually uses the primitive < operator. Alas that logical premise doesn't hold when dealing with NaN:

user=> (<= 1 Double/NaN)
false
user=> (not (< Double/NaN 1))
true

So the bug is not that boxed NaN is treated incorrectly, but rather:

user> (<= 1000 (Double. Double/NaN)) ; becomes !(NaN < 1000) 
true
user> (<= 1000 (double Double/NaN))  ; becomes (1000 <= NaN)
false

In the original example, since there are more than two args, the primitive looking args were boxed:

user=> (<= 10 Double/NaN 1) ; equivalent to logical-and of the following
true
user=> (<= 10 (Double. Double/NaN))  ; becomes !(NaN < 10)
true
user=> (<= (Double. Double/NaN) 1)   ; becomes !(1 < NaN)
true

Note however that java object comparisons for NaNs behave differently: NaN is the largest Double, and NaNs equal each other (see the javadoc).

If we make object NaN comparisons always return false, we would need to add the rest of the comparison methods to Numbers.Ops. Yet doing so could also make collection sorting algorithms behave oddly, deviating from sorting written in java. Besides, (= NaN NaN) => false is annoying.

Clojure already throws out the notion of error-free dividing by zero (which for doubles would otherwise result in NaN or Infinity, depending on the dividend). Perhaps we could similarly error on NaNs passed to clojure numeric ops. They seem to be more trouble than they're worth. That said, people smarter than me thought they were useful.

Then there's that -0.0 nonsense...

Comment by Jouni K. Seppänen [ 19/Mar/11 3:02 PM ]

On current master, (<= x y) seems to be special-cased by the compiler, but when <= is called dynamically, the bug is still there:

user=> (<= 1 Float/NaN)
false
user=> (let [op <=] (op 1 Float/NaN))
true

Since CLJ-354 got marked "Completed", perhaps there was an attempt to fix this.

Comment by Alexander Taggart [ 19/Mar/11 6:45 PM ]

Using let forces calling <= as a function rather than inlining Numbers/lte, which means the args are treated as objects not primitives, thus the different behaviour as I discussed earlier.

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

Rich, what should the behavior be?

Comment by Jouni K. Seppänen [ 29/Jun/11 1:22 AM ]

My suggestion for the behavior is to follow Java (Java Language Specification §15.20.1) and IEEE 754. The java.sun.com site seems to be down right now, but here's a Google cache link:

http://webcache.googleusercontent.com/search?q=cache:http://java.sun.com/docs/books/jls/third_edition/html/expressions.html#15.20.1

Comment by Rich Hickey [ 29/Jul/11 7:48 AM ]

It should work the same as primitive double in all cases

Comment by Luke VanderHart [ 26/Aug/11 11:33 AM ]

Added patches. The problem was that our logic for lte/gte depended on the fact that lte is equivalent to !gt.

However, in Java, this assumption is invalid - any comparison involving NaN always yields false.

The fix was to adding lte and gte methods to Numbers.Ops directly, rather than implementing everything in terms of lt. This was the only fix I could see that didn't incur the cost of runtime checks for NaN.

Comment by Alex Miller [ 04/Mar/14 3:18 PM ]

David Welte noted: "CLJ-738 is marked Closed but the attached patch is has not been applied and both Clojure 1.5.1 and 1.6.0-beta2 exhibit the bad behavior listed in CLJ-738. The issue that CLJ-738 is that (<= (Double. Double/NaN) 1) evaluates to true while (<= Double/NaN 1) evaluates to false."

I concur that this patch was not applied. It looks likely that Luke marked this as Resolved when the patch was ready instead of whatever state change would have been appropriate at the time of the ticket (the process has varied over the years). AFAICT, this ticket should be open and Vetted (accepted as a problem) but probably needs release targeting and an updated patch based on current code.

Comment by Andy Fingerhut [ 05/Mar/14 12:32 PM ]

Might want to make the "Fix Version" on this ticket empty so it is back on the JIRA state chart as Vetted.

Comment by Andy Fingerhut [ 18/Apr/14 11:41 AM ]

Patch clj-738-v2.diff is identical in content to Luke's 2 patches 738.diff and 738-tests.diff, and includes them both, retaining his authorship. The only change is to a few context lines so that the new patch applies cleanly to latest master, whereas the older patches did not.





[CLJ-740] Unnecessary boxing of primitives in case form Created: 17/Feb/11  Updated: 01/Mar/11

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

Type: Defect Priority: Major
Reporter: Mikhail Kryshen Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

Found this while profiling some performance-critical code.

Consider the following Clojure function:

(defn test-case ^double [^long i ^double d1 ^double d2]
  (case (int i)
    0 d1
    d2))

Current Clojure 1.3 snapshot compiles it to:

public final double invokePrim(long, double, double)   throws java.lang.Exception;
  Code:
   0:	lload_1
   1:	invokestatic	#67; //Method clojure/lang/RT.intCast:(J)I
   4:	istore	7
   6:	iload	7
   8:	i2l
   9:	invokestatic	#73; //Method clojure/lang/Numbers.num:(J)Ljava/lang/Number;
   12:	invokestatic	#79; //Method clojure/lang/Util.hash:(Ljava/lang/Object;)I
   15:	iconst_0
   16:	ishr
   17:	iconst_1
   18:	iand
   19:	tableswitch{ //0 to 0
		0: 36;
		default: 58 }
   36:	iload	7
   38:	i2l
   39:	invokestatic	#73; //Method clojure/lang/Numbers.num:(J)Ljava/lang/Number;
   42:	getstatic	#45; //Field const__3:Ljava/lang/Object;
   45:	invokestatic	#83; //Method clojure/lang/Util.equals:(Ljava/lang/Object;Ljava/lang/Object;)Z
   48:	ifeq	58
   51:	dload_3
   52:	invokestatic	#88; //Method java/lang/Double.valueOf:(D)Ljava/lang/Double;
   55:	goto	63
   58:	dload	5
   60:	invokestatic	#88; //Method java/lang/Double.valueOf:(D)Ljava/lang/Double;
   63:	checkcast	#92; //class java/lang/Number
   66:	invokevirtual	#96; //Method java/lang/Number.doubleValue:()D
   69:	dreturn
}

This bytecode contains boxing of primitives (calls to clojure/lang/Numbers.num and java/lang/Double.valueOf) and calls to clojure/lang/Util.hash and clojure/lang/Util.equals that does not seem necessary.

At 60-66 primitive double is boxed into Double only to be converted back into primitive.

The equivalent Java code compiles to much simpler and faster bytecode:

public double testCase(long, double, double);
  Code:
   0:	lload_1
   1:	l2i
   2:	lookupswitch{ //1
		0: 20;
		default: 22 }
   20:	dload_3
   21:	dreturn
   22:	dload	5
   24:	dreturn
}


 Comments   
Comment by Alexander Taggart [ 28/Feb/11 2:16 PM ]

Improved via patch on CLJ-426.

(defn test-case ^double [^long i ^double d1 ^double d2]
  (case (int i)
    0 d1
    d2))

now emits as

 0  lload_1 [i]
 1  invokestatic clojure.lang.RT.intCast(long) : int [67]
 4  istore 7 [G__7903] // let-bound expression
 6  iload 7 [G__7903]
 8  tableswitch default: 32
      case 0: 28
28  dload_3 [d2]
29  goto 34
32  dload 5 [arg2]
34  dreturn

or if the int cast of the expression is omitted:

 0  lload_1 [i]
 1  lstore 7 [G__7903] // let-bound expression
 3  lload 7 [G__7903]
 5  l2i
 6  tableswitch default: 35
      case 0: 24
24  lconst_0           // match, verify long expr wasn't truncated
25  lload 7 [G__7903]
27  lcmp
28  ifne 35
31  dload_3 [d2]
32  goto 37
35  dload 5 [arg2]
37  dreturn




[CLJ-705] Make sorted maps and sets implement j.u.SortedMap and SortedSet interfaces Created: 05/Jan/11  Updated: 02/Jun/12

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

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


 Comments   
Comment by Andy Fingerhut [ 02/Jun/12 2:29 PM ]

This might be a duplicate of CLJ-248. See that one before working on this one, at least.





[CLJ-1108] Allow to specify an Executor instance to be used with future-call Created: 18/Nov/12  Updated: 27/Dec/12

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

Type: Enhancement Priority: Major
Reporter: Max Penet Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File bac37b91230d8e4ab3a1e6042a6e8c4b7e9cbf53.patch     Text File clj-1108-enhance-future-call-patch-v2.txt    
Patch: Code

 Description   

This adds an arity to future-call that expects a java.util.concurrent/ExecutorService instance to be used instead of clojure.lang.Agent/soloExecutor.



 Comments   
Comment by Andy Fingerhut [ 26/Dec/12 4:50 PM ]

Rich Hickey committed a change on Dec 21, 2012 to the future-call function that made the patch bac37b91230d8e4ab3a1e6042a6e8c4b7e9cbf53.patch dated Nov 18 2012 no longer apply cleanly.

clj-1108-enhance-future-call-patch-v2.txt dated Dec 26 2012 is identical to that earlier patch, except it has been updated to apply cleanly to the latest master.

It would be best if Max Penet, author of the earlier patch, could verify I've merged his patch to the latest Clojure master correctly.

Comment by Max Penet [ 27/Dec/12 2:25 AM ]

It's verified, it applies correctly to the latest master 00978c76edfe4796bd6ebff3a82182e235211ed0 .

Thanks Andy.





[CLJ-859] Built in dynamic vars don't have :dynamic metadata Created: 19/Oct/11  Updated: 24/Feb/12

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

Type: Defect Priority: Major
Reporter: Anthony Simpson Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

I'm sure 'built in' is probably not the right term here, but I'm not sure what these are called.

I ran into this issue earlier today while fixing a bug in clojail. Built in vars, particularly ones listed here without a source link: http://clojure.github.com/clojure/clojure.core-api.html, do not have :dynamic metadata despite being dynamic. This includes *in*, *out*, and *err* among others. Here are some examples:

user=> (meta #'*err*)
{:ns #<Namespace clojure.core>, :name *err*, :added "1.0", :doc "A java.io.Writer object representing standard error for print operations.\n\n  Defaults to System/err, wrapped in a PrintWriter"}
user=> (meta #'*in*)
{:ns #<Namespace clojure.core>, :name *in*, :added "1.0", :doc "A java.io.Reader object representing standard input for read operations.\n\n  Defaults to System/in, wrapped in a LineNumberingPushbackReader"}
user=> (meta #'*out*)
{:ns #<Namespace clojure.core>, :name *out*, :added "1.0", :doc "A java.io.Writer object representing standard output for print operations.\n\n  Defaults to System/out, wrapped in an OutputStreamWriter", :tag java.io.Writer}
user=> (meta #'*ns*)
{:ns #<Namespace clojure.core>, :name *ns*, :added "1.0", :doc "A clojure.lang.Namespace object representing the current namespace.", :tag clojure.lang.Namespace}


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

This recent discussion on the users list seems relevant: Should intern obey :dynamic?.

It seems to boil down to this the information that a Var is dynamic (or not) is duplicated. Once as metadata with the key :dynamic, and once as a boolean field on the Var class which implements Clojure's variables. This boolean can be obtained by calling the method isDynamic() on the Var.

The confusion arises because apparently :dynamic and .isDynamic can get out of sync with each other. .isDynamic is the source of truth in this case.

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

Compiler$Parser.parse(...) finds the :dynamic entry left in the metadata of the symbol by LispReader and passes this on when creating a new DefExpr, which in turn, generates the code that will call setDynamic(...) on the var when it is created at runtime.

As far as I can tell, the :dynamic entry is irrelevant once that has occurred. It seems to be implemented only as a way to communicate (by way of the reader) with the compiler. Once the compiler's gotten the message, it isn't needed anymore. Keeping it around seems to just cause confusion.

Dynamic vars created by the Java layer of Clojure core don't use the :dynamic mechanism, they just setDynamic() directly. That's why they don't have :dynamic in their meta-data map.

  • Perhaps the compiler should elide :dynamic from the metadata map available at runtime, since it has served its purpose.
  • Perhaps Clojure should supply the function dynamic?.
    (defn dynamic? [^clojure.lang.Var v] (.isDynamic v))

Or, perhaps one might consider, for 1.4, replacing :dynamic altogether and just enforcing the established naming convention: *earmuffs* are dynamic, everything-else isn't. (The compile warns about violations of this convention in 1.3.)

Comment by Andy Fingerhut [ 24/Feb/12 11:39 AM ]

I recently noticed several lines like this one in core.clj. Depending upon how many symbols are like this, perhaps this method could be used to add :dynamic metadata to symbols in core, along with a unit test to verify that all symbols in core have :dynamic if and only if .isDynamic returns true?

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

Ugh. In my previous comment, by "several lines like this one" I meant to paste the following as an example:

(alter-meta! #'agent assoc :added "1.0")





[CLJ-1016] Global scope overrides lexical scope for classes (Clojure assumes no classes in default package / Clojure cannot handle yFiles JARs in classpath) Created: 21/Jun/12  Updated: 24/May/13

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

Type: Defect Priority: Major
Reporter: Edward Z. Yang Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None

Attachments: Text File collision-workaround.patch    

 Description   

The most visible symptom of this bug is having a class named 'w' (default package) in your classpath (such classes are produced by Java obfuscation tools such as yFiles) and then attempting to load Clojure's core class. For example:

java -cp hotspotapi.jar:clojure-1.4.0-slim.jar clojure.main

(where hotspotapi.jar is a stereotypical example of an obfuscated JAR) results in:

Exception in thread "main" java.lang.ExceptionInInitializerError
at clojure.main.<clinit>(main.java:20)
Caused by: java.lang.NoSuchFieldException: close, compiling:(clojure/core.clj:6139)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6462)
at clojure.lang.Compiler.analyze(Compiler.java:6262)
at clojure.lang.Compiler.analyze(Compiler.java:6223)
at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5618)
at clojure.lang.Compiler$TryExpr$Parser.parse(Compiler.java:2178)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6455)
at clojure.lang.Compiler.analyze(Compiler.java:6262)
at clojure.lang.Compiler.analyze(Compiler.java:6223)
at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5618)
at clojure.lang.Compiler$LetExpr$Parser.parse(Compiler.java:5919)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6455)
at clojure.lang.Compiler.analyze(Compiler.java:6262)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6443)
at clojure.lang.Compiler.analyze(Compiler.java:6262)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6443)
at clojure.lang.Compiler.analyze(Compiler.java:6262)
at clojure.lang.Compiler.analyze(Compiler.java:6223)
at clojure.lang.Compiler$BodyExpr$Parser.parse(Compiler.java:5618)
at clojure.lang.Compiler$FnMethod.parse(Compiler.java:5054)
at clojure.lang.Compiler$FnExpr.parse(Compiler.java:3674)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6453)
at clojure.lang.Compiler.analyze(Compiler.java:6262)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6443)
at clojure.lang.Compiler.analyze(Compiler.java:6262)
at clojure.lang.Compiler.access$100(Compiler.java:37)
at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:518)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6455)
at clojure.lang.Compiler.analyze(Compiler.java:6262)
at clojure.lang.Compiler.analyze(Compiler.java:6223)
at clojure.lang.Compiler.eval(Compiler.java:6515)
at clojure.lang.Compiler.load(Compiler.java:6952)
at clojure.lang.RT.loadResourceScript(RT.java:359)
at clojure.lang.RT.loadResourceScript(RT.java:350)
at clojure.lang.RT.load(RT.java:429)
at clojure.lang.RT.load(RT.java:400)
at clojure.lang.RT.doInit(RT.java:436)
at clojure.lang.RT.<clinit>(RT.java:318)
... 1 more
Caused by: java.lang.NoSuchFieldException: close
at java.lang.Class.getField(Class.java:1537)
at clojure.lang.Compiler$StaticFieldExpr.<init>(Compiler.java:1180)
at clojure.lang.Compiler$HostExpr$Parser.parse(Compiler.java:923)
at clojure.lang.Compiler.analyzeSeq(Compiler.java:6455)
... 37 more
Could not find the main class: clojure.main. Program will exit.

To understand what is going on, consider this simple test:

import java.io.StringReader;

import clojure.lang.Compiler;
import clojure.lang.RT;

public class Test {
public static void main(String[] args) { RT.var("clojure.core", "require"); String s = "(let [mumble (new java.io.StringReader \"\")] (. mumble close))"; Compiler.load(new StringReader(s)); }
}

It should be clear that 'mumble' in the dot operator is referencing the locally defined mumble. However, if we define a class named 'mumble' in the default package, Clojure picks that one up instead.

To forestall any objections: yes, we know that placing classes in the default package is extremely poor form. Point of the matter is, the Java ecosystem is extremely diverse and there are a lot of JARs people may not have control over. While one might argue, "Don't put classes in the default namespace", point of the matter is, Clojure is wrong here, and these situations arise in practice, through no fault of the implementer.



 Comments   
Comment by Edward Z. Yang [ 21/Jun/12 11:01 AM ]

Here is a workaround patch which makes this error less likely to occur.

Comment by Andy Fingerhut [ 27/Aug/12 7:37 PM ]

Edward, it is Rich Hickey's policy only to consider for inclusion in Clojure patches written by people who have signed a Contributor Agreement: http://clojure.org/contributing

Were you interested in becoming a contributor?

Comment by Edward Z. Yang [ 27/Aug/12 9:24 PM ]

Sure, although the patch attached is emphatically not the one you want to actually applying, since it only band-aids the problem.

Comment by Andy Fingerhut [ 24/May/13 1:21 PM ]

I am not sure, but this ticket may be related to CLJ-1171. At least, there the issue was a global name not being shadowed by a local name bound with let. That seems similar to this issue.





[CLJ-2] Scopes Created: 15/Jun/09  Updated: 27/Jul/13

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

Type: Enhancement Priority: Major
Reporter: Assembla Importer Assignee: Rich Hickey
Resolution: Unresolved Votes: 1
Labels: None

Attachments: Text File scopes-spike.patch    

 Description   

Add the scope system for dealing with resource lifetime management



 Comments   
Comment by Assembla Importer [ 24/Aug/10 11:43 AM ]

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

Comment by Assembla Importer [ 24/Aug/10 11:43 AM ]

richhickey said: Updating tickets (#8, #42, #113, #2, #20, #94, #96, #104, #119, #124, #127, #149, #162)

Comment by Stuart Halloway [ 12/Jul/11 8:26 AM ]

Patch demonstrates idea, not ready for prime time.

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

I think the decision of having to specify either a Closeable resource or a close function for an existing non-Closeable resource in with-open is quite awkward, because they have completely different meaning.

  (let [foo (open-my-custom-resource "foo.bar")]
    (with-open [r (reader "foo.txt")
                foo #(.terminate foo)]
      (do-stuff r foo)))

I think a CloseableResource protocol that can be extended to custom types as implemented in the patch to CLJ-308 is somewhat easier to use. Extend it once, and then you can use open-my-custom-resource in with-open just like reader/writer and friends...

That said, Scopes can still be useful, but I'd vote for handling the "how should that resource be closed" question by a protocol. Then the with-open helper can simply add

(swap! *scope* conj (fn [] (clojure.core.protocols/close ~(bindings 0))))

and cleanup-scope only needs to apply each fn without having to distinguish Closeables from fns.





[CLJ-666] Add support for Big* numeric types to Reflector Created: 29/Oct/10  Updated: 27/Jul/13

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

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

Attachments: Text File 0001-Add-Big-support-to-Reflector.patch     Text File 0001-Add-Big-support-to-Reflector-Updated.patch    
Patch: Code and Test

 Description   

This should work as expected, for example:

(Integer. 1N)

Probably for BigInt, BigInteger, and BigDecimal.

Method to look at is c.l.Reflector.paramArgTypeMatch, per Rich in irc.



 Comments   
Comment by Colin Jones [ 30/Mar/11 11:52 PM ]

Questions posed on the clojure-dev list around how this impacts bit-shift-left: http://groups.google.com/group/clojure-dev/browse_thread/thread/2191cbf0048d8ca6

Comment by Alexander Taggart [ 31/Mar/11 12:42 AM ]

Patch on CLJ-445 fixes this as well.

Comment by Colin Jones [ 27/Apr/11 4:41 PM ]

This patch fails a test around bit-shifting a BigInt: `(bit-shift-left 1N 10000)`. The reason is that the patch changes the dispatch of (BigInt, Long) from (Object, Object) to (long, int).

Clearly this can't be applied (unless another change makes it possible), but I'm putting it up as a start of the conversation.

Comment by Alexander Taggart [ 27/Apr/11 5:26 PM ]

My comment from the mailing list:

If the test breaks it likely means Numbers.shiftLeft(long,int) was
selected over Numbers.shiftLeft(Object,Object). Given that 1N is an
Object (one that can exceed the size of a long), the method selection
is incorrect, thus the patch is broken.


The suggestion of "simply" modifying paramArgTypeMatch is not sufficient since the mechanism for preferring one method over another lives in Compiler, and isn't smart enough to make these sorts of decisions.

Comment by Christopher Redinger [ 28/Apr/11 9:21 AM ]

Considering moving this out of Release.next - soliciting comments from Chas.

Comment by Chas Emerick [ 28/Apr/11 9:41 AM ]

I'm afraid I don't have any particular insight into the issues involved at this point. I ran into the problem originally noted a while back, and opened the ticket at Rich's suggestion. I'm sorry if the text of the ticket led anyone down unfruitful paths…

Comment by Luke VanderHart [ 29/Apr/11 10:01 AM ]

The issues relating to bitshift are moot since the decision was made that bit-shifts are only for 32/64 bit values.

Still a valid issue, but de-prioritized as per Rich.

Comment by Alex Ott [ 25/Jun/12 7:19 AM ]

Modified version of original patch

Comment by Andy Fingerhut [ 26/Jun/12 1:38 PM ]

Alex, would you mind attaching it with a unique file name? I know that JIRA lets us create multiple attachments with the same file name, and I know we can tell them apart by date and the account of the person who uploaded the attachment, but giving them the same name only seems to invite confusion.

Comment by Alex Ott [ 28/Jun/12 1:00 PM ]

Renamed updated patch to unique name





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

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

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

Attachments: File thread-bound.diff    
Patch: Code

 Description   

thread-bound? returns true for a non-binding thread, this result (according to the docstring) implies that set! should succeed. However, thread-bound? does not check that any binding that might exist was created by the current thread, and calling set! fails with an exception when it is called from a non-binding thread, even though thread-bound? returns true.

thread-bound? should return false if there is a binding, and that binding was not established by the current thread.

Here is an example REPL session where a thread establishes a binding, those bindings are conveyed to a second thread, the second thread checks thread-bound? to see if it can set the binding, thread-bound? returns true indicating that the binding can be set, the second thread tries to set the binding, and the second thread gets an IllegalStateException:

    Clojure 1.5.1
    user=> (def ^:dynamic *set-me* nil)
    #'user/*set-me*
    user=> (defn try-to-set [] (binding [*set-me* 1] (doall (pcalls #(if (thread-bound? #'*set-me*) (set! *set-me* (inc *set-me*)))))))
    #'user/try-to-set
    user=> (try-to-set)
    IllegalStateException Can't set!: *set-me* from non-binding thread  clojure.lang.Var.set (Var.java:230)
    user=> 


 Comments   
Comment by Paul Stadig [ 01/Oct/12 10:07 AM ]

I have attached a patch that changes clojure.lang.Var and clojure.core/thread-bound? to only return true if a Var is set!-able.

Comment by Alex Miller [ 19/Aug/13 12:16 PM ]

REPL example?

Comment by Joe Gallo [ 20/Aug/13 7:55 AM ]

Sure thing, Alex – here's a repl example I just ran this morning.

; nREPL 0.1.7
user> (def ^:dynamic *set-me* nil)
#'user/*set-me*
user> (defn try-to-set [] (binding [*set-me* 1] (doall (pcalls #(if (thread-bound? #'*set-me*) (set! *set-me* (inc *set-me*)))))))
#'user/try-to-set
user> (try-to-set)
IllegalStateException Can't set!: *set-me* from non-binding thread  clojure.lang.Var.set (Var.java:230)
user>




[CLJ-1237] reduce gives a SO for pathological seqs Created: 27/Jul/13  Updated: 25/Aug/13

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

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

1.5.1


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

 Description   

reduce gives a StackOverflowError on long sequences that contain many transitions between chunked and unchunked:

(->> (repeat 50000 (cons :x [:y]))
     (apply concat)
     (reduce (constantly nil)))
;; throws StackOverflowError

Such a sequence is well behaved under most other sequence operations, and its underlying structure can even be masked such that reduce succeeds:

(->> (repeat 50000 (cons :x [:y]))
     (apply concat)
     (take 10000000)
     (reduce (constantly nil)))
;; => nil

I don't think Clojure developers normally worry about mixing chunked and unchunked seqs, so the existence of such a sequence is not at all unreasonable (and indeed this happened to me at work and was very difficult to debug).

It seems obvious what causes this given the implementation of reduce – it bounces back and forth between the chunked impl and the unchunked impl, consuming more and more stack as it goes. Without proper tail call optimization, it's not obvious to me what a good fix would be.

Presumed bad solutions

Degrade to naive impl after first chunk

In the IChunkedSeq implementation, instead of calling internal-reduce when the
sequence stops being chunked, it could have an (inlined?) unoptimized implementation,
ensuring that no further stack space is taken up. This retains the behavior that a
generic seq with a chunked tail will still run in an optimized fashion, but a seq with
two chunked portions would only be optimized the first time.

Use clojure.core/trampoline

This would presumably work, but requires wrapping the normal return values from all
implementations of internal-reduce.

Proposed Solution

(attached as CLJ-1237c.patch)

Similar to using trampoline, but create a special type (Unreduced) that signals
an implementation change. The two implementation-change points in internal-reduce
(in the IChunkedSeq impl and the Object impl) are converted to return an instance
of Unreduced instead of a direct call to internal-reduce.

Then seq-reduce is converted to check for instances of Unreduced before returning,
and recurs if it finds one.

Pros

  • Only requires one additional check in most cases
  • Reduces stack usage for existing heterogeneous reductions that weren't extreme enough to crash
  • Should be compatible with 3rd-party implementations of internal-reduce, which can still use the old style (direct recursive calls to internal-reduce) or the optimized style if desired.

Cons

  • internal-reduce is slightly more complicated
  • There's an extra check at the end of seq-reduce – is that a performance worry?


 Comments   
Comment by Gary Fredericks [ 25/Aug/13 4:13 PM ]

Added patch.





[CLJ-112] GC Issue 108: All Clojure interfaces should specify CharSequence instead of String when possible Created: 17/Jun/09  Updated: 26/Aug/13

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

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


 Description   
Reported by redchin, Apr 20, 2009

rhickey: unlink: then just use a map {:escaped true :val "foo"}

unlink: What I meant is, everything in between would want to see something 
String-y, not caring whether it's a String or MyString.

hiredman: unlink: if you use something that implements CharSequence and 
IMeta (I think it's IMeta) you get something that is basically a String, 
but with metadata

rhickey: what hiredman said

hiredman: ideally most things would not specify String but CharSequence in 
their interface

hiredman: but somehow I doubt that is case

unlink: ok.

unlink: Good to know.

rhickey: hiredman: unfortunately that's not true of some of Clojure - could 
you enter an issue for it please - use CharSequence when possible?


 Comments   
Comment by Assembla Importer [ 24/Aug/10 3:45 AM ]

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

Comment by Assembla Importer [ 24/Aug/10 3:45 AM ]

richhickey said: Updating tickets (#8, #19, #30, #31, #126, #17, #42, #47, #50, #61, #64, #69, #71, #77, #79, #84, #87, #89, #96, #99, #103, #107, #112, #113, #114, #115, #118, #119, #121, #122, #124)





[CLJ-259] clojure.lang.Reflector.invokeMatchingMethod is not complete (rejects pontentially valid method invocations) Created: 03/Feb/10  Updated: 26/Aug/13

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

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


 Description   

There exists invoke expressions on instances, where Java is able to perform the call, yet clojure is not reflectively.
The problem is when the declaringClass of the found method is not public then the call to getAsMethodOfPublicBase uses the found method or searching for classes/interfaces that contain/define this method yet are declared publicly.

This restricts the possible search space. I suggest that if target is not null (e.g. is not a static method), the target.getClass() should be used instead as a root for getAsMethodOfPublicBase.
This fixes my issue.



 Comments   
Comment by Assembla Importer [ 24/Aug/10 3:12 PM ]

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

Comment by Assembla Importer [ 24/Aug/10 3:12 PM ]

richhickey said: How about some sample case that demonstrates the problem?

Comment by Assembla Importer [ 24/Aug/10 3:12 PM ]

hiredman said: Related association with ticket #126 was added





[CLJ-113] GC Issue 109: RT.load's "don't load if already loaded" mechanism breaks ":reload-all" Created: 17/Jun/09  Updated: 26/Aug/13

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

Type: Defect Priority: Major
Reporter: Anonymous Assignee: Stephen C. Gilardi
Resolution: Unresolved Votes: 0
Labels: None


 Description   
Reported by scgilardi, Apr 24, 2009

What (small set of) steps will reproduce the problem?

"require" and "use" support a ":reload-all" flag that is intended to  
cause the specified libs to be reloaded along with all libs on which  
they directly or indirectly depend. This is implemented by temporarily  
binding a "loaded-libs" var to the empty set and then loading the  
specified libs.

AOT compilation added another "already loaded" mechanism to  
clojure.lang.RT.load() which is currently not sensitive to a "reload-
all" being in progress and breaks its operation in the following case:

        A, B, and C are libs
        A depends on B. (via :require in its ns form)
        B depends on C. (via :require in its ns form)
        B has been compiled (B.class is on classpath)

        At the repl I "require" A which loads A, B, and C (either from
class files or clj files)
        I modify C.clj
        At the repl I "require" A with the :reload-all flag, intending to  
pick up the changes to C
        C is not reloaded because RT.load() skips loading B: B.class
exists, is already loaded, and B.clj hasn't changed since it was compiled.


What is the expected output? What do you see instead?

I expect :reload-all to be effective. It isn't.

What version are you using?

svn 1354, 1.0.0RC1

Was this discussed on the group? If so, please provide a link to the
discussion:

http://groups.google.com/group/clojure/browse_frm/thread/9bbc290321fd895f/e6a967250021462a#e6a967250021462a

Please provide any additional information below.

I'll upload a patch soon that creates a "*reload-all*" var with a  
root binding of nil and code to bind it to true when the current  
thread has a :reload-all call pending. When *reload-all* is true,  
RT.load() will (re)load all libs from their ".clj" files even if  
they're already loaded.

The fix for this may need to be coordinated with a fix for issue #3.


 Comments   
Comment by Assembla Importer [ 24/Aug/10 3:45 AM ]

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

Comment by Assembla Importer [ 24/Aug/10 3:45 AM ]

richhickey said: Updating tickets (#8, #19, #30, #31, #126, #17, #42, #47, #50, #61, #64, #69, #71, #77, #79, #84, #87, #89, #96, #99, #103, #107, #112, #113, #114, #115, #118, #119, #121, #122, #124)

Comment by Assembla Importer [ 24/Aug/10 3:45 AM ]

richhickey said: Updating tickets (#8, #42, #113, #2, #20, #94, #96, #104, #119, #124, #127, #149, #162)

Comment by Kevin Downey [ 08/Aug/11 7:40 PM ]

seems like the code that is emitted in the static init for namespace classes could be emitted into a init_ns() static method and the static init could call init_ns(). then RT.load could call init_ns() to get the behavior of reloading an AOT compiled namespace.

Comment by Kevin Downey [ 09/Aug/11 8:31 PM ]

looking at the compiler it looks like most of what I mentioned above is already implemented, just need RT to reflectively call load() on the namespace class in the right place





[CLJ-270] defn-created fns inherit old metadata from the Var they are assigned to Created: 13/Feb/10  Updated: 26/Aug/13

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

Type: Defect Priority: Major
Reporter: Anonymous Assignee: Rich Hickey
Resolution: Unresolved Votes: 0
Labels: None


 Description   
  • What (small set of) steps will reproduce the problem?

user> (def #^{:foo "bar"} x 5)
#'user/x
user> (meta #'x)
{:ns #<Namespace user>, :name x, :file "NO_SOURCE_FILE", :line 1, :foo "bar"}
user> (defn x [] 5)
#'user/x
user> (meta #'x)
{:ns #<Namespace user>, :name x, :file "NO_SOURCE_FILE", :line 1, :arglists ([])}
user> (meta x)
{:ns #<Namespace user>, :name x, :file "NO_SOURCE_FILE", :line 1, :foo "bar"}

  • What is the expected output? What do you see instead?

I expect (meta #'x) to evaluate to the value of the final (meta x) in the above.

  • What version are you using?

Current master (commit 61202d2ff6925002400a9843e8fbd080f3bef3a5).

  • Was this discussed on the group? If so, please provide a link to the discussion.

http://groups.google.com/group/clojure/browse_thread/thread/4c7151aa9c4d919c/d1b033ef5a13dd89?lnk=gst&q=off-by-one#d1b033ef5a13dd89

Prompted by discussion at

http://groups.google.com/group/clojure/browse_thread/thread/6553d48c981019eb/3c55b0bd43a5d8e9?lnk=gst&q=off-by-one#3c55b0bd43a5d8e9

  • Initial attempt at a diagnosis:

I think this is due to DefExpr's eval method binding the Var to init.eval() first and attaching the supplied metadata to the Var later – see Compiler.java lines 341-352. (Note the Var is always already in place when init.eval() is called, regardless of whether it existed prior to the evaluation of the def / defn.) Thus the init expression supplied by defn sees the old (and wrong) metadata on the Var.



 Comments   
Comment by Assembla Importer [ 24/Aug/10 6:23 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/270
Attachments:
dont-copy-val-metadata-onto-new-var-value-in-defn.patch - https://www.assembla.com/spaces/clojure/documents/dwK4yssayr37y_eJe5d-aX/download/dwK4yssayr37y_eJe5d-aX
0001-set-meta-on-vars-before-evaluating-their-init-see-27.patch - https://www.assembla.com/spaces/clojure/documents/arrhbiAI4r35lQeJe5cbLr/download/arrhbiAI4r35lQeJe5cbLr

Comment by Assembla Importer [ 24/Aug/10 6:23 AM ]

stu said: [file:dwK4yssayr37y_eJe5d-aX]

Comment by Assembla Importer [ 24/Aug/10 6:23 AM ]

stu said: The problem happens with defn, but not with def+fn, so I think the original diagnosis is incorrect.

Another stab at diagnosis: The defn macro copies metadata from a var onto its new value, so if you defn a var that already exists, the old var metadata becomes metadata on the new value. I don't know why this would be the right thing to do, and if you eliminate this behavior (see patch) all the Clojure and Contrib tests still pass.

If this is correct, please assign back to me and I will write tests. If this is wrong, please tell me what's going on here so I know how to write the tests.

Comment by Assembla Importer [ 24/Aug/10 6:23 AM ]

cgrand said: Child association with ticket #363 was added

Comment by Assembla Importer [ 24/Aug/10 6:23 AM ]

cgrand said: I think the original diagnosis is correct. Note that the behavior differ when def iscompiled:

user=> (def #^{:foo "bar"} x 5)
#'user/x
user=> (let [] (defn x [] 5))
#'user/x
user=> (meta x)
{:ns #<Namespace user>, :name x, :file "NO_SOURCE_PATH", :line 83, :arglists ([])}
user=> (meta #'x)
{:ns #<Namespace user>, :name x, :file "NO_SOURCE_PATH", :line 83, :arglists ([])}

See also http://groups.google.com/group/clojure/browse_thread/thread/6afd81896ca368b2#

Comment by Assembla Importer [ 24/Aug/10 6:23 AM ]

cgrand said: [file:arrhbiAI4r35lQeJe5cbLr]

Comment by Assembla Importer [ 24/Aug/10 6:23 AM ]

cgrand said: My patch aligns DefExpr.eval with DefExpr.emit (first set meta then eval the init value).

Comment by Assembla Importer [ 24/Aug/10 6:23 AM ]

cgrand said: Forget it, my current patch is broken.

Comment by Assembla Importer [ 24/Aug/10 6:23 AM ]

cgrand said: My current patch is broken because of #352, what is the rational for #352?





[CLJ-451] fn literals lack name/arglists/namespace metadata Created: 05/Oct/10  Updated: 26/Aug/13

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

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


 Description   

I would expect (meta (fn not-so-anonymous [a b c])) to include {:name not-so-anonymous :arglists ([a b c])} alongside line number information and possibly namespace/file as well, but currently it only includes :line.



 Comments   
Comment by Assembla Importer [ 05/Oct/10 12:29 AM ]

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





[CLJ-379] problem with classloader when run as windows service Created: 13/Jun/10  Updated: 26/Aug/13

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

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


 Description   

I found following error when I run clojure application as MS Windows service (via procrun from Apache Daemon project). When I tried to do 'require' during run-time, I got NullPointerException. This happened as baseLoader function from RT class returned null in such environment (the value of Thread.currentThread().getContextClassLoader()). (Although my app works fine when I run my application as standalone program, not as service).
This error was fixed by explicit setting of class loader with following code:

(.setContextClassLoader (Thread/currentThread) (java.lang.ClassLoader/getSystemClassLoader))

before any call to 'require'....

May be you need to modify 'baseLoader' function, so it will check is value of Thread.currentThread().getContextClassLoader() is null or not, and if null, then return value of java.lang.ClassLoader.getSystemClassLoader() ?



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

Converted from http://www.assembla.com/spaces/clojure/tickets/379
Attachments:
ticket-379-fix.diff - https://www.assembla.com/spaces/clojure/documents/c5XWHcD4yr34HveJe5ccaP/download/c5XWHcD4yr34HveJe5ccaP

Comment by Assembla Importer [ 24/Aug/10 10:40 AM ]

alexott said: possible fix is attached

Comment by Assembla Importer [ 24/Aug/10 10:40 AM ]

alexott said: [file:c5XWHcD4yr34HveJe5ccaP]





[CLJ-126] abstract superclass with non-public accessibility Created: 17/Jun/09  Updated: 26/Aug/13

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

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


 Description   

The following code works in Java 6 but not in Java 5:

(def Clojure 1.1.0-alpha-SNAPSHOT
user=> (def s (new StringBuilder "aaa"))
#'user/s
user=> (. s setCharAt (int 0) (char \a))
java.lang.Exception: Unable to resolve symbol: setCharAt in this context

This was discussed on the Clojure mailing list and Stephen C. Gillardi came up with the following conclusion:

_StringBuilder extends AbstractStringBuilder (though the JavaDoc docs lie and say it extends Object). AbstractStringBuilder has default accessibility (not public, protected, or private) which makes the class inaccessible to code outside the java.lang package. In both Java SE 5 and Java SE 6, StringBuilder does not contain a .setCharAt method definition. It relies on the inherited public method in AbstractStringBuilder. (I downloaded the source code for both versions from Sun to check.)

In Java SE 5, when Clojure checks whether or not .setCharAt on StringBuilder is public, it finds that it's a public method of a non-public base class and throws the exception you saw. (It looks like you're using a version of Clojure older than 18 May 2009 (Clojure svn r1371). Versions later than that print the more detailed message I saw.)

In Java SE 6, Clojure's checks for accessibility of this method succeed and the method call works.

I'm not sure whether or not Clojure could be modified to make this method call work in Java 5. Google searches turn up discussion that this pattern of using an undocumented abstract superclass with non-public accessibility is not common in the JDK._

This ticket is being filed in the event that Clojure can handle these types of situations somehow.



 Comments   
Comment by Assembla Importer [ 24/Aug/10 4:45 AM ]

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

Comment by Assembla Importer [ 24/Aug/10 4:45 AM ]

richhickey said: Updating tickets (#8, #19, #30, #31, #126, #17, #42, #47, #50, #61, #64, #69, #71, #77, #79, #84, #87, #89, #96, #99, #103, #107, #112, #113, #114, #115, #118, #119, #121, #122, #124)

Comment by Assembla Importer [ 24/Aug/10 4:45 AM ]

hiredman said: Related association with ticket #259 was added





[CLJ-396] Better support for multiple inheritance in hierarchies and multimethods Created: 07/Jul/10  Updated: 26/Aug/13

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

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


 Description   

While the hierarchies produced with 'derive' allow multiple parents per child, there is no way to indicate precedence between those parents, other than by laboriously specifying 'prefer-method' for every type X every multimethod. When 2 multimethods are both applicable to the supplied arguments, Clojure produces a nonspecific IllegalArgumentException containing only an error string. All this means that while Clojure does have an "inheritance" mechanism in the form of the ad hoc hierarchies, it is currently not really possible to implement multiple inheritance using the ad hoc hierarchy mechanism. 'Prefer-method' will not scale up to use in large applications with complex type hierarchies and heavy use of multimethods.

Some potential ways to solve this are:

  • allowing 'defmulti' to take a 'tie-breaker' function (tie-breaker [arglist speclist1 speclist2] ...) which is called instead of throwing an IllegalArgumentException, and must return the 'winning speclist'.
  • instead of throwing IllegalArgumentException, throw a TiedMultiMethodsException – the exception instance should contain the offending speclists, the function, and the arguments that were supplied.
  • allowing specification of precedence when using 'derive' (if only via a "last in = highest precedence" rule).

Paul



 Comments   
Comment by Assembla Importer [ 24/Aug/10 11:06 AM ]

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





[CLJ-401] Promote "seqable?" from contrib? Created: 13/Jul/10  Updated: 26/Aug/13

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

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


 Description   

This was vaguely discussed here and could potenntially help this ticket as well as be generally useful.

I don't speak for everyone but when I saw sequential? I assumed it would have the semantics that seqable? does. Just my opinion, I'd love to hear someone's who is more informed than mine.

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

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

And an example:

user=> (flatten #{1 2 3 #{4 5 {6 {7 8 9 10 #tok1-block-tok}}}})
(1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18)



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

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





[CLJ-115] GC Issue 111: Enable naming an array parameter for areduce Created: 17/Jun/09  Updated: 26/Aug/13

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

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


 Description   
Reported by bo...@boriska.com, Apr 28, 2009

Currently there is no way to access anonymous array parameter of areduce.

Consider:

(areduce (.. System getProperties values toArray) 
     i r 0 (some_expression))
some_expression has no way to access the array.

Per Rich:
--------------------
Yes, areduce would be nicer if it looked like a binding set:

(areduce [aname anarray, ret init] expr)
(areduce [aname anarray, ret init, start-idx  start-n] expr)
(areduce [aname anarray, ret init, start-idx  start-n, end-idx end-n]
expr) 
--------------------

This was discussed here:
http://groups.google.com/group/clojure/tree/browse_frm/thread/40597a8ac322bc37/8cf6b17328ea7e8b?rnum=1&_done=%2Fgroup%2Fclojure%2Fbrowse_frm%2Fthread%2F40597a8ac322bc37%2F8cf6b17328ea7e8b%3Ftvc%3D1%26pli%3D1%26#doc_9ea7e3c5d500ed3c


 Comments   
Comment by Assembla Importer [ 24/Aug/10 5:45 AM ]

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

Comment by Assembla Importer [ 24/Aug/10 5:45 AM ]

richhickey said: Updating tickets (#8, #19, #30, #31, #126, #17, #42, #47, #50, #61, #64, #69, #71, #77, #79, #84, #87, #89, #96, #99, #103, #107, #112, #113, #114, #115, #118, #119, #121, #122, #124)





[CLJ-319] TransactionalHashMap bug Created: 26/Apr/10  Updated: 26/Aug/13

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

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


 Description   

TransactionalHashMap computation of the bin is buggy. The implementation doesn't unset the sign bit before using it in accessing the bin array which in some cases cause an ArrayOutOfBoundException to be thrown.

As Rich Hickey has pointed out, this is an unsupported experimental Class and won't be fixed unless I provided a patch, so attached is the patch file.



 Comments   
Comment by Assembla Importer [ 01/Oct/10 4:06 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/319
Attachments:
TransactionalHashMap.java.patch - https://www.assembla.com/spaces/clojure/documents/cuuZnsuuWr36H0eJe5dVir/download/cuuZnsuuWr36H0eJe5dVir

Comment by Assembla Importer [ 01/Oct/10 4:06 PM ]

megabyte2021 said: [file:cuuZnsuuWr36H0eJe5dVir]: The patch file

Comment by Assembla Importer [ 01/Oct/10 4:06 PM ]

stu said: Please add a test case.





[CLJ-21] GC Issue 17: arity checking during compilation Created: 17/Jun/09  Updated: 26/Aug/13

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

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


 Description   
Reported by richhickey, Dec 17, 2008
Use available metadata to check calls when possible


 Comments   
Comment by Assembla Importer [ 24/Aug/10 2:44 PM ]

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





[CLJ-42] GC Issue 38: When using AOT compilation, "load"ed files are not reloaded on (require :reload 'name.space) Created: 17/Jun/09  Updated: 26/Aug/13

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

Type: Defect Priority: Major
Reporter: Anonymous Assignee: Stephen C. Gilardi
Resolution: Unresolved Votes: 0
Labels: None


 Description   
Reported by m...@kotka.de, Jan 07, 2009
What (small set of) steps will reproduce the problem?

1. Create a file src/foo.clj

cat >src/foo.clj <<EOF
(ns foo (:load "bar"))
EOF

2. Create a file src/bar.clj

cat >src/bar.clj <<EOF
(clojure.core/in-ns 'foo)
(def x 8)
EOF

3. Start Clojure Repl: java -cp src:classes clojure.main -r

4. Compile the namespace.

user=> (compile 'foo)
foo

5. Require the namespace
user=> (require :reload-all :verbose 'foo)
(clojure.core/load "/foo")
(clojure.core/load "/bar")

What is the expected output? What do you see instead?

6. Re-Require the namespace

user=> (require :reload-all :verbose 'foo)
(clojure.core/load "/foo")

Only the "master" file is loaded, but not the bar file.
Expected would have been to also load the bar file.
Changes to bar.clj are not reflected, and depending
on the setting (eg. using multimethods in foo from
a different namespace) code may be corrupted.

What version are you using?

SVN rev. 1195


 Comments   
Comment by Assembla Importer [ 24/Aug/10 6:44 AM ]

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

Comment by Assembla Importer [ 24/Aug/10 6:44 AM ]

richhickey said: Updating tickets (#8, #19, #30, #31, #126, #17, #42, #47, #50, #61, #64, #69, #71, #77, #79, #84, #87, #89, #96, #99, #103, #107, #112, #113, #114, #115, #118, #119, #121, #122, #124)

Comment by Assembla Importer [ 24/Aug/10 6:44 AM ]

richhickey said: Updating tickets (#42, #71)

Comment by Assembla Importer [ 24/Aug/10 6:44 AM ]

richhickey said: Updating tickets (#8, #42, #113, #2, #20, #94, #96, #104, #119, #124, #127, #149, #162)





[CLJ-213] Invariants and the STM Created: 01/Dec/09  Updated: 26/Aug/13

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

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


 Description   

(ticket requested here http://groups.google.com/group/clojure/browse_thread/thread/119311e89fa46806/4903ce25ff6deaa6#4903ce25ff6deaa6)

The general idea is to declare invariants inside a transaction and, when at commit time an invariant doesn't hold anymore, the transaction retries.
So it can both act as a kind of soft ensure or to specify actions that "partially commute".
Thus it would enable coarser refs.

See the attached file for quick prototype.

User code would looks like:

(invariant (@world :key))
(commute world update-in [:key] val-transform-fn)

This means the commute will occur only if (@world :key) returns the same value in-transaction and at commit point.



 Comments   
Comment by Assembla Importer [ 24/Aug/10 7:23 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/213
Attachments:
invariants.patch - https://www.assembla.com/spaces/clojure/documents/dd4kUS3MWr3QvMeJe5aVNr/download/dd4kUS3MWr3QvMeJe5aVNr





[CLJ-77] GC Issue 74: Clojure compiler emits too-large classfiles (results in ClassFormatError) Created: 17/Jun/09  Updated: 26/Aug/13

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

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


 Description   
Reported by cemer...@snowtide.com, Feb 10, 2009

The jvm has certain implementation limits around the maximum size of
classfiles, literal strings, method length, etc; however, in certain
circumstances, the Clojure compiler can currently emit classfiles that
violate some of those limitations, causing an error later when the
classfile is loaded.

While test coverage would necessarily detect this sort of problem on a
project-by-project basis when one's tests attempted to load a project's
classfiles, it seems like Clojure should do the following to ensure failure
as quickly as possible:

- throw an exception immediately if, while compiling a lib, it is detected
that the resulting classfile(s) would violate any classfile implementation
limits.  Ideally, the exception's message would detail what file and on
which line number the offending form is (e.g. if a method's bytecode would
be too long).  I can imagine that doing this may not be straightforward; a
reasonable stop-gap would be for the compiler to immediately attempt to
load the generated classfile in order to ensure up-front failure.

- emit a warning if any clojure form is read that would, upon being
compiled, require violating any of the classfile implementation limits; I
suspect that *most* people looking to generate classfiles would be doing so
in a "build" environment (rather than loading some code, tinkering, and
then using clojure.core/compile), but for those that aren't, I can imagine
there being a good deal of frustration around seeing that loading and using
some code successfully would eventually produce unusable classfiles.

I've appended a sample stack trace emitted by java when it attempted to
load a too-long method implementation (which was produced by embedding a
large list literal in a compiled lib).

Exception in thread "main" java.lang.ClassFormatError: Invalid method  
Code length 105496 in class file com/foo/MyClass__init
         at java.lang.ClassLoader.defineClass1(Native Method)
         at java.lang.ClassLoader.defineClass(ClassLoader.java:675)
         at  
java.security.SecureClassLoader.defineClass(SecureClassLoader.java:124)
         at java.net.URLClassLoader.defineClass(URLClassLoader.java:260)
         at java.net.URLClassLoader.access$000(URLClassLoader.java:56)
         at java.net.URLClassLoader$1.run(URLClassLoader.java:195)
         at java.security.AccessController.doPrivileged(Native Method)
         at java.net.URLClassLoader.findClass(URLClassLoader.java:188)
         at java.lang.ClassLoader.loadClass(ClassLoader.java:316)
         at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:
288)
         at java.lang.ClassLoader.loadClass(ClassLoader.java:251)
         at java.lang.ClassLoader.loadClassInternal(ClassLoader.java:
374)
         at java.lang.Class.forName0(Native Method)
         at java.lang.Class.forName(Class.java:247)
         at clojure.lang.RT.loadClassForName(RT.java:1512)
         at clojure.lang.RT.load(RT.java:394)
         at clojure.lang.RT.load(RT.java:374)
         at clojure.core$load__4911$fn__4913.invoke(core.clj:3623)
         at clojure.core$load__4911.doInvoke(core.clj:3622)
         at clojure.lang.RestFn.invoke(RestFn.java:413)
         at clojure.core$load_one__4863.invoke(core.clj:3467)
         at clojure.core$compile__4918$fn__4920.invoke(core.clj:3633)
         at clojure.core$compile__4918.invoke(core.clj:3632)
         at clojure.lang.Var.invoke(Var.java:336)
         at clojure.lang.Compile.main(Compile.java:56)


 Comments   
Comment by Assembla Importer [ 24/Aug/10 6:45 AM ]

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

Comment by Assembla Importer [ 24/Aug/10 6:45 AM ]

richhickey said: Updating tickets (#8, #19, #30, #31, #126, #17, #42, #47, #50, #61, #64, #69, #71, #77, #79, #84, #87, #89, #96, #99, #103, #107, #112, #113, #114, #115, #118, #119, #121, #122, #124)





[CLJ-1136] Type hinting for array classes does not work in binding forms Created: 20/Dec/12  Updated: 03/Sep/13

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

Type: Defect Priority: Major
Reporter: Luke VanderHart Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop, typehints
Environment:

replicated on OpenJDK 7u9 on Ubuntu 12.04, and Hotspot 1.6.0_37 on OSX Lion



 Description   

Type hints don't work as expected in binding forms.

The following form results in a reflection warning:

(let [^{:tag (Class/forName "[Ljava.lang.Object;")} a (make-array Object 2)]
(aget a 0))

However, hinting does appear to work correctly on vars:

(def ^{:tag (Class/forName "[Ljava.lang.Object;")} a (make-array Object 2))
(aget a 0) ;; no reflection warning



 Comments   
Comment by Ghadi Shayban [ 20/Dec/12 10:51 PM ]

It's a little more insidious than type hinting: the compiler doesn't evaluate metadata in the binding vec.

This doesn't throw the necessary exception...

(let [^{:foo (Class/forName "not real")} bar 42]
bar)

neither this...

(let [^{gyorgy ligeti} a 42]
a)

Gyorgy Ligeti never resolves.

These two equivalent examples don't reflect:
(let [^objects a (make-array Object 2)]
(aget a 0))

(let [a ^objects (make-array Object 2)]
(aget a 0))

Comment by Ghadi Shayban [ 21/Dec/12 11:09 AM ]

On only the left-hand side of a local binding, metadata on a symbol is not analyzed or evaluated.





[CLJ-1001] Proxy cannot call proper super-class method Created: 23/May/12  Updated: 03/Sep/13

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

Type: Defect Priority: Major
Reporter: Guanpeng Xu Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop
Environment:

Linux herberteuler 3.2.0-2-amd64 #1 SMP Sat May 12 23:08:28 UTC 2012 x86_64 GNU/Linux


Attachments: File proxy-bug.clj    

 Description   

Attached is a program that reproduces this issue. We have a proxy, `p', which sub-classes java.io.InputStream. There are three methods named `read' in java.io.InputStream: abstract int read(); int read(byte[] b); and int read(byte[] b, int off, int len); see http://docs.oracle.com/javase/6/docs/api/java/io/InputStream.html. In the definition of proxy `p', we implement the abstract variant of method `read', making `p' a concrete instance of java.io.InputStream.

The first invocation, (. p read), returns -1, which is expected.

The second invocation, (. p (read b 0 n)), should call int read(byte[] b, int off, int len); in java.io.InputStream. But these are actual behavior:

$ clojure1.2 ~/tmp/proxy-bug.clj
Exception in thread "main" java.lang.IllegalArgumentException: Wrong number of args (4) passed to: user$eval1$fn (proxy-bug.clj:0)
at clojure.lang.Compiler.eval(Compiler.java:5441)
at clojure.lang.Compiler.load(Compiler.java:5858)
at clojure.lang.Compiler.loadFile(Compiler.java:5821)
at clojure.main$load_script.invoke(main.clj:221)
at clojure.main$script_opt.invoke(main.clj:273)
at clojure.main$main.doInvoke(main.clj:354)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at clojure.lang.Var.invoke(Var.java:365)
at clojure.lang.AFn.applyToHelper(AFn.java:161)
at clojure.lang.Var.applyTo(Var.java:482)
at clojure.main.main(main.java:37)
Caused by: java.lang.IllegalArgumentException: Wrong number of args (4) passed to: user$eval1$fn
at clojure.lang.AFn.throwArity(AFn.java:437)
at clojure.lang.AFn.invoke(AFn.java:51)
at user.proxy$java.io.InputStream$0.read(Unknown Source)
at user$eval1.invoke(proxy-bug.clj:9)
at clojure.lang.Compiler.eval(Compiler.java:5425)
... 10 more

$ clojure1.2 ~/tmp/proxy-bug.clj
Exception in thread "main" java.lang.IllegalArgumentException: Wrong number of args (4) passed to: user$eval1$fn (proxy-bug.clj:0)
at clojure.lang.Compiler.eval(Compiler.java:5441)
at clojure.lang.Compiler.load(Compiler.java:5858)
at clojure.lang.Compiler.loadFile(Compiler.java:5821)
at clojure.main$load_script.invoke(main.clj:221)
at clojure.main$script_opt.invoke(main.clj:273)
at clojure.main$main.doInvoke(main.clj:354)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at clojure.lang.Var.invoke(Var.java:365)
at clojure.lang.AFn.applyToHelper(AFn.java:161)
at clojure.lang.Var.applyTo(Var.java:482)
at clojure.main.main(main.java:37)
Caused by: java.lang.IllegalArgumentException: Wrong number of args (4) passed to: user$eval1$fn
at clojure.lang.AFn.throwArity(AFn.java:437)
at clojure.lang.AFn.invoke(AFn.java:51)
at user.proxy$java.io.InputStream$0.read(Unknown Source)
at user$eval1.invoke(proxy-bug.clj:9)
at clojure.lang.Compiler.eval(Compiler.java:5425)
... 10 more



 Comments   
Comment by Guanpeng Xu [ 23/May/12 10:24 PM ]

The second behavior should be in Clojure 1.3:

$ clojure1.3 ~/tmp/proxy-bug.clj
Exception in thread "main" clojure.lang.ArityException: Wrong number of args (4) passed to: user$eval1$fn
at clojure.lang.AFn.throwArity(AFn.java:437)
at clojure.lang.AFn.invoke(AFn.java:51)
at user.proxy$java.io.InputStream$0.read(Unknown Source)
at user$eval1.invoke(proxy-bug.clj:9)
at clojure.lang.Compiler.eval(Compiler.java:6468)
at clojure.lang.Compiler.load(Compiler.java:6905)
at clojure.lang.Compiler.loadFile(Compiler.java:6866)
at clojure.main$load_script.invoke(main.clj:282)
at clojure.main$script_opt.invoke(main.clj:342)
at clojure.main$main.doInvoke(main.clj:426)
at clojure.lang.RestFn.invoke(RestFn.java:408)
at clojure.lang.Var.invoke(Var.java:401)
at clojure.lang.AFn.applyToHelper(AFn.java:161)
at clojure.lang.Var.applyTo(Var.java:518)
at clojure.main.main(main.java:37)

Sorry for the inconvenience.

Comment by Russell Mull [ 01/Sep/13 3:12 AM ]

Verified with Clojure 1.5.1:

Stack Trace
clojure.lang.ArityException: Wrong number of args (4) passed to: user$eval147$fn
                                      AFn.java:437 clojure.lang.AFn.throwArity
                                       AFn.java:51 clojure.lang.AFn.invoke
                                  (Unknown Source) user.proxy/java.io.InputStream[fn]
                                  NO_SOURCE_FILE:9 user/eval147
                                Compiler.java:6619 clojure.lang.Compiler.eval
                                Compiler.java:6582 clojure.lang.Compiler.eval
                                     core.clj:2852 clojure.core/eval
                                      main.clj:259 clojure.main/repl[fn]
                                      main.clj:259 clojure.main/repl[fn]
                                      main.clj:277 clojure.main/repl[fn]
                                      main.clj:277 clojure.main/repl
                                  RestFn.java:1096 clojure.lang.RestFn.invoke
                         interruptible_eval.clj:56 clojure.tools.nrepl.middleware.interruptible-eval/evaluate[fn]
                                      AFn.java:159 clojure.lang.AFn.applyToHelper
                                      AFn.java:151 clojure.lang.AFn.applyTo
                                      core.clj:617 clojure.core/apply
                                     core.clj:1788 clojure.core/with-bindings*
                                   RestFn.java:425 clojure.lang.RestFn.invoke
                         interruptible_eval.clj:41 clojure.tools.nrepl.middleware.interruptible-eval/evaluate
                        interruptible_eval.clj:171 clojure.tools.nrepl.middleware.interruptible-eval/interruptible-eval[fn]
                                     core.clj:2330 clojure.core/comp[fn]
                        interruptible_eval.clj:138 clojure.tools.nrepl.middleware.interruptible-eval/run-next[fn]
                                       AFn.java:24 clojure.lang.AFn.run
                      ThreadPoolExecutor.java:1110 java.util.concurrent.ThreadPoolExecutor.runWorker
                       ThreadPoolExecutor.java:603 java.util.concurrent.ThreadPoolExecutor$Worker.run
                                   Thread.java:722 java.lang.Thread.run




[CLJ-1046] Drop-while as a reducer Created: 18/Aug/12  Updated: 03/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reducers

Attachments: Text File drop-while-reducer.patch    
Patch: Code and Test

 Description   

Implement drop-while as a reducer. Follows the same atom-based strategy as drop and take.

Does not depend on any of my other reducer patches, but there will probably be some minor merge conflicts unless it is merged after CLJ-1045, and before CLJ-992 and CLJ-993.






[CLJ-1022] gen-class destroys method annotations Created: 03/Jul/12  Updated: 03/Sep/13

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

Type: Defect Priority: Major
Reporter: Maris Orbidans Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop


 Description   

When extending a class gen-class doesn't preserve method annotations.

If class com.bar.Foo has annotated methods then in MyClass all annotations are gone.

(gen-class
:name com.my.MyClass
:extends com.bar.Foo
:implements [com.google.common.base.Supplier]
:prefix demo-
:post-init post-init)

(defn demo-post-init [this]
(info "initialized")
(swank.swank/start-server :port 68478))

(defn demo-get [_]
(get-msg))

Class<?> aClass = Class.forName("com.my.MyClass");
Method[] methods = aClass.getMethods();

for (Method m : methods) {
Annotation[] annotations = m.getAnnotations();
System.out.println(m.getName()+" "+annotations.length);
for (Annotation a : annotations) { System.out.println(a.annotationType().getClass().getName()); }
}






[CLJ-994] repeat reducer Created: 11/May/12  Updated: 03/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Jason Jackson Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: reducers

Attachments: Text File 0001-repeat-for-clojure.core.reducers.patch    
Patch: Code and Test

 Description   

i'm working on clojure.core/repeat reducer.



 Comments   
Comment by Andy Fingerhut [ 17/May/12 6:18 PM ]

Jason, have you tried to build this using JDK 1.6.0? I've tried on Mac OS X 10.6.8 + Oracle/Apple JDK 1.6.0 and Ubuntu 11.10 + IBM JDK 1.6.0, and on both it compiles, but during the tests fails with a ClassNotFoundException for class jsr166y.ForkJoinTask.

It builds and tests cleanly on Ubuntu 11.10 + Oracle JDK 1.7.0 for me.

Comment by Jason Jackson [ 17/May/12 6:41 PM ]

That's an issue that applies to all of core.reducers. Alan Malloy experienced it as well. I tried fixing it, but eventually just upgraded to JDK 1.7. I don't understand why it's happening.

Comment by Jason Jackson [ 19/May/12 2:55 PM ]

This issue is isolated to mvn test afaik.

When I include clojure inside a leiningen project, and add jsr166y.jar to lib directory, core.reducers works fine with java 1.6.

Comment by Andy Fingerhut [ 20/May/12 3:00 AM ]

Jason, you say it applies to all of core.reducers in your May 17, 2012 comment. I don't understand. Without your patch applied, I can run "./antsetup.sh ; ant" in a freshly-pulled Clojure git repo on either of the JDK 1.6.0 versions mentioned in my earlier comment, and do not get any errors during the tests. Are you saying perhaps that core.reducers currently has no tests that exercise the problem now, but your patch adds such tests that fail, even with no other changes to the code?

Comment by Jason Jackson [ 20/May/12 11:55 AM ]

Yah that's right. Now that you mention it, my patch is the first unit test to call r/fold (the existing tests do non-parallel reductions).

Comment by Andy Fingerhut [ 08/Jun/12 7:11 PM ]

With Stuart Halloway's commit to Clojure master on June 8, 2012 titled "let reducers tests work under ant", patch 0001-repeat-for-clojure.core.reducers.patch dated May 11, 2012 now runs correctly even the new unit tests requiring class jsr166y.ForkJoinTask with Oracle/Apple JDK 1.6 and Linux IBM JDK 1.6.

Comment by Jason Jackson [ 14/Aug/12 1:17 AM ]

I'm on the contributors list. Is this patch still needed?
sorry for long long delay.

Comment by Jason Jackson [ 14/Sep/12 2:37 PM ]

This patch should wait until http://dev.clojure.org/jira/browse/CLJ-993 is committed. I think there's a some shared code.





[CLJ-993] `range` reducer Created: 10/May/12  Updated: 03/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: reducers

Attachments: Text File 0001-CLJ-993-implement-range-and-iterate-as-reducers.patch     Text File 0001-CLJ-993-implement-range-and-iterate-as-reducers.patch     Text File 0001-CLJ-993-implement-range-as-a-reducer.patch     Text File 0002-Make-iterate-and-range-Seqable.patch     Text File 0003-Implement-fold-for-Range-objects.patch     Text File just-iseq.patch     Text File range-reducer.patch    
Patch: Code and Test

 Description   

Rich mentioned in IRC today he'd welcome a reducer implementation of clojure.core/range. Now that I've figured out how to do iterate, I figure I'll knock out range as well by the end of the night. Just opening the issue early to announce my intentions to anyone else interested in doing it.



 Comments   
Comment by Alan Malloy [ 10/May/12 10:45 PM ]

Implemented range. A separate commit is attached, making iterate and range also Seqable, since I'm not sure if that's desired. Apply it or not, as you prefer.

Comment by Jason Jackson [ 11/May/12 11:20 AM ]

Range should be foldable

Comment by Alan Malloy [ 11/May/12 12:53 PM ]

Yep, so it should. Time for me to dig into the folding implementations!

Comment by Alan Malloy [ 11/May/12 2:42 PM ]

Should I fold (har har) all of these commits into one? I don't know what is preferred on JIRA, and I also don't know whether range/iterate should be seqable or if I should just drop the second commit.

Comment by Rich Hickey [ 11/May/12 3:21 PM ]

Yes, please merge these together, it's hard to see otherwise (I can barely read diffs as is . range and iterate shouldn't be novel in reducers, but just enhanced return values of core fns. The enhancement (e.g. protocol extensions) can come by requiring reducers since it can't be leveraged without it. Also, I'm not sure how I feel about an allocating protocol for 'splittable' - I've avoided it thus far.

Comment by Alan Malloy [ 11/May/12 3:30 PM ]

So you want clojure.core/range to return some object (a Range), which implements Counted and Seqable (but isn't just a lazy-seq), and then inside of clojure.core.reducers I extend CollReduce and CollFold to that type? Okay, I can do that.

I don't quite follow what you mean by an allocating protocol. I see your point that my fold-by-halves which takes a function in is analogous to a protocol with a single function, but it doesn't allocate anything more than foldvec already does - I just pulled that logic out so that the fork/join fiddly work doesn't need to be repeated in everything foldable. Do you have an alternative recommendation, or is it just something that makes you uneasy and you're still thinking about?

Comment by Rich Hickey [ 11/May/12 3:52 PM ]

While vector-fold allocs subvecs, the halving-fn must return a new vector, for all implementations. It's ok, I don't think it's likely to dominate (since fj needs new closures anyway). Please proceed, but keep range and iterate in core. They are sources, not transformers, and only transformers (which must be different from their seq-based counterparts) must reside in reducers. Thanks!

Comment by Stuart Sierra [ 11/May/12 5:01 PM ]

One big patch file is preferred, although that file may contain multiple commits if that makes the intent clearer.

When adding a patch, update the description of the ticket to indicate which file is the most recent. Leave old patch files around for historical reference.

Comment by Alan Malloy [ 11/May/12 9:00 PM ]

It's looking harder than I expected to move iterate and range into core.clj. My plan was to just have them implement Seqable, which is easy enough, but currently they are actually instances of ISeq, because they inherit from LazySeq. A bunch of code all over the place (eg, to print them in the repl) depends on them being ISeq, so I can't just ignore it. To implement all of these methods (around thirty) would take a large amount of code, which can't easily be shared between Iteration, Range, and any future reducible sources that are added to core.clj.

I could write a macro like (defseq Range [start end step] Counted (count [this] ...) ...) which takes normal deftype args and also adds in implementations for ISeq, Collection, and so forth in terms of (.seq this), which will be a LazySeq. However, this seems like a somewhat awkward approach that I would be a little embarrassed to clutter up core.clj with. If anyone has a better alternative I will be pleased to hear it. In the mean time, I will go ahead with this macro implementation, in case it turns out to be the best choice.

Comment by Alan Malloy [ 11/May/12 11:52 PM ]

– This patch subsumes all previous patches to this issue and to CLJ-992

In order to create an object which is both a lazy sequence and a
reducible source, I needed to add a macro named defseq to core_deftype.
It is basically a reimplementation of clojure.lang.LazySeq as a clojure
macro, so that I can "mix in" lazy-sequence functions into a new class
with whatever methods are needed for reducing and folding.

If we wanted, we could use this macro to implement lazy-seq in clojure instead of in java, but that's unrelated so I didn't do that in this patch.

As noted in a previous comment, defseq may not be the right approach, but this works until something better is suggested.

Comment by Alan Malloy [ 11/May/12 11:58 PM ]

I accidentally included an implementation of drop-while in this patch, which I was playing around with to make sure I understood how this all works. I guess I'll leave it in for the moment, since it works and is useful, but I can remove it, or move it to a new JIRA ticket, if it's not wanted at this time.

Comment by Rich Hickey [ 12/May/12 10:52 AM ]

Ok, I think this patch is officially off the rails. There must be a better way. Let's start with: touching core/deftype and reimplementing lazy-seq as a macro are off the table. The return value of range doesn't have to be a LazySeq, it has to be a lazy seq, .e.g. implement ISeq (7 methods, not 30) which it can do by farming out to its existing impl. It can also implement some new interface for use by the reducer logic. There is also still clojure.lang.Range still there, which is another approach. Please take an extremely conservative approach in these things.

Comment by Alan Malloy [ 12/May/12 5:53 PM ]

Okay, thanks for the feedback - I'm glad I went into that last patch knowing it was probably wrong . I thought I would need to implement the java collection interfaces that LazySeq does, eg java.util.List, in order to avoid breaking interop functions like (defn range-list [n] (ArrayList. (range n))). If it's sufficient to implement ISeq (and thus IPersistentCollection), then that's pretty manageable.

It's still an unpleasant chunk of boilerplate for each new source, though; would you welcome a macro like defseq if I didn't put it in core_deftype? If so, it seems like it might as well implement the interop interfaces; if not, I can skip them and implement the 7 (isn't it more like 9?) methods in ISeq, IPersistentCollection, and Seqable for each new source type.

Thanks for pointing out clojure.lang.Range to me - I didn't realize we had it there. Of course with implementation inheritance it would be easy to make Range, Iteration, etc inherit from LazySeq and just extend protocols from them. But that means moving functionality out of clojure and into java, which I didn't think we'd want to do.

I'll put together a patch that just implements ISeq by hand for both of these new types, and attach it probably later today.

Comment by Alan Malloy [ 12/May/12 7:49 PM ]

So I've written a patch that implements ISeq, but not the java Collections interfaces, and it mostly works but there are definitely assumptions in some parts of clojure.core and clojure.lang that assume seqs are Collections. The most obvious to me (ie, it shows up when running mvn test) is RT/toArray - it tests for Collection, but never for ISeq, implying that it's not willing to handle an ISeq that is not also a collection. Functions which rely on toArray (eg to-array and vec) now fail.

This patch subsumes all previous patches on this issue, but is not suitable for application because it leaves some failing tests behind - it is intended only for intermediate feedback.

Comment by Rich Hickey [ 13/May/12 8:50 AM ]

It would be a great help if, time permitting, you could please write up the issues, challenges and options you've discovered somewhere on the dev wiki (even a simple table would be fantastic). I realize this has been a challenging task, and at this point perhaps we should opt for the more modest reducers/range and reducers/iterate and leave the two worlds separate. I'd like at some point to unify range, as there are many extant ranges it would be nice to be able to fold, as we can extant vectors.

Comment by Jason Jackson [ 13/May/12 9:24 AM ]

Should r/range return something Seqable and Counted?

If so, I'll do the same for r/repeat.

Comment by Alan Malloy [ 13/May/12 1:59 PM ]

I've sketched out a description of the issues and options. I'm not very familiar with the dev wiki and couldn't figure out where was the right place to put this. "release.next" seems to still be about 1.4 issues, and I don't know if it's "appropriate" to create a whole new category for this. It's available as a gist until a better home can be found for it.

Comment by Alan Malloy [ 23/May/12 7:54 PM ]

Here's a single patch summing up the state Rich suggested "rolling back" to: separate r/range and r/iterate functions. I haven't heard any feedback since doing the writeup Rich asked for, so am not making any further progress at the moment; if something other than this patch is desired just let me know.

Comment by Rich Hickey [ 14/Aug/12 2:07 PM ]

I prefer not to see the use of extend like this for new types. Perhaps this code is too DRY? Also, it does a lot in one patch which makes it hard to parse and accept. This adds Range, switches impl of vector folds etc. Can it be broken up into separate tickets that do each step that builds on the previous, e.g. one ticket could be: capture vector fold impl for reuse by similar things.

Comment by Alan Malloy [ 18/Aug/12 6:19 PM ]

Okay, I should be able to split it up over the weekend. I'll also see about converting fold-by-halves into a function that is used by Range/Vector, rather than a function that gets extended onto them.

Comment by Alan Malloy [ 18/Aug/12 7:18 PM ]

As requested, I have split up the large patch on this issue into four smaller tickets. The other three are: CLJ-1045, CLJ-1046, and CLJ-992.

CLJ-1045 contains the implementation of fold-by-halves, and as such this patch cannot be applied until CLJ-1045 is accepted. This ticket does not depend on the other two, but there will be minor merge conflicts if this is merged before them.





[CLJ-986] Adds an exit function to exit clojure process Created: 06/May/12  Updated: 03/Sep/13

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

Type: Enhancement Priority: Major
Reporter: dennis zhuang Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: None


 Description   

There is no standard function to exit the clojure process.
In java implementation,we use (System/exit 0),but in other implementations(CLR), i have to use another function.

Why not add a standard function in clojure.core?
For example:

(defn exit
([] (exit 0)
([status] (System/exit status)))

I think it's useful for us.






[CLJ-969] Symbol/keyword implements IFn for lookup but a non-collection argument produces non-intuitive results Created: 09/Apr/12  Updated: 03/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Sean Corfield Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs


 Description   

('+ 1 2) ;; return 2 because it is treated as (get 1 '+ 2)

Whilst this is "consistent" once you know the lookup behavior, it's confusing for Clojure newbies and it seems to be a non-useful behavior.

Proposal: modify Keyword.invoke() and Symbol.invoke() to restrict first Object argument to instanceof ILookup, Map or IPersistentSet (or null) so that the "not found" behavior doesn't produce non-intuitive behavior.






[CLJ-968] ns emitting gen-class before imports results in imported annotations being discarded. Created: 09/Apr/12  Updated: 03/Sep/13

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

Type: Defect Priority: Major
Reporter: Charles Duffy Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop


 Description   

The following discards the imported annotations:

(ns com.example.BaseXModuleTest
  (:import (org.basex.query QueryModule QueryModule$Deterministic))
  (:gen-class
     :extends org.basex.query.QueryModule
     :methods [
       [^{QueryModule$Deterministic {}}
        addOne [int] int]]))

However, when moving the gen-class call out of the ns declaration, the annotation is correctly applied:

(ns com.example.BaseXModuleTest
  (:import (org.basex.query QueryModule QueryModule$Deterministic)))

(gen-class
  :extends org.basex.query.QueryModule
  :name com.example.BaseXModuleTest
  :methods [
    [^{QueryModule$Deterministic {}}
     addOne [int] int]])

It appears that imported names are not yet in-scope when gen-class is run from a ns declaration.






[CLJ-919] cannot create anonymous primitive functions Created: 27/Jan/12  Updated: 03/Sep/13

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

Type: Defect Priority: Major
Reporter: Ben Mabey Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: typehints


 Description   

Primitive functions only work (e.g. return primitive types) when defined with `defn`. An equivalent function created with `fn` does not behave the same way as when created with `defn`. For example:

(definterface IPrimitiveTester
(getType [^int x])
(getType [^long x])
(getType [^float x])
(getType [^double x])
(getType [^Object x]))

(deftype PrimitiveTester []
IPrimitiveTester
(getType [this ^int x] :int)
(getType [this ^long x] :long)
(getType [this ^float x] :float)
(getType [this ^double x] :double)
(getType [this ^Object x] :object))

(defmacro pt [x]
`(.getType (PrimitiveTester.) ~x))

(defn with-defn ^double [^double x]
(+ x 0.5))

(pt (with-defn 1.0)) ; => :double

(let [a (fn ^double [^double x] (+ x 0.5))]
(pt (a 0.1))) ; => :object

Please see the discussion on the mailing list for more details and thoughts on what is happening:
http://groups.google.com/group/clojure/browse_thread/thread/d83c8643a7c7d595?hl=en






[CLJ-911] 'proxy' prevents overriding Object.finalize (and doesn't document it) Created: 16/Jan/12  Updated: 03/Sep/13

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

Type: Defect Priority: Major
Reporter: Norman Gray Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop
Environment:

OS X, Java 1.6.0?



 Description   

It appears to be impossible to override Object.finalize() using proxy. If the method is defined using proxy, then it cannot be called straightforwardly (see below), and it is not called as a finalizer during normal program execution (not demonstrated below).

See extensive discussion at: https://groups.google.com/group/clojure/browse_thread/thread/a1e2fca45af6c1af

user=> (def m (proxy [java.util.HashMap] []
(finalize []
;(proxy-super finalize)
(prn "finalizing..."))
(hashCode []
99)))
#'user/m
user=> (.hashCode m)
99
user=> (.finalize m)
IllegalArgumentException No matching field found: finalize for class user.proxy$java.util.HashMap$0 clojure.lang.Reflector.getInstanceField (Reflector.java:289)

There is at least one of two bugs here (thanks to Cedric Greevey for summarising this way):

  • If the inability to override finalize() is unintentional, that's a bug.
  • If it's intentional for some reason, then (a) that's not documented, and (b) the failure is silent, in the sense that an explicit call produces an apparently completely unrelated error (above), and the failure to call the method during object finalization is completely silent.





[CLJ-903] extend-protocol does not allow classnames as a String Created: 30/Dec/11  Updated: 03/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Meikel Brandmeyer Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: interop


 Description   

In various places Clojure accepts classnames as String, eg. in gen-class or type hints. However it does not in extend-protocol. This does not allow simple specification of array types.

See also here: http://groups.google.com/group/clojure/browse_thread/thread/722a0c09d02bb0ac






[CLJ-790] Primitive type hints on function names should print error message Created: 10/May/11  Updated: 03/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Alan Dipert Assignee: Alan Dipert
Resolution: Unresolved Votes: 0
Labels: errormsgs


 Description   

Functions returning primitives are hinted with metadata on the argument list, not on the function name. Using a primitive type hint on a function name should print an error message.

Currently, misplaced primitive hints are read without error.






[CLJ-434] Additional copy methods for URLs in clojure.java.io Created: 10/Sep/10  Updated: 03/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Anonymous Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: io


 Description   

The copy method in clojure.java.io doesn't handle java.net.URL as input.
The necessary methods can be found in the mailing list post:
[[url:http://groups.google.com/group/clojure/browse_thread/thread/24a105b12466a8e8]]



 Comments   
Comment by Assembla Importer [ 10/Sep/10 7:32 AM ]

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





[CLJ-400] A faster flatten Created: 13/Jul/10  Updated: 03/Sep/13

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

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


 Description   

As discussed in this thread, I am submitting a more performant version of flatten for review. It has the same semantics as the current core/flatten. I have also updated the doc string to say that "(flatten nil) returns the empty list", because that's what the current version of core/flatten does as well.

I haven't mailed in a CA yet, but I will tomorrow morning.

Edit: Of course I'd mess my first ticket up . I am not immediately seeing an option to edit this to add the "patch" tag or add the "ready to test" action. Sorry folks



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

Converted from http://www.assembla.com/spaces/clojure/tickets/400
Attachments:
flatten-enhancement.diff - https://www.assembla.com/spaces/clojure/documents/c5chtAJQir353NeJe5cbLr/download/c5chtAJQir353NeJe5cbLr





[CLJ-405] better error messages for bad defrecord calls Created: 20/Jul/10  Updated: 03/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Anonymous Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: defrecord, errormsgs


 Description   

defrecord could tell you if, e.g., you didn't specify an interface before leaping into method bodies. See http://groups.google.com/group/clojure/browse_thread/thread/f52f90954edd8b09



 Comments   
Comment by Assembla Importer [ 24/Aug/10 12:28 AM ]

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

Comment by Assembla Importer [ 24/Aug/10 12:28 AM ]

stu said: This could be fixed with an assert-valid-defrecord call in core_deftype, similar to assert-valid-fdecl in core.clj. Such a function would also be a place to hang other defrecord error messages.





[CLJ-277] Making clojure.xml/emit a little friendler to xml consumers Created: 03/Mar/10  Updated: 03/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Anonymous Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: xml


 Description   

Currently, clojure.xml/emit breaks the eBay api, because emit adds whitespace before and after :contents. This trivial patch fixes it for me:
http://github.com/tjg/clojure/commit/bbff079d26e627c655b847319a58d76b8b3cec7c

(Dunno whether there's a good reason emit works that way, or if I'm missing something obvious.)

I realize that emit's behavior conforms to the XML spec and it's probably eBay at fault here. But I can nevertheless see this whitespace causing problems.



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

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

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

bpsm said: I've attached a patch to #410, which also fixes this issue. (In fact, it turns out that it's the same patch tlj previously attached here.)

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

stu said: Duplicated association with ticket #410 was added





[CLJ-252] Support typed non-primitive fields in deftype Created: 29/Jan/10  Updated: 03/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Anonymous Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: deftype


 Description   

Right now hints are accepted but not used as field type.



 Comments   
Comment by Assembla Importer [ 24/Aug/10 6:07 AM ]

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





[CLJ-233] better error reporting of nonexistent var Created: 31/Dec/09  Updated: 03/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Anonymous Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: errormsgs


 Description   

simple improvement to error message when referencing a var that doesn't exist.



 Comments   
Comment by Assembla Importer [ 29/Sep/10 5:29 AM ]

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

Comment by Assembla Importer [ 29/Sep/10 5:29 AM ]

chouser@n01se.net said: Stuart, I don't see a patch attached.





[CLJ-190] enhance with-open to be extensible with a new close multimethod Created: 13/Sep/09  Updated: 03/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Anonymous Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: io


 Description   

Discussion: http://groups.google.com/group/clojure/browse_thread/thread/8e4e56f6fc65cc8e/618a893a5b2a5410

Currently, with-open calls .close when it's finished. I'd like it to have a (defmulti close type) so it's behavior is extensible. A standard method could be defined for java.io.Closeable and a :default method with no type hint. I've come across a few cases where some external library defines what is essentially a close method but names it shutdown or disable, etc., and adding my own "defmethod close" would be much easier than rewriting with-open. This would also allow people to eliminate reflection for classes like sql Connection that were created before Closeable.



 Comments   
Comment by Assembla Importer [ 24/Aug/10 4:30 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/190
Attachments:
clojure-190-with-open.patch - https://www.assembla.com/spaces/clojure/documents/ca27R6Ojur3PQ0eJe5afGb/download/ca27R6Ojur3PQ0eJe5afGb

Comment by Assembla Importer [ 24/Aug/10 4:30 AM ]

mikehinchey said: [file:ca27R6Ojur3PQ0eJe5afGb]: fix adds close method and tests

Comment by Assembla Importer [ 24/Aug/10 4:30 AM ]

mikehinchey said: Note, I only defined methods for :default (reflection of .close) and Closeable, not sql or the numerous other classes in java that should be Closeable but are not. Maybe clojure.contrib.sql and other such libraries should define related close methods.

Comment by Assembla Importer [ 24/Aug/10 4:30 AM ]

richhickey said: I want to hold off on this until scopes are in

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

Probably better implemented using a protocol. See http://dev.clojure.org/jira/browse/CLJ-308





[CLJ-163] Enhance = and == docs Created: 30/Jul/09  Updated: 03/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Anonymous Assignee: Laurent Petit
Resolution: Unresolved Votes: 0
Labels: docstring


 Description   

Enhance = and == docs as far as numbers handling is concerned (make them self referenced, make clear what == offers beyond = -except that it will only work for numbers)



 Comments   
Comment by Assembla Importer [ 24/Aug/10 1:04 PM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/163
Attachments:
fixbug163.diff - https://www.assembla.com/spaces/clojure/documents/bH0XMCFjur3PLMeJe5aVNr/download/bH0XMCFjur3PLMeJe5aVNr

Comment by Assembla Importer [ 24/Aug/10 1:04 PM ]

laurentpetit said: [file:bH0XMCFjur3PLMeJe5aVNr]

Comment by Assembla Importer [ 24/Aug/10 1:04 PM ]

richhickey said: I don't want to recommend, in = doc, that people should prefer == for any case. People should always prefer =. If there is a perf, difference we can make that go away. Then the only difference with == is that it will fail on non-numbers, and that should be the only reason to choose it.

Comment by Assembla Importer [ 24/Aug/10 1:04 PM ]

richhickey said: Updating tickets (#94, #96, #104, #119, #163)

Comment by Assembla Importer [ 24/Aug/10 1:04 PM ]

laurentpetit said: Richn, by "will fail on non-numbers", do you mean "should throw an exception" (and thus the patch must change the code), or just as it works today :

(== :a :a)
false

?

Comment by Assembla Importer [ 24/Aug/10 1:04 PM ]

richhickey said: I've fixed the code so == on non-numbers throws





[CLJ-150] Doc for array-map should mention its characteristics/caveats Created: 10/Jul/09  Updated: 03/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Anonymous Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: docstring


 Description   

Doc for array-map should mention its characteristics: preserves order of keys, linear O search so appropriate only for small maps, operations on array-maps return hash-maps.



 Comments   
Comment by Assembla Importer [ 24/Aug/10 4:54 AM ]

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





[CLJ-148] Poor reporting of symbol conflicts when using (ns) Created: 10/Jul/09  Updated: 03/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Anonymous Assignee: Unassigned
Resolution: Unresolved Votes: 1
Labels: errormsgs


 Description   

I have a module that includes pprint and my own utils.

When com.howard.lewisship.cascade.dom/write was changed from private to public I get the following error:

java.lang.IllegalStateException: write already refers to: #'clojure.contrib.pprint/write in namespace: com.howardlewisship.cascade.test-views (test_views.clj:0)

(ns com.howardlewisship.cascade.test-views ; line 15
(:use
(clojure.contrib test-is pprint duck-streams)
(app1 views fragments)
(com.howardlewisship.cascade config dom view-manager)
com.howardlewisship.cascade.internal.utils))

That line number is wrong but better yet, identifying the true conflict (com.howard.lewisship.cascade.dom/write) would be even more important.



 Comments   
Comment by Assembla Importer [ 24/Aug/10 3:54 AM ]

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

Comment by Assembla Importer [ 24/Aug/10 3:54 AM ]

scgilardi said: It's saying that the symbol com.howardlewisship.cascade.test-views/write already resolves to #���clojure.contrib.pprint/write, so you can't def a new write in com.howardlewisship.cascade.test-views.

What do you propose for an alternate wording of the error message here?

Comment by Jeff Rose [ 12/May/11 9:49 AM ]

I think the issue is that only one side of the conflict is reported in the error, so if you get this kind of error in the middle of a large project it can be hard to figure out which namespace is conflicting. Take a toy example:

user=> (ns foo)
foo=> (def foobar 42)
foo=> (ns bar)
bar=> (def foobar 0)
bar=> (ns problem)
problem=> (refer 'foo)
problem=> (refer 'bar)
java.lang.IllegalStateException: foobar already refers to: #'foo/foobar in namespace: problem (NO_SOURCE_FILE:0)

In this case it would be best if the error said something like:

"Conflict referring to #'bar/foobar in #<Namespace problem> because foobar already refers to: #'foo/foobar."

This way the error message clearly identifies the location of the conflict, and the locations of the two conflicting vars.

Hopefully this helps clarify. I think I see where to fix it in warnOrFailOnReplace on line 88 of src/jvm/clojure/lang/Namespace.java, and this reminds me I need to send in a CA so I can pitch in next time...

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

It looks like the true conflict is in test-views, not in dom. A small example of the line number breakage showing the problem on master (1.3) would be very helpful.





[CLJ-140] Single :tag for type hints conflates value's type with type of return value from an invoke Created: 01/Jul/09  Updated: 03/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Anonymous Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: typehints


 Description   

The value of a Var can be operated on directly, or, if it is a fn, it can be invoked and the resulting value operated on. :tag metadata on a Var is used to provide a type hint to the compiler to avoid reflection. Having a single metadata key for this two distinct uses makes it possible (even easy, if unlikely) to create a situation where type-hinting the value causes a ClassCastException on an operation on the invocation return value, or the reverse. The only obvious solution is two use different keys for the two uses.



 Comments   
Comment by Assembla Importer [ 24/Aug/10 3:51 AM ]

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





[CLJ-129] Add documentation to sorted-set-by detailing how the provided comparator may change set membership semantics Created: 18/Jun/09  Updated: 03/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Anonymous Assignee: Chas Emerick
Resolution: Unresolved Votes: 0
Labels: docstring


 Description   

To start, let's look at some simple default sorted-set behaviour (which uses PersistentHashMap via PersistentHashSet, and therefore uses equality/hashCode to determine identity):

user=> (sorted-set [1 2] [-5 10] [1 5])
#{[-5 10] [1 2] [1 5]}
</code></pre>

sorted-set-by uses PersistentTreeMap via PersistentTreeSet though, which implies that the comparator provided to sorted-set-by will be used to determine identity, and therefore member in the set.  This can lead to (IMO) non-intuitive behaviour:

<pre><code>
user=> (sorted-set-by #(> (first %) (first %2)) [1 2] [-5 10] [1 5])
#{[1 2] [-5 10]}

Notice that because the provided comparison fn determines that [1 2] and [1 5] have the same sort order, the latter value is considered identical to the former, and not included in the set. This behaviour could be very handy, but is also likely to cause confusion when what the user almost certainly wants is to maintain the membership semantics of the original set (e.g. relying upon equality/hashCode), but only modify the ordering.

(BTW, yes, I know there's far easier ways to get the order I'm indicating above over a set of vectors thanks to vectors being comparable via the compare fn. The examples are only meant to be illustrative. The same non-intuitive result would occur, with no easy fallback (like the 'compare' fn when working with vectors) when the members of the set are non-Comparable Java object, and the comparator provided to sorted-set-by is defining a sort over some values returned by method calls into those objects.)

I'd be happy to change the docs for sorted-set-by, but I suspect that there are others who could encapsulate what's going on here more correctly and more concisely than I.



 Comments   
Comment by Assembla Importer [ 24/Aug/10 7:45 AM ]

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

Comment by Assembla Importer [ 24/Aug/10 7:45 AM ]

richhickey said: Updating tickets (#127, #128, #129, #130)





[CLJ-19] GC Issue 15: JavaDoc for interfaces Created: 17/Jun/09  Updated: 03/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Anonymous Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: documentation


 Description   
Reported by richhickey, Dec 17, 2008
Add JavaDoc to those interfaces supported for public use - IFn,
IPersistentCollection etc.


 Comments   
Comment by Assembla Importer [ 24/Aug/10 6:44 AM ]

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

Comment by Assembla Importer [ 24/Aug/10 6:44 AM ]

richhickey said: Updating tickets (#8, #19, #30, #31, #126, #17, #42, #47, #50, #61, #64, #69, #71, #77, #79, #84, #87, #89, #96, #99, #103, #107, #112, #113, #114, #115, #118, #119, #121, #122, #124)

Comment by Kevin Downey [ 17/Nov/12 8:05 PM ]

this seems like a great task for someone just starting out contributing to clojure.





[CLJ-1045] Generalize/refactor implementation of PersistentVector/coll-fold Created: 18/Aug/12  Updated: 03/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Alan Malloy Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: reducers

Attachments: Text File clj-1045-fold-by-halves-patch-v2.txt     Text File fold-by-halves.patch    
Patch: Code

 Description   

Vector currently contains a specialized implementation of the folding algorithm "split the collection in half until the pieces are small enough". The attached commit lifts out the general strategy so that it can be reused by other collection types amenable to splitting.

CLJ-993 depends on this patch, as it uses the new fold-by-halves function.



 Comments   
Comment by Andy Fingerhut [ 25/Jan/13 2:29 PM ]

clj-1045-fold-by-halves-patch-v2.txt dated Jan 25 2013 is identical to fold-by-halves.patch dated Aug 18 2012, except it updates one line of context changed by a recent commit to Clojure master.





[CLJ-153] Suggest adding set-precision! API to accompany with-precision Created: 12/Jul/09  Updated: 04/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Anonymous Assignee: Unassigned
Resolution: Unresolved Votes: 0
Labels: math


 Description   

Ticket #137 makes math-context</code> settable at the REPL. However, <code>math-context is not a public name in Clojure.

The related public function is with-precision</code> which works by pushing a new binding for <code>math-context</code>. This ticket suggests adding <code>set-precision!</code> to Clojure's public API. Its effect would be the same as <code>with-precision</code>, but accomplished by using <code>set!</code> on the current <code>math-context binding rather than by pushing a new binding.

Chouser suggests that we also add a doc string for math-context</code> noting that it is private and pointing the user to <code>with-precision</code> and <code>set-precision!. I agree.



 Comments   
Comment by Assembla Importer [ 24/Aug/10 12:55 AM ]

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





[CLJ-1096] Make destructuring emit direct keyword lookups Created: 29/Oct/12  Updated: 04/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Christophe Grand Assignee: Christophe Grand
Resolution: Unresolved Votes: 0
Labels: performance

Attachments: File desctructure-keyword-lookup.diff     File inline-get-keyword.diff    
Patch: Code

 Description   

Currently associative destructuring emits calls to get. The attached patch modify desctruture to emit direct keyword lookups when possible.

Approved here https://groups.google.com/d/msg/clojure-dev/MaYcHQck8VA/nauMus4mzPgJ



 Comments   
Comment by Christophe Grand [ 04/Sep/13 3:40 AM ]

Rethinking about this patch now, it may be too specific: get's inline expansion should be modified when the key is a literal keyword.

Comment by Christophe Grand [ 04/Sep/13 3:41 AM ]

More generic patch (inline-get-keyword.diff): all get calls with literal keywords as keys are inlined to direct keyword lookup.





[CLJ-272] load/ns/require/use overhaul Created: 18/Feb/10  Updated: 04/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Assembla Importer Assignee: Unassigned
Resolution: Unresolved Votes: 3
Labels: None


 Description   

Creating this ticket to describe various things people have wanted to change about how ns works:

Minimal needs

  1. there should be a primitive level of loading (presumably load) that just loads without question.
  2. the api should be unified across the ns and direct forms. No more keywords or quoting! So (use foo) not (use 'foo). This makes use et al macros, so there should also be new fn versions (maybe use*).

Other possibilities to discuss.

  1. Feature addressing the :like and :clone ideas from http://onclojure.com/2010/02/17/managing-namespaces/. I think I would prefer a single new option :clone which allows :only and :exclude features as subspecifiers.
  2. Convenience fn to unmap all names in a namespace?


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

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

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

stu said: Suggestions from Volkan Yazici:

Hi,

I saw your "load/ns/require/use overhaul" ticket[1] and would like to
ask for a few extra overhaulings. I have a project called retop, and
here is its file hiearachy:

tr/edu/bilkent/cs/retop.clj
tr/edu/bilkent/cs/retop/km.clj
tr/edu/bilkent/cs/retop/graph.clj
tr/edu/bilkent/cs/retop/main.clj
tr/edu/bilkent/cs/retop/util.clj

In retop.clj, I have below ns definition.

(ns tr.edu.bilkent.cs.retop
(:gen-class)
(:import
(com.sun.jna
Function
Pointer)
(com.sun.jna.ptr
IntByReference)
(tr.edu.bilkent.cs.patoh
HyperGraph
HyperGraphException
Parititoning
ParititoningParameters))
(:load
"retop/util"
"retop/km"
"retop/graph"
"retop/main"))

And in every .clj file in retop/ directory I have below in-ns in the
very first line.

(in-ns 'tr.edu.bilkent.cs.retop)

The problems with the ns decleration are:

1) Most of the :import's in retop.clj only belong to a single .clj file.
For instance,

(tr.edu.bilkent.cs.patoh
HyperGraph
HyperGraphException
Parititoning
ParititoningParameters)

imports are only used by graph.clj. Yep, I can add an (import ...)
line just after the (in-ns ...), but wouldn't it be better if I can
specify that in (in-ns ...) form?

2) See (:load ...) clause in (ns ...) form. There are lots of
unnecessary directory prefixes. I'd be prefer something ala Common
Lisp's defpackage:

(:load
"packages" ; packages.clj
("retop"
"util" ; retop/util.clj
"km" ; retop/km.clj
"graph" ; retop/graph.clj
("graph"
"foo" ; retop/graph/foo.clj
"bar) ; retop/graph/bar.clj
"main")) ; retop/main.clj

Also, being able to use wildcards would be awesome.

3) There are inconsistencies between macros and functions. For instance,
consider:

(ns foo.bar.baz (:use mov))
(in-ns 'foo.bar.baz)
(use 'mov)

I'd like to get rid of quotations in both cases.

I'm not sure if I'm using the right tools and doing the right approach
for such a project. But if you agree with the above overhauling
requirements, I'd like to see them appear in the same assembla ticket as
well.

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

stuart.sierra said: My requests:

1. If writing macros that do not evaluate their arguments, provide function versions that do evaluate their arguments.

2. Do not support prefix lists for loading Clojure namespaces. It's hard to parse with external tools.

3. Do not conflate importing Java classes with loading Clojure namespaces. They are fundamentally different operations with different semantics.

I have implemented some ideas in a macro called "need" at http://github.com/stuartsierra/need

Comment by Stuart Sierra [ 12/Dec/10 4:08 PM ]

Further requests:

Permit tools to read the "ns" declaration and statically determine the dependencies of a namespace, without evaluating any code.

Comment by Paudi Moriarty [ 28/Feb/12 3:56 AM ]

Permit tools to read the "ns" declaration and statically determine the dependencies of a namespace, without evaluating any code.

This would be great for building OSGi bundles where Bnd is currently not much help.





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

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

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

Attachments: Text File clj-1191-patch-v1.txt    
Patch: Code
Approval: Triaged

 Description   

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

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

It would be nice if the returned symbols could indicate the namespace, either always, or if the found symbol is not in the current namespace.



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

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

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

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

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

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

+1

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

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





[CLJ-1169] Add filename and line number to defn parameter declaration error Created: 22/Feb/13  Updated: 28/Sep/13

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

Type: Enhancement Priority: Major
Reporter: Andrei Kleschinski Assignee: Unassigned
Resolution: Unresolved Votes: 2
Labels: errormsgs
Environment:

Windows


Attachments: Text File 0001-CLJ-1169-proposed-patch.patch     Text File 0002-CLJ-1169-fix-unit-tests.patch     File defn_error_message.clj    
Patch: Code
Approval: Triaged

 Description   

When mistyping parameter list in defn declaration, e.g.

(defn test
(some-error))

error message shows name of parameter (without quotes), but not function name, filename or line number:

Exception in thread "main" java.lang.IllegalArgumentException: Parameter declaration some-error should be a vector
at clojure.core$assert_valid_fdecl.invoke(core.clj:6567)
at clojure.core$sigs.invoke(core.clj:220)
at clojure.core$defn.doInvoke(core.clj:294)
at clojure.lang.RestFn.invoke(RestFn.java:467)
at clojure.lang.Var.invoke(Var.java:427)
at clojure.lang.AFn.applyToHelper(AFn.java:172)
at clojure.lang.Var.applyTo(Var.java:532)
at clojure.lang.Compiler.macroexpand1(Compiler.java:6366)
at clojure.lang.Compiler.macroexpand(Compiler.java:6427)
at clojure.lang.Compiler.eval(Compiler.java:6495)
at clojure.lang.Compiler.load(Compiler.java:6952)
at clojure.lang.Compiler.loadFile(Compiler.java:6912)
at clojure.main$load_script.invoke(main.clj:283)
at clojure.main$init_opt.invoke(main.clj:288)
at clojure.main$initialize.invoke(main.clj:316)
at clojure.main$null_opt.invoke(main.clj:349)
at clojure.main$main.doInvoke(main.clj:427)
at clojure.lang.RestFn.invoke(RestFn.java:421)
at clojure.lang.Var.invoke(Var.java:419)
at clojure.lang.AFn.applyToHelper(AFn.java:163)
at clojure.lang.Var.applyTo(Var.java:532)
at clojure.main.main(main.java:37)



 Comments   
Comment by Andrei Kleschinski [ 22/Feb/13 5:39 AM ]

Proposed patch for issue
Process exceptions in macroexpand1 and wraps them in CompilerException with source,line,column information.

Also patch adds quotes around invalid symbol name in error message to make them more distinguishable from rest of message.

Comment by Andy Fingerhut [ 01/Mar/13 9:32 AM ]

Patch 0001-CLJ-1169-proposed-patch.patch dated Feb 22 2013 causes several tests to fail. Run "./antsetup.sh" then "ant" to see which ones (or "mvn package").

Comment by Andrei Kleschinski [ 01/Mar/13 10:25 AM ]

Fix for failed unit-tests





[CLJ-322] Enhance AOT compilation process to emit classfiles only for explicitly-specified namespaces Created: 29/Apr/10  Updated: 22/Oct/13

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

Type: Enhancement Priority: Major
Reporter: Chas Emerick Assignee: Unassigned
Resolution: Unresolved Votes: 11
Labels: aot

Attachments: Text File 0322-limit-aot-resolved.patch     File CLJ-322.diff     Text File compile-interop-1.patch     GZip Archive write-classes-1.diff.gz    
Patch: Code and Test
Approval: Vetted
Waiting On: Chas Emerick

 Description   

Summary: still needs decision on implementation approach.

This was originally/erroneously reported by Howard Lewis Ship in the clojure-contrib assembla:

My build file specifies the namespaces to AOT compile but if I include another namespace
(even from a JAR dependency) that is not AOT compiled, the other namespace will be compiled as well.

In my case, I was using clojure-contrib's clojure.contrib.str-utils2 namespace, and I got a bunch of
clojure/contrib/str_utils2 classes in my output directory.

I think that the AOT compiler should NOT precompile any namespaces that are transitively reached,
only namespaces in the set specified by the command line are appropriate.

As currently coded, you will frequently find unwanted third-party dependencies in your output JARs;
further, if multiple parties depend on the same JARs, this could cause bloating and duplication in the
eventual runtime classpath.

Having the option of shipping either all AOT-compiled classfiles or mixed source/AOT depending upon one's distribution requirements would make that phase of work with a clojure codebase significantly easier and less error-prone. The only question in my mind is what the default should be. We're all used to the current behaviour, but I'd guess that any nontrivial project where the form of the distributable matters (i.e. the source/AOT mix), providing as much control as possible by default makes the most sense. Given the tooling that most people are using, it's trivial (and common practice, IIUC) to provide a comprehensive list of namespaces one wishes to compile, so making that the default shouldn't be a hurdle to anyone. If an escape hatch is desired, a --transitive switch to clojure.lang.Compile could be added.



 Comments   
Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

Converted from http://www.assembla.com/spaces/clojure/tickets/322
Attachments:
aot-transitivity-option-compat-322.diff - https://www.assembla.com/spaces/clojure/documents/aI7Eu-HeGr35ImeJe5cbLA/download/aI7Eu-HeGr35ImeJe5cbLA
aot-transitivity-option-322.diff - https://www.assembla.com/spaces/clojure/documents/aIWFiWHeGr35ImeJe5cbLA/download/aIWFiWHeGr35ImeJe5cbLA

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

hlship said: I'd like to reinforce this. I've been doing research on Clojure build tools for an upcoming talk and all of them (Maven, Leiningen, Gradle) have the same problem: the AOT compile extends from the desired namespaces (such as one containing a :gen-class) to every reached namespace. This is going to cause a real ugliness when application A uses libraries B and C that both depend on library D (such as clojure-contrib) and B and C are thus both bloated with duplicate, unwanted AOT compiled classes from the library D.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

cemerick said: This behaviour is an implementation detail of Clojure's AOT compilation process, and is orthogonal to any particular build tooling.

I am working on a patch that would provide a mechanism for such tooling to disable this default behaviour.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

cemerick said: A first cut of a change to address this issue is here (caution, work in progress!):

http://github.com/cemerick/clojure/commit/6f14e0790c0d283a7e44056adf1bb3f36bb16e0e

This makes available a new recognized system property, clojure.compiler.transitive, which defaults to true. When set/bound to false (i.e. -Dclojure.compiler.transitive=false when using clojure.lang.Compile), only the first loaded file (either the ns named in the call to compile or each of the namespaces named as arguments to clojure.lang.Compile) will have classfiles written to disk.

This means that this compilation invocation:

java -cp <your classpath> -Dclojure.compiler.transitive=false clojure.lang.Compile com.bar com.baz

will generate classfiles only for com.bar and com.baz, but not for any of the namespaces or other files they load, require, or use.


The only shortcoming of this WIP patch is that classfiles are still generated for proxy and gen-class classes defined outside of the explicitly-named namespaces. What I thought was a solution for this ended up breaking the loading of generated interfaces (as produced by defprotocol, etc).

I'll take a second look at this before the end of the week, but wanted to get this out there so as to get any comments people might have.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

technomancy said: Looks good, but I'm having trouble getting it to work. I tried compiling from master of Chas's fork on github, but I still got the all the .class files generated with -Dclojure.compiler.transitive=false. It could be a quirk of the way I'm using ant to fork off processes though. Is it possible to set it using System/setProperty, or must it be given as a property on the command-line?

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

cemerick said: Bah, that's just bad documentation. :-/

The system property is only provided by clojure.lang.Compile; the value of it drives the binding of clojure.core/transitive-compile, which has a root binding of true.

You should be able to configure the transitivity the same way you configure compile-path (system prop to clojure.lang.Compile or a direct binding when at the REPL, etc).

If not, ping me in irc or elsewhere.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

meikelbrandmeyer said: I think, excluding parts 'load'ed is a little strong. I have some namespaces which load several parts from different files, but which belong to the same namespace. The most prominent example of such a case is clojure.core itself. I'm find with stopping require and use, but load is a bit too much, I'd say.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

technomancy said: Chas: Thanks; will give that a go.

Meikel: Do people actually use load outside of clojure.core? I thought it was only used there because clojure.core is a "special" namespace where you want more vars to be available than can reasonably fit in a single file. Splitting up a namespace into several files is quite unadvisable otherwise.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

technomancy said: I can confirm that this works for me modulo the proxy/gen-class issue that Chas mentioned. I would love to see this in Clojure 1.2; it would really clean up a lot of build-related issues.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

meikelbrandmeyer said: I used it several times and this is the first time, I hear that it is unadvisable to do so. Even with a lower number of Vars in the namespace (c.c is here certainly exceptional) and might be of use to split several "sections" of code which belong to the same namespace but have different functionality. Whether to use a big comment in the source to indicate the section or split things into subfiles is a matter of taste. But it's a perfectly reasonable thing todo.

Another use case, where I use this (and c.c.lazy-xml, IIRC) is to conditionally load code depending on whether a dependency is available or not. Eg. vimclojure uses c.c.pprint and c.c.stacktrace/clj-stacktrace transparently depending on their availability.

There are perfectly legal uses of load. I don't see any "unadvisable" here.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

cemerick said: Thanks, Meikel; I had forgotten about that use case, as I don't use load directly myself at all. I probably wouldn't say it's inadvisable, just mostly unnecessary. In any case, that's a good catch. It complicates things a bit, but we'll see what happens. I'm going to take another whack at resolving the proxy/gen-class case and narrowing the impact of nontransitivity to use and require later tonight.

I agree wholeheartedly that this should be in 1.2, assuming the technical bits work out. This has been an irritant for quite a long time. I actually believe that nontransitivity should be the default – no one wants or expects to have classfiles show up for dependencies show up when a project is AOT-compiled. I think the only negative impact would be whoever still fiddles with compilation at the REPL, and doesn't use maven or lein – and even then, it's just a matter of binding another var.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

meikelbrandmeyer said: Then the var should be added to the default bindings in the clojure.main repl. Then it's set!-able like the other vars ��� warn-on-reflection and friends.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

cemerick said: This is looking pretty good (still WIP):

http://github.com/cemerick/clojure/commit/fedfb022ecef420a932b3d69c182ec7a8e5960a6

Thank you again for mentioning load, Meikel: it was very helpful in resolving the proxy/gen-class issue as well.

Just a single data point: the jar produced by the medium-sized project I've been using for testing the changes has shrunk from 1.8MB to less than 1MB. That's not the only reason this is a good change, but it's certainly a nice side-effect.

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

cemerick said: [file:aIWFiWHeGr35ImeJe5cbLA]

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

cemerick said: [file:aI7Eu-HeGr35ImeJe5cbLA]

Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

cemerick said: Patched attached. The compat one retains the current default behaviour [*transitive-compile* true], the other changes the default so that transitivity is a non-default option. At least of those I've spoken to about this, the latter is preferred.

The user impact of changing the default would be:

  1. The result of compiling from the REPL will change. Getting back current behaviour would require adding a [*transitive-compile* true] binding to the existing bindings one must set when compiling from the REPL.
  2. The same as #1 goes for those scripting AOT compilation via clojure.lang.Compile as well (whether by shell scripts, ant, etc).
  3. Those using lein, clojure-maven-plugin, gradle, and others will likely have a new option provided by those tools, and perhaps a different default than the language's. I suspect those using such tools would much prefer a change from the default behaviour in any case.
Comment by Assembla Importer [ 28/Sep/10 12:18 AM ]

hlship said: Just had a brain-storm:

How about an option to support transitive compilation, but only if the Clojure source file being compiled as a file: URL (i.e., its a local file on the file system, not a file stored in a JAR). That would make it easier to use compilation on the local project without transi