ClojureScript

REPL doesn't give error for expressions with too many right parentheses.

Details

  • Type: Defect Defect
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Completed
  • Affects Version/s: 1.7.228
  • Fix Version/s: None
  • Component/s: None
  • Labels:
  • Environment:
    Fedora 23, java version "1.8.0_40", javac 1.8.0_40, clojure 1.7.0
  • Patch:
    Code

Description

I was expecting an error message from this; using [org.clojure/clojurescript "1.7.228"]; the Clojure REPL does produce an error.

To quit, type: :cljs/quit
cljs.user=> (+ 3 5)))))
8

Activity

David Nolen made changes -
Field Original Value New Value
Description
I was expecting an error message from this; using [org.clojure/clojurescript "1.7.228"]; the Clojure REPL does produce an error.

[david@localhost ~]$ mkdir testdir
[david@localhost ~]$ cd testdir
[david@localhost testdir]$ lein new figwheel repltest
Generating fresh 'lein new' figwheel project.
[david@localhost testdir]$ cd repltest
[david@localhost repltest]$ lein figwheel
Figwheel: Starting server at http://localhost:3449
Figwheel: Watching build - dev
Compiling "resources/public/js/compiled/repltest.js" from ["src"]...
Successfully compiled "resources/public/js/compiled/repltest.js" in 19.61 seconds.
;; snip help text
Prompt will show when Figwheel connects to your application
To quit, type: :cljs/quit
cljs.user=> (+ 3 5)))))
8
I was expecting an error message from this; using [org.clojure/clojurescript "1.7.228"]; the Clojure REPL does produce an error.

{code}
To quit, type: :cljs/quit
cljs.user=> (+ 3 5)))))
8
{code}
Hide
Mike Fikes added a comment -

A suggestion on a strategy to fix this: Make the ClojureScript REPL sequentially process all of the forms it can read on a line, just like the Clojure REPL does:

user=> 3 (+ 3 5) 7
3
8
7

If this is done, then the fix for this ticket will fall out “for free” and the ClojureScript REPL will error when it hits a form that appears to start with ).

Show
Mike Fikes added a comment - A suggestion on a strategy to fix this: Make the ClojureScript REPL sequentially process all of the forms it can read on a line, just like the Clojure REPL does:
user=> 3 (+ 3 5) 7
3
8
7
If this is done, then the fix for this ticket will fall out “for free” and the ClojureScript REPL will error when it hits a form that appears to start with ).
Hide
Mike Fikes added a comment -

The REPL code is very close to working the way mentioned in the previous comment. It currently does not only because this line

https://github.com/clojure/clojurescript/blob/c59e957f6230c07e7a228070dd8eb393d5b8ce40/src/main/clojure/cljs/repl.cljc#L100

invokes code that causes a new PushbackReader to wrap things (discarding things):

https://github.com/clojure/clojurescript/blob/c59e957f6230c07e7a228070dd8eb393d5b8ce40/src/main/clojure/cljs/repl.cljc#L773-L775

If you either let the PushbackReader once and let that reader fn close over it, or otherwise comment out things so that a new PushbackReader is not created for each loop / recur, you will see that the code behaves as suggested in the previous comment, having the desired effect.

The only thing I see that would need to be additionally sorted out with such a patch is being a little more clever about when need-prompt evaluates to true, etc. (otherwise polishing thing so there are no missed corner cases).

Show
Mike Fikes added a comment - The REPL code is very close to working the way mentioned in the previous comment. It currently does not only because this line https://github.com/clojure/clojurescript/blob/c59e957f6230c07e7a228070dd8eb393d5b8ce40/src/main/clojure/cljs/repl.cljc#L100 invokes code that causes a new PushbackReader to wrap things (discarding things): https://github.com/clojure/clojurescript/blob/c59e957f6230c07e7a228070dd8eb393d5b8ce40/src/main/clojure/cljs/repl.cljc#L773-L775 If you either let the PushbackReader once and let that reader fn close over it, or otherwise comment out things so that a new PushbackReader is not created for each loop / recur, you will see that the code behaves as suggested in the previous comment, having the desired effect. The only thing I see that would need to be additionally sorted out with such a patch is being a little more clever about when need-prompt evaluates to true, etc. (otherwise polishing thing so there are no missed corner cases).
Hide
Mike Fikes added a comment -

Attached a patch that, in essence makes the ClojureScript REPL behave like the Clojure REPL with respect to multiple items on a line and with respect to detecting malformed input. The patch is fairly straightforward, but could use some testing. I've tried things like

cljs.user=> 3_    ; where _ here is a space

cljs.user=> 3 4 5

cljs.user=> 3)

cljs.user=> 3))

cljs.user=> 3 [4
5]

cljs.user=> (let [x 1]
(+ 1 "a"))         ;; testing to make sure line numbers are right

All the above is looking good to me.

Here is the commit comment:

Take extra care to preserve the state of in so that anything beyond
the first form remains for reading. This fundamentally makes the
ClojureScript REPL behave like the Clojure REPL. In particular, it
allows entering multiple forms on a single line (which will be evaluated
serially). It also means that if malformed input lies beyond the initial
form, it will be read and will cause an exception (just like in the
Clojure REPL).

