Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Keep REPL running on runtime errors #2817

Merged
merged 1 commit into from Mar 13, 2013

Conversation

Projects
None yet
4 participants
Contributor

epidemian commented Mar 12, 2013

I don't know if there's an open issue for this, but basically this patch makes the REPL keep running after runtime errors instead of terminating (which i've found quite annoying a couple of times).

Collaborator

vendethiel commented Mar 12, 2013

I think this was changed for a reason. I can't find @jashkenas 's commit again tho :(

Contributor

epidemian commented Mar 12, 2013

@jashkenas, if you plan on merging this, please merge the test cases for #2814 first.

Contributor

epidemian commented Mar 12, 2013

@nami-doc, hmm... OK, i'll see if i can find it; maybe that reason no longer exists =P. Redux does something similar though

Contributor

epidemian commented Mar 12, 2013

@nami-doc, this is the commit that moves the vm.runInContext out of the try: be9707f, but it doesn't explain why it does so. Maybe it was an oversight.

Collaborator

michaelficarra commented Mar 12, 2013

@epidemian: It was an accident. It was fixed some time after porting it to CSR. I could've sworn we already fixed this in jashkenas/coffee-script, though I can't find it. Rebase and I'll merge.

Contributor

epidemian commented Mar 12, 2013

@michaelficarra, rebased.

@michaelficarra michaelficarra commented on the diff Mar 12, 2013

test/repl.coffee
@@ -91,3 +91,9 @@ testRepl "existential assignment of previously declared variable", (input, outpu
input.emitLine 'a = null'
input.emitLine 'a ?= 42'
eq '42', output.lastWrite()
+
+testRepl "keeps running after runtime error", (input, output) ->
+ input.emitLine 'a = b'
+ eq 0, output.lastWrite().indexOf 'ReferenceError: b is not defined'
+ input.emitLine 'a'
+ eq 'undefined', output.lastWrite()
@michaelficarra

michaelficarra Mar 12, 2013

Collaborator

Shouldn't this one be an error as well? I would expect ReferenceError: a is not defined.

@epidemian

epidemian Mar 12, 2013

Contributor

Shouldn't have used such a corner-case there 😟

I'd say it shouldn't raise an error on the second line. Basically, what's happening is that the first line executes:

var a;
a = b;

The assignment fails, but the variable a has already been declared, so then accessing it afterwards is legal.

Same thing happens in the Node REPL (and the FF one) even if you declare the variable and assign it in the same line:

> var a = b
ReferenceError: b is not defined
> a
undefined

So i would assume it's standard JS behaviour that when you do var identifier = expr the variable declaration is done first, then the evaluation of expr and finally the assignment (don't quote me on that though =P).

But anyway, that's not what this test should be testing. Maybe we should change the second line to something totally unrelated to the first one, like "i'm alive", just to test that the REPL is still alive at that point (in the current master, the runtime error bubbles all the way up, so the second emitLine is never executed).

@michaelficarra

michaelficarra Mar 12, 2013

Collaborator

Each entry is atomic in my REPL, and that's how I would want it to behave:

$ bin/coffee
coffee> a = 0
0
coffee> a = 1; throw new Error
Error:
coffee> a
0
coffee>
$

Unfortunately, node doesn't work like that...

$ node
> var a = 0
undefined
> a = 1; throw new Error
Error
    at repl:1:14
    at REPLServer.self.eval (repl.js:109:21)
    at rli.on.self.bufferedCmd (repl.js:258:20)
    at REPLServer.self.eval (repl.js:116:5)
    at Interface.<anonymous> (repl.js:248:12)
    at Interface.EventEmitter.emit (events.js:96:17)
    at Interface._onLine (readline.js:200:10)
    at Interface._line (readline.js:518:8)
    at Interface._ttyWrite (readline.js:736:14)
    at ReadStream.onkeypress (readline.js:97:10)
> a
1
>

I used these tests in CSR: https://github.com/michaelficarra/CoffeeScriptRedux/blob/c2b2f4d671af235ec22ff78c1266e5e6189daaf1/test/repl.coffee#L94-L100

@epidemian

epidemian Mar 12, 2013

Contributor

@michaelficarra, how do you achieve that atomicity? I can't find anything in Redux's repl.coffee that would seem to do the trick.

I personally don't find it very intuitive TBH; i prefer Node's behaviour. And the atomicity on Redux's REPL seems to be a bit leaky:

$ bin/coffee # On Redux
coffee> a = 1; throw new Error
Error: 
coffee> a
1
coffee> # And also...
coffee> b = {foo: 1}
{ foo: 1 }
coffee> b.foo = 2; throw new Error
Error: 
coffee> b
{ foo: 2 }

jashkenas added a commit that referenced this pull request Mar 13, 2013

Merge pull request #2817 from epidemian/keep-repl-running
Keep REPL running on runtime errors

@jashkenas jashkenas merged commit 56413ba into jashkenas:master Mar 13, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment