Primitive return type of loop and try is lost

Question

(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

Answer 1

Comment made by: michaelblume

https://github.com/MichaelBlume/hoist-fail

I'd rather remove the dependency on compojure-api but I'm having trouble reproducing without it

Answer 2

Comment made by: michaelblume

Ok, less stupid test case:

`
(let [^boolean foo true]
(do

(try foo
  (catch Throwable t))
nil))    

`

Answer 3

Comment made by: michaelblume

...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.

Answer 4

Comment made by: bronsa

(link: ~michaelblume) 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])
is lying to the compiler telling it a is an unboxed boolean when infact it is a boxed Boolean.

I'd say we wait for what (link: ~alexmiller) or (link: ~richhickey) think of this before considering (link: ~hiredman)'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)

Answer 5

Comment made by: bronsa

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

`

Answer 6

Comment made by: hiredman

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

Answer 7

Comment made by: bronsa

Last patch doesn't compile anymore since it hits the bug reported in CLJ-1809

Answer 8

Comment made by: alexmiller

Moving to incomplete for now since it seems to be blocked on the other ticket

Answer 9

Comment made by: hiredman

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

Answer 10

Comment made by: michaelblume

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?

Answer 11

Comment made by: michaelblume

Also the latest patch seems to add a patch file to the root directory

Answer 12

Comment made by: michaelblume

New failing code

`

(defn foo [req]
(let [^boolean b (get req :is-baz)]

(do
  (try
    :bar)
  :foo)))

`

Answer 13

Comment made by: hiredman

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 :>))
`

Answer 14

Comment made by: bronsa

(link: ~michaelblume) 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.

Answer 15

Comment made by: hiredman

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.