The bulk of the complexity in this commit has to do with the case where
a new line-numbering reader is established, so that errors in forms
can be associated with line numbers, starting with line 1 being the
first line of the form. This requires a little extra handling because
the source-logging-push-back-reader introduces an extra 1-character
buffer which must be transferred back to the original (pre-bound) in,
otherwise things like an unmatched extra paren right after a well-formed
form won't be detected (as the paren would be in the 1-char buffer and
discarded.)

Also, a Java PushbackReader needs to be eliminated, as it causes things
to fail to behave like the Clojure REPL.

Show
Mike Fikes added a comment - Attached a patch that, in essence makes the ClojureScript REPL behave like the Clojure REPL with respect to multiple items on a line and with respect to detecting malformed input. The patch is fairly straightforward, but could use some testing. I've tried things like
cljs.user=> 3_    ; where _ here is a space

cljs.user=> 3 4 5

cljs.user=> 3)

cljs.user=> 3))

cljs.user=> 3 [4
5]

cljs.user=> (let [x 1]
(+ 1 "a"))         ;; testing to make sure line numbers are right
All the above is looking good to me. Here is the commit comment: Take extra care to preserve the state of in so that anything beyond the first form remains for reading. This fundamentally makes the ClojureScript REPL behave like the Clojure REPL. In particular, it allows entering multiple forms on a single line (which will be evaluated serially). It also means that if malformed input lies beyond the initial form, it will be read and will cause an exception (just like in the Clojure REPL). The bulk of the complexity in this commit has to do with the case where a new line-numbering reader is established, so that errors in forms can be associated with line numbers, starting with line 1 being the first line of the form. This requires a little extra handling because the source-logging-push-back-reader introduces an extra 1-character buffer which must be transferred back to the original (pre-bound) in, otherwise things like an unmatched extra paren right after a well-formed form won't be detected (as the paren would be in the 1-char buffer and discarded.) Also, a Java PushbackReader needs to be eliminated, as it causes things to fail to behave like the Clojure REPL.
Mike Fikes made changes -
Assignee Mike Fikes [ mfikes ]
Attachment CLJS-1572.patch [ 15498 ]
Hide
Mike Fikes added a comment -

Note that one extremely useful thing this patch enables is pasting of multiple forms into a ClojureScript REPL!

This fails if pasted using the current cljs.jar, but works with the patch applied:

(def a 1)

(def b 2)

(def c (+ a b))

c
Show
Mike Fikes added a comment - Note that one extremely useful thing this patch enables is pasting of multiple forms into a ClojureScript REPL! This fails if pasted using the current cljs.jar, but works with the patch applied:
(def a 1)

(def b 2)

(def c (+ a b))

c
Mike Fikes made changes -
Patch Code [ 10001 ]
David Nolen made changes -
Assignee Mike Fikes [ mfikes ] David Nolen [ dnolen ]
Hide
Mike Fikes added a comment -

I tested this with Figwheel [figwheel-sidecar "0.5.0-6"] and it worked properly evaluating multiple forms on a single line, evaluating pasted forms (as in the previous comment), and it properly indicates Unmatched delimiter ) for the case in the description.

Show
Mike Fikes added a comment - I tested this with Figwheel [figwheel-sidecar "0.5.0-6"] and it worked properly evaluating multiple forms on a single line, evaluating pasted forms (as in the previous comment), and it properly indicates Unmatched delimiter ) for the case in the description.
David Nolen made changes -
Resolution Completed [ 1 ]
Status Open [ 1 ] Resolved [ 5 ]
David Nolen made changes -
Status Resolved [ 5 ] Closed [ 6 ]
Hide
Mike Fikes added a comment -

Confirmed that this change works downstream with Figwheel (currently at 0.5.10).

Show
Mike Fikes added a comment - Confirmed that this change works downstream with Figwheel (currently at 0.5.10).
Hide
Antonin Hildebrand added a comment -

Just for record, I had to return to previous implementation in Dirac's REPL:
https://github.com/binaryage/dirac/commit/8f50cd17744f33c0f7258ca116e01b9b41243f14

I was unable to track the exact issue down. It has probably something to do with using :reader in repl-opts, which then creates inconsistency between in and current-in (not sure if bind-in? is true in my case).

Also I wonder if nil case should not be properly handled here:
https://github.com/clojure/clojurescript/commit/dfadee51fa3fad58b7c4cf7de532e9a10e0f802f#diff-37f2c970502705d61a0ab1f75ce8fe12R107

And also what if (readers/read-char in) returns nil here?:
https://github.com/clojure/clojurescript/commit/dfadee51fa3fad58b7c4cf7de532e9a10e0f802f#diff-37f2c970502705d61a0ab1f75ce8fe12R109

Show
Antonin Hildebrand added a comment - Just for record, I had to return to previous implementation in Dirac's REPL: https://github.com/binaryage/dirac/commit/8f50cd17744f33c0f7258ca116e01b9b41243f14 I was unable to track the exact issue down. It has probably something to do with using :reader in repl-opts, which then creates inconsistency between in and current-in (not sure if bind-in? is true in my case). Also I wonder if nil case should not be properly handled here: https://github.com/clojure/clojurescript/commit/dfadee51fa3fad58b7c4cf7de532e9a10e0f802f#diff-37f2c970502705d61a0ab1f75ce8fe12R107 And also what if (readers/read-char in) returns nil here?: https://github.com/clojure/clojurescript/commit/dfadee51fa3fad58b7c4cf7de532e9a10e0f802f#diff-37f2c970502705d61a0ab1f75ce8fe12R109

People

Vote (2)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: