Clojure

top level conditional splicing ignores all but first element

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Completed
  • Affects Version/s: Release 1.7
  • Fix Version/s: Release 1.7
  • Component/s: None
  • Labels:
  • Patch:
    Code and Test
  • Approval:
    Ok

Description

user=> (def a (java.io.PushbackReader. (java.io.StringReader. "#?@(:clj [1 2])")))
#'user/a
user=> (read {:read-cond :allow} a)
1
user=> (read {:read-cond :allow} a)
RuntimeException EOF while reading  clojure.lang.Util.runtimeException (Util.java:221)

Currently the reader is stateless (read is a static call) but utilizes a stateful reader (and has a few hooks into compiler/runtime state for autoresolving keywords, etc). If the call into the reader at the top level calls a splicing reader conditional, then only the first one will be returned. The remaining forms are stranded in the pendingForms list and will be lost for subsequent reads.

Approach: Make top level reader conditional splicing an error:

user=> (read-string {:read-cond :allow} "#?@(:clj [1 2])")
IllegalStateException Reader conditional splicing not allowed at the top level.  clojure.lang.LispReader.read (LispReader.java:200)

Patch: clj-1706-2.patch

Alternatives:

1. Make top-level reader conditional splicing an error and throw an exception. (SELECTED)

2. Allow the caller to pass in a stateful collection to catch or return the pendingForms. This changes the effective calling API for the reader. You would only need to do this in the cases where reader conditionals were allowed/preserved.

3. Add a static (or threadlocal?) pendingForms attribute to the reader to capture the pendingForms across calls. A static field would have concurrency issues - anyone using the reader across threads would get cross-talk in this field. The pendingForms could be threadlocal which would probably achieve separation in the majority of cases, but also creates a number of lifecycle questions about those forms. When do they get cleared or reset? What happens if reading the same reader happens across threads? Another option would be an identity map keyed by reader instance - would need to be careful about lifecycle management and clean up, as it's basically a cache.

4. Add more state into the reader itself to capture the pendingForms. The reader interfaces and hierarchy would be affected. This would allow the reader to stop passing the pendingForms around inside but modifies the interface in other ways. Again, this would only be needed for the specific case where reader conditionals are allowed so other uses could continue to work as is?

5. If read is going to exit with pendingForms on the stack, they could be printed and pushed back onto the reader. This adds new read/print roundtrip requirements on things at the top level of reader conditionals that didn't exist before.

6. Wrap spliced forms at the top level in a `do`. This seems to violate the intention of splicing reader conditional to read as spliced since it is not the same as if those forms were placed separately in the input stream.

  1. 0001-CLJ-1706-Make-top-level-reader-conditional-splicing-.patch
    19/May/15 11:59 AM
    4 kB
    Nicola Mometto
  2. clj-1706-2.patch
    19/May/15 11:21 AM
    11 kB
    Alex Miller
  3. clj-1706-3.patch
    19/May/15 2:54 PM
    4 kB
    Alex Miller
  4. clj-1706-make-error.patch
    18/May/15 8:29 AM
    2 kB
    Alex Miller

Activity

Hide
Alex Miller added a comment -

pulling into 1.7 so we can discuss

Show
Alex Miller added a comment - pulling into 1.7 so we can discuss
Hide
Alex Miller added a comment -

Compiler.load() makes calls into LispReader.read() (static call). If the reader reads a top-level splicing conditional read form, that will read the entire form, then return the first spliced element. when LispReader.read() returns, the list carrying the other pending forms is lost.

I see two options:

1) Allow the compiler to call the LispReader with a mutable pendingForms list, basically maintaining that state across the static calls from outside the reader. makes the calling model more complicated and exposes the internal details of the pendingform stuff, but is probably the smaller change.

2) Enhance the LineNumberingPushbackReader in some way to remember the pending forms. This would probably allow us to remove the pending form stuff carried around throughout the LispReader and retain the existing (sensible) api. Much bigger change but probably better direction.

