Details
-
Type:
Defect
-
Status:
Open
-
Priority:
Major
-
Resolution: Unresolved
-
Affects Version/s: Backlog
-
Fix Version/s: Release 1.11
-
Component/s: None
-
Labels:None
-
Environment:Clojure commit 9052ca1854b7b6202dba21fe2a45183a4534c501, version 1.3.0-master-SNAPSHOT
-
Patch:Code and Test
-
Approval:Incomplete
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
See Also: CLJ-1422
Patch: 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti-v2.patch
Attachments
-
- 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch
- 28/Aug/15 11:51 AM
- 25 kB
- Nicola Mometto
-
- 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti-v2.patch
- 28/Aug/15 6:38 PM
- 24 kB
- Nicola Mometto
-
- 0002-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch
- 19/Jan/16 4:04 PM
- 60 kB
- Kevin Downey
-
- hoistedmethod-pass-1.diff
- 22/Jul/14 8:39 PM
- 22 kB
- Kevin Downey
-
- hoistedmethod-pass-2.diff
- 23/Jul/14 12:43 AM
- 24 kB
- Kevin Downey
-
- hoistedmethod-pass-3.diff
- 25/Jul/14 7:08 PM
- 24 kB
- Kevin Downey
-
- hoistedmethod-pass-4.diff
- 26/Jul/14 12:52 PM
- 29 kB
- Kevin Downey
-
- hoistedmethod-pass-5.diff
- 26/Jul/14 12:58 PM
- 29 kB
- Kevin Downey
-
- hoistedmethod-pass-6.diff
- 25/Aug/15 12:58 PM
- 36 kB
- Kevin Downey
-
- hoistedmethod-pass-7.diff
- 27/Aug/15 9:20 PM
- 52 kB
- Kevin Downey
Activity
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.
- 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.
I don't think this is going to make it into 1.6, so removing the 1.6 tag.
an immediate solution to this might be to hoist loops out in to distinct non-ifn types generated by the compiler with an invoke method that is typed to return the getJavaClass() type of the expression, that would give us the simplifying benefits of hoisting the code out and free use from the Object semantics of ifn
I have attached a 3 part patch as hoistedmethod-pass-1.diff
3ed6fed8 adds a new ObjMethod type to represent expressions hoisted out in to their own methods on the enclosing class
9c39cac1 uses HoistedMethod to compile loops not in the return context
901e4505 hoists out try expressions and makes it possible for try to return a primitive expression (this might belong on http://dev.clojure.org/jira/browse/CLJ-1422)
with hoistedmethod-pass-1.diff the example code generates bytecode like this
user=> (println (no.disassemble/disassemble (fn [] (loop [b 0] (recur (loop [a 1] a)))))) // Compiled from form-init1272682692522767658.clj (version 1.5 : 49.0, super bit) public final class user$eval1675$fn__1676 extends clojure.lang.AFunction { // Field descriptor #7 Ljava/lang/Object; public static final java.lang.Object const__0; // Field descriptor #7 Ljava/lang/Object; public static final java.lang.Object const__1; // Method descriptor #10 ()V // Stack: 2, Locals: 0 public static {}; 0 lconst_0 1 invokestatic java.lang.Long.valueOf(long) : java.lang.Long [16] 4 putstatic user$eval1675$fn__1676.const__0 : java.lang.Object [18] 7 lconst_1 8 invokestatic java.lang.Long.valueOf(long) : java.lang.Long [16] 11 putstatic user$eval1675$fn__1676.const__1 : java.lang.Object [20] 14 return Line numbers: [pc: 0, line: 1] // Method descriptor #10 ()V // Stack: 1, Locals: 1 public user$eval1675$fn__1676(); 0 aload_0 [this] 1 invokespecial clojure.lang.AFunction() [23] 4 return Line numbers: [pc: 0, line: 1] // Method descriptor #25 ()Ljava/lang/Object; // Stack: 3, Locals: 3 public java.lang.Object invoke(); 0 lconst_0 1 lstore_1 [b] 2 aload_0 [this] 3 lload_1 [b] 4 invokevirtual user$eval1675$fn__1676.__hoisted1677(long) : long [29] 7 lstore_1 [b] 8 goto 2 11 areturn Line numbers: [pc: 0, line: 1] Local variable table: [pc: 2, pc: 11] local: b index: 1 type: long [pc: 0, pc: 11] local: this index: 0 type: java.lang.Object // Method descriptor #27 (J)J // Stack: 2, Locals: 5 public long __hoisted1677(long b); 0 lconst_1 1 lstore_3 [a] 2 lload_3 3 lreturn Line numbers: [pc: 0, line: 1] Local variable table: [pc: 2, pc: 3] local: a index: 3 type: long [pc: 0, pc: 3] local: this index: 0 type: java.lang.Object [pc: 0, pc: 3] local: b index: 1 type: java.lang.Object } nil user=>
the body of the method __hoisted1677 is the inner loop
for reference the part of the bytecode from the same function compiled with 1.6.0 is pasted here https://gist.github.com/hiredman/f178a690718bde773ba0 the inner loop body is missing because it is implemented as its own IFn class that is instantiated and immediately executed. it closes over a boxed version of the numbers and returns an boxed version
user=> (println (no.disassemble/disassemble (fn [] (loop [b 0] (recur (loop [a 1] a)))))) // Compiled from form-init1272682692522767658.clj (version 1.5 : 49.0, super bit) public final class user$eval1675$fn__1676 extends clojure.lang.AFunction { // Field descriptor #7 Ljava/lang/Object; public static final java.lang.Object const__0; // Field descriptor #7 Ljava/lang/Object; public static final java.lang.Object const__1; // Method descriptor #10 ()V // Stack: 2, Locals: 0 public static {}; 0 lconst_0 1 invokestatic java.lang.Long.valueOf(long) : java.lang.Long [16] 4 putstatic user$eval1675$fn__1676.const__0 : java.lang.Object [18] 7 lconst_1 8 invokestatic java.lang.Long.valueOf(long) : java.lang.Long [16] 11 putstatic user$eval1675$fn__1676.const__1 : java.lang.Object [20] 14 return Line numbers: [pc: 0, line: 1] // Method descriptor #10 ()V // Stack: 1, Locals: 1 public user$eval1675$fn__1676(); 0 aload_0 [this] 1 invokespecial clojure.lang.AFunction() [23] 4 return Line numbers: [pc: 0, line: 1] // Method descriptor #25 ()Ljava/lang/Object; // Stack: 3, Locals: 3 public java.lang.Object invoke(); 0 lconst_0 1 lstore_1 [b] 2 aload_0 [this] 3 lload_1 [b] 4 invokevirtual user$eval1675$fn__1676.__hoisted1677(long) : long [29] 7 lstore_1 [b] 8 goto 2 11 areturn Line numbers: [pc: 0, line: 1] Local variable table: [pc: 2, pc: 11] local: b index: 1 type: long [pc: 0, pc: 11] local: this index: 0 type: java.lang.Object // Method descriptor #27 (J)J // Stack: 2, Locals: 5 public long __hoisted1677(long b); 0 lconst_1 1 lstore_3 [a] 2 lload_3 3 lreturn Line numbers: [pc: 0, line: 1] Local variable table: [pc: 2, pc: 3] local: a index: 3 type: long [pc: 0, pc: 3] local: this index: 0 type: java.lang.Object [pc: 0, pc: 3] local: b index: 1 type: java.lang.Object } nil user=>
hoistedmethod-pass-2.diff replaces 901e4505 with f0a405e3 which fixes the implementation of MaybePrimitiveExpr for TryExpr
with hoistedmethod-pass-2.diff the largest clojure project I have quick access to (53kloc) compiles and all the tests pass
I have been working through running the tests for all the contribs projects with hoistedmethod-pass-2.diff, there are some bytecode verification errors compiling data.json and other errors elsewhere, so there is still work to do
hoistedmethod-pass-3.diff
49782161 * add HoistedMethod to the compiler for hoisting expresssions out well typed methods
e60e6907 * hoist out loops if required
547ba069 * make TryExpr MaybePrimitive and hoist tries out as required
all contribs whose tests pass with master pass with this patch.
the change from hoistedmethod-pass-2.diff in this patch is the addition of some bookkeeping for arguments that take up more than one slot
Kevin there's still a bug regarding long/doubles handling:
On commit 49782161, line 101 of the diff, you're emitting gen.pop() if the expression is in STATEMENT position, you need to emit gen.pop2() instead if e.getReturnType is long.class or double.class
Test case:
user=> (fn [] (try 1 (finally)) 2) VerifyError (class: user$eval1$fn__2, method: invoke signature: ()Ljava/lang/Object;) Attempt to split long or double on the stack user/eval1 (NO_SOURCE_FILE:1)
user=> (fn [] (try 1 (finally)) 2) VerifyError (class: user$eval1$fn__2, method: invoke signature: ()Ljava/lang/Object;) Attempt to split long or double on the stack user/eval1 (NO_SOURCE_FILE:1)
bah, all that work to figure out the thing I couldn't get right and of course I overlooked the thing I knew at the beginning. I want to get rid of some of the code duplication between emit and emitUnboxed for TryExpr, so when I get that done I'll fix the pop too
hoistedmethod-pass-4.diff logically has the same three commits, but fixes the pop vs pop2 issue and rewrites emit and emitUnboxed for TryExpr to share most of their code
hoistedmethod-pass-5.diff fixes a stupid mistake in the tests in hoistedmethod-pass-4.diff
A naive attempt to bring the patch up to date results in
compile-clojure: [java] Exception in thread "main" java.lang.ExceptionInInitializerError [java] at clojure.lang.Compile.<clinit>(Compile.java:29) [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod, compiling:(clojure/core.clj:439:11) [java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:7360) [java] at clojure.lang.Compiler.analyze(Compiler.java:7154) [java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:7341) [java] at clojure.lang.Compiler.analyze(Compiler.java:7154) [java] at clojure.lang.Compiler.access$300(Compiler.java:38) [java] at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:589) [java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:7353) [java] at clojure.lang.Compiler.analyze(Compiler.java:7154) [java] at clojure.lang.Compiler.analyze(Compiler.java:7112) [java] at clojure.lang.Compiler.eval(Compiler.java:7416) [java] at clojure.lang.Compiler.load(Compiler.java:7859) [java] at clojure.lang.RT.loadResourceScript(RT.java:372) [java] at clojure.lang.RT.loadResourceScript(RT.java:363) [java] at clojure.lang.RT.load(RT.java:453) [java] at clojure.lang.RT.load(RT.java:419) [java] at clojure.lang.RT.doInit(RT.java:461) [java] at clojure.lang.RT.<clinit>(RT.java:331) [java] ... 1 more [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod [java] at clojure.lang.Compiler$FnExpr.parse(Compiler.java:4466) [java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:7351) [java] ... 17 more
compile-clojure: [java] Exception in thread "main" java.lang.ExceptionInInitializerError [java] at clojure.lang.Compile.<clinit>(Compile.java:29) [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod, compiling:(clojure/core.clj:439:11) [java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:7360) [java] at clojure.lang.Compiler.analyze(Compiler.java:7154) [java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:7341) [java] at clojure.lang.Compiler.analyze(Compiler.java:7154) [java] at clojure.lang.Compiler.access$300(Compiler.java:38) [java] at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:589) [java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:7353) [java] at clojure.lang.Compiler.analyze(Compiler.java:7154) [java] at clojure.lang.Compiler.analyze(Compiler.java:7112) [java] at clojure.lang.Compiler.eval(Compiler.java:7416) [java] at clojure.lang.Compiler.load(Compiler.java:7859) [java] at clojure.lang.RT.loadResourceScript(RT.java:372) [java] at clojure.lang.RT.loadResourceScript(RT.java:363) [java] at clojure.lang.RT.load(RT.java:453) [java] at clojure.lang.RT.load(RT.java:419) [java] at clojure.lang.RT.doInit(RT.java:461) [java] at clojure.lang.RT.<clinit>(RT.java:331) [java] ... 1 more [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod [java] at clojure.lang.Compiler$FnExpr.parse(Compiler.java:4466) [java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:7351) [java] ... 17 more
the pass 6 patch builds cleanly on 1d5237f9d7db0bc5f6e929330108d016ac7bf76c(HEAD of master as of this moment) and runs clojure's tests. I have not verified it against other projects as I did with the previous patches (I don't remember how I did that)
I get the same error I shared before applying the new patch to 1d5237f
I get some whitespace warnings with hoistedmethod-pass-6.diff but the patch applies. On a compile I get:
compile-clojure: [java] Exception in thread "main" java.lang.ExceptionInInitializerError [java] at clojure.lang.Compile.<clinit>(Compile.java:29) [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod, compiling:(clojure/core.clj:439:11) [java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:7362) [java] at clojure.lang.Compiler.analyze(Compiler.java:7156) [java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:7343) [java] at clojure.lang.Compiler.analyze(Compiler.java:7156) [java] at clojure.lang.Compiler.access$300(Compiler.java:38) [java] at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:588) [java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:7355) [java] at clojure.lang.Compiler.analyze(Compiler.java:7156) [java] at clojure.lang.Compiler.analyze(Compiler.java:7114) [java] at clojure.lang.Compiler.eval(Compiler.java:7418) [java] at clojure.lang.Compiler.load(Compiler.java:7861) [java] at clojure.lang.RT.loadResourceScript(RT.java:372) [java] at clojure.lang.RT.loadResourceScript(RT.java:363) [java] at clojure.lang.RT.load(RT.java:453) [java] at clojure.lang.RT.load(RT.java:419) [java] at clojure.lang.RT.doInit(RT.java:461) [java] at clojure.lang.RT.<clinit>(RT.java:331) [java] ... 1 more [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod [java] at clojure.lang.Compiler$FnExpr.parse(Compiler.java:4468) [java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:7353) [java] ... 17 more
compile-clojure: [java] Exception in thread "main" java.lang.ExceptionInInitializerError [java] at clojure.lang.Compile.<clinit>(Compile.java:29) [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod, compiling:(clojure/core.clj:439:11) [java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:7362) [java] at clojure.lang.Compiler.analyze(Compiler.java:7156) [java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:7343) [java] at clojure.lang.Compiler.analyze(Compiler.java:7156) [java] at clojure.lang.Compiler.access$300(Compiler.java:38) [java] at clojure.lang.Compiler$DefExpr$Parser.parse(Compiler.java:588) [java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:7355) [java] at clojure.lang.Compiler.analyze(Compiler.java:7156) [java] at clojure.lang.Compiler.analyze(Compiler.java:7114) [java] at clojure.lang.Compiler.eval(Compiler.java:7418) [java] at clojure.lang.Compiler.load(Compiler.java:7861) [java] at clojure.lang.RT.loadResourceScript(RT.java:372) [java] at clojure.lang.RT.loadResourceScript(RT.java:363) [java] at clojure.lang.RT.load(RT.java:453) [java] at clojure.lang.RT.load(RT.java:419) [java] at clojure.lang.RT.doInit(RT.java:461) [java] at clojure.lang.RT.<clinit>(RT.java:331) [java] ... 1 more [java] Caused by: java.lang.ClassCastException: clojure.lang.Compiler$HoistedMethod cannot be cast to clojure.lang.Compiler$FnMethod [java] at clojure.lang.Compiler$FnExpr.parse(Compiler.java:4468) [java] at clojure.lang.Compiler.analyzeSeq(Compiler.java:7353) [java] ... 17 more
Looks like direct linking interacts with the diffs in this patch in non trivial ways.
I must have screwed up running the tests some how, I definitely get the same error now
after you get past the cast issuing (just adding some conditional logic there)
it looks like HoistedMethodInvocationExpr needs to be made aware of if it is emitting in an instance method or a static method, and do the right thing with regard to this pointers and argument offsets. this will likely require HoistedMethod growing the ability to be a static method (and maybe preferring static methods when possible).
if you cause HoistedMethod to set usesThis to true on methods that use it, then everything appears hunky-dory (if I ran the tests correctly), but this largely negates the new direct linking stuff, which is not good.
hoistedmethod-pass-7.diff adds a single commit to hoistedmethod-pass-5.diff
the single commit changes hoisted methods to always be static methods, and adjusts the arguments in the invocation of the hoisted method based on if the containing function is a static/direct function or not.
again I haven't done the extended testing with this patch.
here is an example of what the hoisted methods look like
user> (println (disassemble (fn ^long [^long y] (let [x (try (/ 1 0) (catch Throwable t 0))] x)))) // Compiled from form-init3851661302895745152.clj (version 1.5 : 49.0, super bit) public final class user$eval1872$fn__1873 extends clojure.lang.AFunction implements clojure.lang.IFn$LL { // Field descriptor #9 Lclojure/lang/Var; public static final clojure.lang.Var const__0; // Field descriptor #11 Ljava/lang/Object; public static final java.lang.Object const__1; // Field descriptor #11 Ljava/lang/Object; public static final java.lang.Object const__2; // Method descriptor #14 ()V // Stack: 2, Locals: 0 public static {}; 0 ldc <String "clojure.core"> [16] 2 ldc <String "/"> [18] 4 invokestatic clojure.lang.RT.var(java.lang.String, java.lang.String) : clojure.lang.Var [24] 7 checkcast clojure.lang.Var [26] 10 putstatic user$eval1872$fn__1873.const__0 : clojure.lang.Var [28] 13 lconst_1 14 invokestatic java.lang.Long.valueOf(long) : java.lang.Long [34] 17 putstatic user$eval1872$fn__1873.const__1 : java.lang.Object [36] 20 lconst_0 21 invokestatic java.lang.Long.valueOf(long) : java.lang.Long [34] 24 putstatic user$eval1872$fn__1873.const__2 : java.lang.Object [38] 27 return Line numbers: [pc: 0, line: 1] // Method descriptor #14 ()V // Stack: 1, Locals: 1 public user$eval1872$fn__1873(); 0 aload_0 [this] 1 invokespecial clojure.lang.AFunction() [41] 4 return Line numbers: [pc: 0, line: 1] // Method descriptor #43 (J)J // Stack: 2, Locals: 4 public final long invokePrim(long y); 0 lload_1 [y] 1 invokestatic user$eval1872$fn__1873.__hoisted1874(long) : java.lang.Object [47] 4 astore_3 [x] 5 aload_3 [x] 6 aconst_null 7 astore_3 8 checkcast java.lang.Number [50] 11 invokevirtual java.lang.Number.longValue() : long [54] 14 lreturn Line numbers: [pc: 0, line: 1] Local variable table: [pc: 5, pc: 8] local: x index: 3 type: java.lang.Object [pc: 0, pc: 14] local: this index: 0 type: java.lang.Object [pc: 0, pc: 14] local: y index: 1 type: long // Method descriptor #59 (Ljava/lang/Object;)Ljava/lang/Object; // Stack: 5, Locals: 2 public java.lang.Object invoke(java.lang.Object arg0); 0 aload_0 [this] 1 aload_1 [arg0] 2 checkcast java.lang.Number [50] 5 invokestatic clojure.lang.RT.longCast(java.lang.Object) : long [63] 8 invokeinterface clojure.lang.IFn$LL.invokePrim(long) : long [65] [nargs: 3] 13 new java.lang.Long [30] 16 dup_x2 17 dup_x2 18 pop 19 invokespecial java.lang.Long(long) [68] 22 areturn // Method descriptor #45 (J)Ljava/lang/Object; // Stack: 4, Locals: 4 public static java.lang.Object __hoisted1874(long arg0); 0 lconst_1 1 lconst_0 2 invokestatic clojure.lang.Numbers.divide(long, long) : java.lang.Number [74] 5 astore_2 6 goto 17 9 astore_3 [t] 10 getstatic user$eval1872$fn__1873.const__2 : java.lang.Object [38] 13 astore_2 14 goto 17 17 aload_2 18 areturn Exception Table: [pc: 0, pc: 6] -> 9 when : java.lang.Throwable Line numbers: [pc: 0, line: 1] [pc: 2, line: 1] Local variable table: [pc: 9, pc: 14] local: t index: 3 type: java.lang.Object [pc: 0, pc: 18] local: y index: 1 type: java.lang.Object }
user> (println (disassemble (fn ^long [^long y] (let [x (try (/ 1 0) (catch Throwable t 0))] x)))) // Compiled from form-init3851661302895745152.clj (version 1.5 : 49.0, super bit) public final class user$eval1872$fn__1873 extends clojure.lang.AFunction implements clojure.lang.IFn$LL { // Field descriptor #9 Lclojure/lang/Var; public static final clojure.lang.Var const__0; // Field descriptor #11 Ljava/lang/Object; public static final java.lang.Object const__1; // Field descriptor #11 Ljava/lang/Object; public static final java.lang.Object const__2; // Method descriptor #14 ()V // Stack: 2, Locals: 0 public static {}; 0 ldc <String "clojure.core"> [16] 2 ldc <String "/"> [18] 4 invokestatic clojure.lang.RT.var(java.lang.String, java.lang.String) : clojure.lang.Var [24] 7 checkcast clojure.lang.Var [26] 10 putstatic user$eval1872$fn__1873.const__0 : clojure.lang.Var [28] 13 lconst_1 14 invokestatic java.lang.Long.valueOf(long) : java.lang.Long [34] 17 putstatic user$eval1872$fn__1873.const__1 : java.lang.Object [36] 20 lconst_0 21 invokestatic java.lang.Long.valueOf(long) : java.lang.Long [34] 24 putstatic user$eval1872$fn__1873.const__2 : java.lang.Object [38] 27 return Line numbers: [pc: 0, line: 1] // Method descriptor #14 ()V // Stack: 1, Locals: 1 public user$eval1872$fn__1873(); 0 aload_0 [this] 1 invokespecial clojure.lang.AFunction() [41] 4 return Line numbers: [pc: 0, line: 1] // Method descriptor #43 (J)J // Stack: 2, Locals: 4 public final long invokePrim(long y); 0 lload_1 [y] 1 invokestatic user$eval1872$fn__1873.__hoisted1874(long) : java.lang.Object [47] 4 astore_3 [x] 5 aload_3 [x] 6 aconst_null 7 astore_3 8 checkcast java.lang.Number [50] 11 invokevirtual java.lang.Number.longValue() : long [54] 14 lreturn Line numbers: [pc: 0, line: 1] Local variable table: [pc: 5, pc: 8] local: x index: 3 type: java.lang.Object [pc: 0, pc: 14] local: this index: 0 type: java.lang.Object [pc: 0, pc: 14] local: y index: 1 type: long // Method descriptor #59 (Ljava/lang/Object;)Ljava/lang/Object; // Stack: 5, Locals: 2 public java.lang.Object invoke(java.lang.Object arg0); 0 aload_0 [this] 1 aload_1 [arg0] 2 checkcast java.lang.Number [50] 5 invokestatic clojure.lang.RT.longCast(java.lang.Object) : long [63] 8 invokeinterface clojure.lang.IFn$LL.invokePrim(long) : long [65] [nargs: 3] 13 new java.lang.Long [30] 16 dup_x2 17 dup_x2 18 pop 19 invokespecial java.lang.Long(long) [68] 22 areturn // Method descriptor #45 (J)Ljava/lang/Object; // Stack: 4, Locals: 4 public static java.lang.Object __hoisted1874(long arg0); 0 lconst_1 1 lconst_0 2 invokestatic clojure.lang.Numbers.divide(long, long) : java.lang.Number [74] 5 astore_2 6 goto 17 9 astore_3 [t] 10 getstatic user$eval1872$fn__1873.const__2 : java.lang.Object [38] 13 astore_2 14 goto 17 17 aload_2 18 areturn Exception Table: [pc: 0, pc: 6] -> 9 when : java.lang.Throwable Line numbers: [pc: 0, line: 1] [pc: 2, line: 1] Local variable table: [pc: 9, pc: 14] local: t index: 3 type: java.lang.Object [pc: 0, pc: 18] local: y index: 1 type: java.lang.Object }
there is still an issue with patch 7, (defn f [^long y] (let [x (try (+ 1 0) (catch Throwable t y))] x)) causes a verifier error
Patch 0001-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch is patch hoistedmethod-pass-7 with the following changes:
- fixes the bug mentioned in the last comment by
Kevin Downey
- removes unnecessary whitespace and indentation changes
- conforms indentation with the surrounding lines
- squashes the commits into one as preferred by Rich
Authorship is maintained for Kevin Downey
- fixes the bug mentioned in the last comment by
Kevin Downey
- removes unnecessary whitespace and indentation changes
- conforms indentation with the surrounding lines
- squashes the commits into one as preferred by Rich

maybe we need a "Bronsa's Guide to Submitting Patches" to supplement http://dev.clojure.org/display/community/Developing+Patches, I had no idea a single commit was preferred, but that makes sense given the format, although I just noticed the bit on deleting old patches to avoid confusion. Is there a preferred format for patch names too?
If you have recommendations beyond patch formatting into the code itself I am all ears.
Kevin, having a single commit per patch is something that I've seen Rich and Alex ask for in a bunch of tickets, as I guess it makes it easier to evaluate the overall diff (even though it sacrifices granularity of description).
No idea for a preferred patch name, I find it hard to imagine it would matter – I just use whatever git format patch outputs.
One thing I personally prefer is to add the ticket name at the beginning of the commit message, it makes it easier to understand changes when using e.g. git blame
Just now I added a suggestion to http://dev.clojure.org/display/community/Developing+Patches that one read their patches before attaching them, and remove any spurious white space changes. Also to consider submitting patches with a single commit, rather than ones broken up into multiple commits, as reviewers tend to prefer those.
Alex Miller recently edited that page with the note about putting the ticket id first in the commit comment.
Only preference on patch file names is the one on that page – that they end with '.patch' or '.diff', because Rich's preferred editor for reading them recognizes those suffixes and displays the file in a patch-specific mode, I would guess.
I have a verify error on the latest patch, will attempt to provide a small test case:
Exception in thread "main" java.lang.VerifyError: (class: com/climate/scouting/homestead_test$fn__17125, method: invokeStatic signature: (Ljava/lang/Object;)Ljava/lang/Object Expecting to find integer on stack, compiling:(homestead_test.clj:43:3)

https://github.com/MichaelBlume/hoist-fail
I'd rather remove the dependency on compojure-api but I'm having trouble reproducing without it
Ok, less stupid test case:
(let [^boolean foo true] (do (try foo (catch Throwable t)) nil))
(let [^boolean foo true] (do (try foo (catch Throwable t)) nil))
...I wonder how hard it would be to write a generative test which produced small snippets of valid Clojure code, compiled them, and checked for errors. I bet we could've caught this.
Michael Blume I'm not sure this should be considered a compiler bug.
Contrary to numeric literals, boolean literals are never emitted unboxed and type hints are not casts so
(let [^boolean a true])
I'd say we wait for what Alex Miller or
Rich Hickey think of this before considering
Kevin Downey's current patch bugged, I personally (given the current compiler implementation) would consider this an user-code bug, currently ignored, that this patch merely exposes. IOW an instance of currently working code that is not however valid code
(I rest my case that if we had some better spec on what type-hints are supposed to be valid and some better compile-time validation on them, issues like this would not arise)

(let [^boolean a true])



If this is considered a bug, the fix is trivial btw
@@ -5888,7 +5888,7 @@ public static class LocalBinding{ public boolean hasJavaClass() { if(init != null && init.hasJavaClass() - && Util.isPrimitive(init.getJavaClass()) + && (Util.isPrimitive(init.getJavaClass()) || Util.isPrimitive(tag)) && !(init instanceof MaybePrimitiveExpr)) return false; return tag != null
@@ -5888,7 +5888,7 @@ public static class LocalBinding{ public boolean hasJavaClass() { if(init != null && init.hasJavaClass() - && Util.isPrimitive(init.getJavaClass()) + && (Util.isPrimitive(init.getJavaClass()) || Util.isPrimitive(tag)) && !(init instanceof MaybePrimitiveExpr)) return false; return tag != null
I imagine ^boolean type hints (that don't do anything and are ignored) are going to become very common in clojure code, given everyone's keen interest in code sharing between clojure and clojurescript, and iirc clojurescript actually using ^boolean
Last patch doesn't compile anymore since it hits the bug reported in CLJ-1809
Moving to incomplete for now since it seems to be blocked on the other ticket
0002-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch makes the change that Nicola suggested (with an extra null check). Michael's test case with the ^boolean type hint compiles now
Maybe this is out of scope for this ticket since it's just existing code that you're moving around, but I've always been confused by
//exception should be on stack
gen.visitVarInsn(OBJECT_TYPE.getOpcode(Opcodes.ISTORE), finallyLocal);
finallyExpr.emit(C.STATEMENT, objx, gen);
gen.visitVarInsn(OBJECT_TYPE.getOpcode(Opcodes.ILOAD), finallyLocal);
If we emit finallyExpr as C.STATEMENT, it should have no impact on the stack, and the exception should just be there without us needing to store and load it, right?
//exception should be on stack
gen.visitVarInsn(OBJECT_TYPE.getOpcode(Opcodes.ISTORE), finallyLocal);
finallyExpr.emit(C.STATEMENT, objx, gen);
gen.visitVarInsn(OBJECT_TYPE.getOpcode(Opcodes.ILOAD), finallyLocal);
Also the latest patch seems to add a patch file to the root directory
New failing code
(defn foo [req] (let [^boolean b (get req :is-baz)] (do (try :bar) :foo)))
(defn foo [req] (let [^boolean b (get req :is-baz)] (do (try :bar) :foo)))
the latest patch applied to master is causing some test failures for data.xml
Testing clojure.data.xml.test-seq-tree FAIL in (release-head-top) (test_seq_tree.clj:52) expected: (= nil (.get input-ref)) actual: (not (= nil (0 1 2 3 4 5 6 7 8 9))) FAIL in (release-head-nested-late) (test_seq_tree.clj:60) expected: (= nil (.get input-ref)) actual: (not (= nil (1 2 :< 3 4 5 :>))
Testing clojure.data.xml.test-seq-tree FAIL in (release-head-top) (test_seq_tree.clj:52) expected: (= nil (.get input-ref)) actual: (not (= nil (0 1 2 3 4 5 6 7 8 9))) FAIL in (release-head-nested-late) (test_seq_tree.clj:60) expected: (= nil (.get input-ref)) actual: (not (= nil (1 2 :< 3 4 5 :>))
Michael Blume that's still an invalid type hint. Clojure is inconsistent all over the place with enforcing valid type hints, so even though I think we should handle the VerifyError explicitly, I don't think we need to care about making that code work.
WRT ^boolean type hints going to be common because clojurescript uses them – I don't think we should accept invalid code for the sake of interoparability. See CLJ-1883 for an instance of such a wrong type hint being used in Om, the solution was to fix Om, not to make the clojure compiler ignore yet another wrong type hint.

I've just got around to trying to dig in to the data.xml failures I mentioned above.
Both those tests are related to head holding on a lazy seq. Both tests reliably fail with 0002-CLJ-701-add-HoistedMethod-to-the-compiler-for-hoisti.patch
So I suspect the hoisted method isn't properly clearing locals.
so I don't forget, I realized the issue with data.xml is because the 'is' macro in the test expands in to a try/catch in an expression context, which is hoisted, and the hoist causes the whole environment not to be cleared until the hoisted method returns, which is obviously not correct.
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
gets converted into
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