PersistentQueue objects are improperly eval'd/compiled

[Jon Distad]

I discovered while writing a data-reader for PersistentQueue that PersistentQueue objects do not follow the correct evaluation path in the Compiler.

The simplest case:

user=> (def q (conj clojure.lang.PersistentQueue/EMPTY 1 2 3))

#'user/q

user=> q

#<PersistentQueue clojure.lang.PersistentQueue@7861>

user=> (eval q)

CompilerException java.lang.ClassCastException: clojure.lang.PersistentQueue cannot be cast to java.util.List, compiling:(NO_SOURCE_PATH:4:1)

Turns out that exception is coming from the emitConstants function in ObjExpr. In the large if-statement in emitValue, PersistentQueue passes the `value instanceof IPersistentList` check (Compiler.java, line 4639), but does NOT implement java.util.List, and therefore throws an exception within emitListAsObjectArray.

However, even if it did implement List, it would still be incorrect because any PersistentQueue object that got passed to the compiler would be emitted as a PersistentList, which is still wrong.

Moreover, in the simple example above we should not even be entering emitConstants because eval'ing a non-syntax object should pass it straight through. Then we should be hitting the analyze(C.EVAL, form) call on line 6707, which should fall through to line 6459 (return new ConstantExpr(form)), and a ConstantExpr evals to the value it wraps.

But, once again, the PersistentQueue passes the `form instanceof IPersistentCollection && [form is not a def]` check on lines 6695-8, and ends up being compiled in a thunk, which causes it to be emitted as a constant, and ultimately throws the above exception.

Adding `&& !(value instanceof PersistentQueue)` to both the above predicates (lines 4639 and 6695-8) effectively resolves the issue. It treats PersistentQueue as any other object with no literal syntax, requiring definition of print-dup to embed in macros, etc. Even if print-dup is defined, it'll never be attempted as long as PersistentQueue is treated as special.

Clojure 1.6.0:

user=> (eval `(fn [] ~q))

CompilerException java.lang.ClassCastException: clojure.lang.PersistentQueue cannot be cast to java.util.List, compiling:(NO_SOURCE_PATH:2:1)

Clojure 1.7.0-master-SNAPSHOT (c9e70649d2652baf13b498c4c3ebb070118c4573) with the !PersistentQueue checks added:

user=> (eval `(fn [] ~q))

CompilerException java.lang.RuntimeException: Can't embed unreadable object in code: #<PersistentQueue clojure.lang.PersistentQueue@7861>, compiling:(NO_SOURCE_PATH:3:1)

which is what you would expect.

And the data-reader case, with the following setup:

user=> (defn make-queue [elems] (into clojure.lang.PersistentQueue/EMPTY elems))

#'user/make-queue

user=> (set! *data-readers* {'user/queue #'make-queue})

{user/queue #'user/make-queue}

user=> (defmethod print-method clojure.lang.PersistentQueue [q w] (.write w "#user/queue ")(print-method (into [] q) w))

#<MultiFn clojure.lang.MultiFn@275c9b76>

user=> (defmethod print-dup clojure.lang.PersistentQueue [q w] (print-method q w))

#<MultiFn clojure.lang.MultiFn@1d865aad>

Clojure 1.6.0:

user=> #user/queue [1 2]

CompilerException java.lang.ClassCastException: clojure.lang.PersistentQueue cannot be cast to java.util.List, compiling:(NO_SOURCE_PATH:0:0)

user=> (def q #user/queue [1 2 3]) ;; Works because def is special

#'user/q

user=> q

#user/queue [1 2 3]

user=> (eval q)

CompilerException java.lang.ClassCastException: clojure.lang.PersistentQueue cannot be cast to java.util.List, compiling:(NO_SOURCE_PATH:7:1)

user=> (eval `(fn [] ~q))

CompilerException java.lang.ClassCastException: clojure.lang.PersistentQueue cannot be cast to java.util.List, compiling:(NO_SOURCE_PATH:8:1)

Clojure 1.7.0-master-SNAPSHOT (c9e70649d2652baf13b498c4c3ebb070118c4573) with the !PersistentQueue checks:

user=> #user/queue [1 2]

#user/queue [1 2]

user=> (def q #user/queue [1 2 3])

#'user/q

user=> q

#user/queue [1 2 3]

user=> (eval q)

#user/queue [1 2 3]

user=> (eval `(fn [] ~q))

#<user$eval33$fn__34 user$eval33$fn__34@7bce20e8>

user=> (*1)

#user/queue [1 2 3]

The fix I'm proposing is not pretty, and I'm interested in suggestions. Given that PersistentQueue is the only (I think) internal persistent collection that is not treated specially by the compiler, I believe it may be acceptable to call it out explicitly. However this could also be a good opportunity to provide an IPersistentQueue interface, and possibly the Clojure-sanctioned #queue data reader people have been whispering about for years.

But this is definitely broken, and even if we don't expand the behavior this behavior should be fixed.

What do you think?

Jon

[Philip Potter]

CLJ-1059 is related to this: "PersistentQueue doesn't implement
java.util.List, causing nontransitive equality", though as you say
making PersistentQueue implement java.util.List won't solve the
problem.

> --
> You received this message because you are subscribed to the Google Groups
> "Clojure Dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to clojure-dev...@googlegroups.com.
> To post to this group, send email to cloju...@googlegroups.com.
> Visit this group at http://groups.google.com/group/clojure-dev.
> For more options, visit https://groups.google.com/d/optout.

[Jozef Wagner]

A couple of questions. Evaling a collection evals its items. Should we follow this idiom for queue too? How does c.c.eval handle custom types which implement IPersistentList?

Jozef

[Jon Distad]

Philip:

They are not actually much related, but I do agree that CLJ-1059 is a good idea. In this case, if PersistentQueue did implement java.util.List it would have only have masked the error.

Jozef:

I don't think we would want to eval a queue's elements. Since PersistentQueue is not a syntax-literal collection, then I think it would make sense to follow the behavior of records, which do not eval their attributes when eval'd.

I think any type which implements IPersistentList would fall into this compiler path and either throw an exception or end up emitted as a call to PersistentList.create(), losing the object's original type.

Jon

[Jon Distad]

I have opened CLJ-1492 (http://dev.clojure.org/jira/browse/CLJ-1492).

--
You received this message because you are subscribed to a topic in the Google Groups "Clojure Dev" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/clojure-dev/LDUQfqjFg9w/unsubscribe.
To unsubscribe from this group and all its topics, send an email to clojure-dev...@googlegroups.com.