Show
Alex Miller added a comment - Compiler.load() makes calls into LispReader.read() (static call). If the reader reads a top-level splicing conditional read form, that will read the entire form, then return the first spliced element. when LispReader.read() returns, the list carrying the other pending forms is lost. I see two options: 1) Allow the compiler to call the LispReader with a mutable pendingForms list, basically maintaining that state across the static calls from outside the reader. makes the calling model more complicated and exposes the internal details of the pendingform stuff, but is probably the smaller change. 2) Enhance the LineNumberingPushbackReader in some way to remember the pending forms. This would probably allow us to remove the pending form stuff carried around throughout the LispReader and retain the existing (sensible) api. Much bigger change but probably better direction.
Hide
Nicola Mometto added a comment -

What about simply disallowing cond-splicing when top level?
Both proposed options are breaking changes since read currently only requires a java.io.PushbackReader

Show
Nicola Mometto added a comment - What about simply disallowing cond-splicing when top level? Both proposed options are breaking changes since read currently only requires a java.io.PushbackReader
Hide
Alex Miller added a comment -

We want to allow top-level cond-splicing.

Show
Alex Miller added a comment - We want to allow top-level cond-splicing.
Hide
Nicola Mometto added a comment - - edited

Would automatically wrapping a top-level cond-splicing in a (do ..) form be out of the question?

I'm personally opposed to supporting this feature as it would change the contract of c.c/read, complicate the implementation of LineNumberingPushbackReader or LispReader and complicate significantly the implementaion of tools.reader's reader types, for no significant benefit.
Is it really that important to be able to write

#~@(:clj [1 2])

rather than

(do #~@(:clj [1 2]))

?

Show
Nicola Mometto added a comment - - edited Would automatically wrapping a top-level cond-splicing in a (do ..) form be out of the question? I'm personally opposed to supporting this feature as it would change the contract of c.c/read, complicate the implementation of LineNumberingPushbackReader or LispReader and complicate significantly the implementaion of tools.reader's reader types, for no significant benefit. Is it really that important to be able to write
#~@(:clj [1 2])
rather than
(do #~@(:clj [1 2]))
?
Hide
Rich Hickey added a comment -

Please "Make top-level reader conditional splicing an error and throw an exception" for now.

Show
Rich Hickey added a comment - Please "Make top-level reader conditional splicing an error and throw an exception" for now.
Hide
Nicola Mometto added a comment -

Might be too late since Rich already gave the OK but the proposed patch doesn't prevent single-element top level conditional splicing forms.
e.g

;; this fails
#~@(:clj [1 2])
;; this works
#~@(:clj [1])

Is this intended?

Show
Nicola Mometto added a comment - Might be too late since Rich already gave the OK but the proposed patch doesn't prevent single-element top level conditional splicing forms. e.g
;; this fails
#~@(:clj [1 2])
;; this works
#~@(:clj [1])
Is this intended?
Hide
Alex Miller added a comment -

New -2 patch catches reader conditional splice of 0 or 1 element.

Show
Alex Miller added a comment - New -2 patch catches reader conditional splice of 0 or 1 element.
Hide
Nicola Mometto added a comment -

Attached alternative patch that is less intrusive than clj-1706-2.patch

Show
Nicola Mometto added a comment - Attached alternative patch that is less intrusive than clj-1706-2.patch
Hide
Alex Miller added a comment -

clj-1706-3.patch is identical to 0001-CLJ-1706Make-top-level-reader-conditional-splicing.patch but with one whitespace change reverted. Marking latest as screened.

Show
Alex Miller added a comment - clj-1706-3.patch is identical to 0001-CLJ-1706Make-top-level-reader-conditional-splicing.patch but with one whitespace change reverted. Marking latest as screened.
Hide
Alex Miller added a comment -

Rich didn't like the dynvar in -3, so switching back to -2.

Show
Alex Miller added a comment - Rich didn't like the dynvar in -3, so switching back to -2.

People

Vote (2)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: