Welcome! Please see the About page for a little more info on how this works.

0 votes
in Clojure by

(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

47 Answers

0 votes
by

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

0 votes
by

Comment made by: michaelblume

Ok, less stupid test case:

`
(let [^boolean foo true]
(do

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

`

0 votes
by

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.

0 votes
by

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)

0 votes
by

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

`

0 votes
by

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

0 votes
by

Comment made by: bronsa

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

0 votes
by

Comment made by: alexmiller

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

0 votes
by

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

0 votes
by

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?

0 votes
by

Comment made by: michaelblume

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

0 votes
by

Comment made by: michaelblume

New failing code

`

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

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

`

0 votes
by

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

0 votes
by

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.

0 votes
by

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.

...