search

jasongilman / proto-repl

A Clojure Development Environment package for the Atom editor
https://atom.io/packages/proto-repl
MIT License
563 stars 50 forks source link

handling exceptions #141

Open carocad opened 8 years ago

carocad commented 8 years ago

Hi jason, I was taking a look at what we would need to do to make handling of exceptions possible in proto-repl and here are the points that I think are necessary to implement such behaviour.

*_the reason why I decided to display err as stderr and not as an exception message is that I found out that proto-repl uses a truncated err messages. For console.stderr that is ok as they are merely text but if you want to put html/js inside then the truncated messages don't work anymore.

\ try calling (clojure.repl/pst) on an editor (not the console). You will see the editor flicker for a brief moment due to it rendering initialy err as the inline-result and then getting 'nil' as result.value, thus overwriting the previous value. I guess it is possible to write to stderr without having a real exception so those messages that do not match should probably go to console.stderr directly

*\ There are several ways to display stacktraces in Clojure so supporting all of them if probably too much initially but at least clojure.repl | stacktrace should be enough to begin with.

**\ I'm not completely sure if the highlighting + file-hyperlink would work directly on the console, but I see no reason to believe otherwise.

I would appreciate your thoughts on the subject

jasongilman commented 8 years ago

Hi Camilo,

I wrote up the following. It's a bit long and rambling but I wanted to capture all my thoughts about exceptions. Let me know if I missed anything in your questions.

That all sounds good to me. I just went to the Atom ink README and watched this gif which shows the error handling in action https://raw.githubusercontent.com/JunoLab/atom-ink/master/demos/errors.gif

There are a few nice features that I see:

Of all of those the last one seems the least useful to me. I'm not against putting it in though if you find it useful. The first two are the most useful to me. Another thing that would be really nice is if the stack traces in the REPL output were clickable. Sometimes stack traces are directed to standard out like in test results. Maybe we could detect anything that looks like part of a stack trace in standard output and make it clickable. I'm ok with doing a simple approach at first though and improving it later.

It might not make sense to try to parse exception information from strings. If we can get access to the Clojure exception object we can use a library like clj-stacktrace to return the file information. The problem with that is we won't be able to handle cases where the output of an exception is from a logging framework that went to standard output or when running tests. So I guess I'm on the side of manually parsing it from strings so that we can support the most usecases.

I found this gist containing examples of stacktraces output by different tools. https://gist.github.com/mmcgrana/649282 It would be nice if whatever code we write for analyzing stack traces could be expanded in the future to allow for different stack trace dialects. Also, I encourage you to write that portion of those code in the ClojureScript section of Proto REPL. I can help with how to run code there.

We could probably detect that a file has a stacktrace element in it if it matches the regex \([A-Za-z0-9_-]+\.[A-Za-z0-9_-]+:\d+\) To break that down into bnf: "(" <filename_without_extension> "." <extension> ":" <linenumber> ")"

This is output from a test that had an exception.

Testing proto-repl-demo.demo-test

ERROR in (add-numbers-test) (Numbers.java:158)
Add two numbers
expected: (= 5 (d/add-numbers 2 3))
  actual: java.lang.ArithmeticException: Divide by zero
 at clojure.lang.Numbers.divide (Numbers.java:158)
    clojure.lang.Numbers.divide (Numbers.java:3808)
    proto_repl_demo.demo$add_numbers.invokeStatic (demo.clj:6)
    proto_repl_demo.demo$add_numbers.invoke (demo.clj:3)
    proto_repl_demo.demo_test$fn__3197.invokeStatic (demo_test.clj:7)
    proto_repl_demo.demo_test/fn (demo_test.clj:5)
    clojure.test$test_var$fn__7983.invoke (test.clj:716)
    clojure.test$test_var.invokeStatic (test.clj:716)
    clojure.test$test_var.invoke (test.clj:707)
    clojure.test$test_vars$fn__8005$fn__8010.invoke (test.clj:734)

Finding the location of the file could be a little tricky from proto_repl_demo.demo$add_numbers.invokeStatic (demo.clj:6) There could be multiple files called "demo.clj"on the classpath.

The first part before the dollar sign tells you that there will be a file on the classpath with the name `proto_repl_demo/demo.clj"

carocad commented 8 years ago

Hi Jason, Yes we are totally thinking about the same things :)

In my experience, there are 3 ways to handle exceptions from an IDE point:

  1. wrap everything in a try/catch block and call clojure.repl/pst (or similar library) on catch.
  2. if you detect a message from the nrepl with a eval-error, send another message requesting the last exception (.printStackTrace *e)
  3. let the user decide what to do with their exception but try to parse and output them as such.

Disadvantages:

  1. you must "marry" a specific exception handling library and force the users to put it on their dependencies if it is not included by clojure (clj-stacktrace, aviso/pretty, etc).
  2. This falls back to the least-common-denominator. those exceptions are ugly but they will always work.
  3. You will have to keep updating the regex for changes in output from the respective libraries.

Of those, I think that option 2 and 3 are the less problematic. Option 2 is probably the easiest and most maintainable of those but option 3 is the most user friendly.

Also, I encourage you to write that portion of those code in the ClojureScript section of Proto REPL. I can help with how to run code there.

I am not sure if I get that right but I don't think that would be a good idea. To make the highlighting and clickable exceptions I would need to interface with the atom editor directly. I guess it would be easier to just make the regex on js and if it matches start the linking/highlighting process with the respective files.

Expandable stack trace from an error Error messages displayed in red so they stand out.

I agree on these two points but if we chose option 3 (which is what I understand we are implicitely doing) then we would only be able to do those things on the console as we would be converting stderr to an html/js object. So in order to avoid that information getting lost by overwriting it with a normal result, I think the best option is to just put it on the console.

carocad commented 8 years ago

@jasongilman I found a problem in my previous 'design'. As the ink-team pointed out, we would have quite some troubles (from the user experience side) if we display exceptions on the console with highlighting (see ink-team comment)

The problem being: when to show/hide the highlighting? if the console is cleared, do we destroy the highlighting? How do we know which code caused the exception? the call to pst is unrelated to the one that caused the exception :(

I think we have two options to solve this:

*_right conditions:

I think that if those conditions are met we can be (mostly) sure that the user called something like (clojure.repl/pst). In any other case, we would just send it to stderr as expected.

jasongilman commented 8 years ago

Is "highlighting" the highlighting of lines in red that were involved in an exception? If so I don't think it would be really useful since a lot of the stack trace is scattered throughout files.

What I want is to quickly go from any displayed stack trace to the line of code in a file related to it. I want to be able to click on a line of a stack trace and have it open the relevant file and line.

You've mentioned a few times that you want to direct it to std err. Why do you want to do that? Is that how we have to hook it up to ink?

What I'm saying is that I don't think we need to do that. We can match on every single output line with a fast regex to see if it's possibly a stack trace. once we know something is a stack trace we extract the info and make the displayed data link that will trigger the opening of the correct file.

What do you think?

jasongilman commented 8 years ago

I reread your earlier comment and I think you already understand my approach and the trade offs, especially the part about different exception libraries.

It might not be that big of a problem. Almost all of them are very similar and my guess is that they don't change the format very often. It will still probably happen sometimes though. I'm not sure of a better solution though if we want to be able to handle stack traces that come randomly via std out.

carocad commented 8 years ago

yes highlighting is putting a red line over the lines that were involved in an exception. But it is not only about the current file where the exception happened but it is also possible to make the file-links contain js code that would highlight the lines in other files that were involved in the exceptions once those files are opened.

That's why I'm so reluctant to let that go. I think that it would immensely help users to clearly identify which line in which files were involved in an exception. Notice how in this gif from atom ink, the exception happened in a file called 'untitled' and when he clicks it, a new file opens ('rationals.jl') with the source code and red highlighting. That's what I'm aiming for.

I will go with option 2 from my last comment. If I break something or you don't think it is worth the trouble, you can always reject the PR ;)

jasongilman commented 8 years ago

Thanks for continuing to work on this. Nice job so far. It's great that you have the linking working. Here's some questions based on this diff: https://github.com/jasongilman/proto-repl/compare/master...carocad:exception-handling

  1. What's the purpose lib/views/value/value.coffee? I see some copy and pasted code there from the REPL. Is this other code copied from ink? We can probably refactor some of the inline display / tree code out of REPL to be used more easily.
  2. You should put a file level comment at the top of each file to explain their purpose.
  3. Why did you move load-widget.coffee under /views/value? What's the significance of "value"?
  4. Why are some existing references to result.error changed to result.err?
carocad commented 8 years ago

Hi Jason, I think I should explain a bit the background better since the changes are so many.

As I mentioned before I wanted to separate stderr from exceptions and have a symmetric way to handle result; for example if you eval and print something, you get a value back and messages to stdout. So I wanted to extend that logic to errors. Eval something that fails, get an exception back and messages to stderr.

That was my initial thought, but when I did that I found out that lots of other things were failing due to some ResultHandler() functions expecting result.error, that's when I decided to almost switch things back but since the messages from the repl already contain all the information necessary for us to process it I thought it would be simpler to simply pass the message back instead of creating new objects that copy/past the information that we need.

Now to your questions:

  1. The purpose of value.coffee is to render anything that will displayed on an editor. I didn't want to use the word Result to avoid confusion with ink's result and with a 'correct' result. "Value" is arguably not a good name either as I actually import it as 'Renderer' on the repl.coffee file:https://github.com/jasongilman/proto-repl/compare/master...carocad:exception-handling#diff-55de1e9e7ce745ded36b006d65a27a0bR4. None of the code is copied from ink though. The main idea of value.coffee is precisely to simplify and encapsulate all inline-result related functionality. That's the reason why I moved recurseTree to value.coffee. There were so many special/particular conditions for exceptions that I thought that it would be better to avoid having that code on the repl. I actually decided to use atom-ink's tree view instead of proto-repl precisely to allow some refactor of the treeView code on proto-repl without interfeering with that of exceptions.
  2. Yes definitely. I will do that
  3. I wanted to have all result related functionality at a simple logical place. As I said, 'value' is probably not the best name but I think that the loading widget is more related to inline-results rendering that to proto-repl.coffee, repl.coffee and so on. This change is actually not related to the exception handling so I could revert it if you want.
  4. I wanted to simplify the way that the resultHandler() and handleConnectionMessage() get their values and since the messages from the repl already contain specific keys (err, out, value & ex) that convey their meaning I thought it would be better to return the already existing messages instead of creating yet another object with a custom key. Before both handleConnectionMessage() and resultHandler() used different keys for errors: connectionMessage() used result.err whereas resultHandler() used result.error which was quite confusing for me at the beginning. Since both functions are now used at the connection.send() command I thought it would be clearer.
jasongilman commented 8 years ago

@carocad Thanks for the explanation. I don't completely get the stderr change yet. I figure at some point you'll declare it's done and I'll start digging in to see how everything works in detail.

I'd like to get rid of the "value" name since we both agree it's not specific enough. I like the idea of pulling out view related things. I don't think it makes sense yet to have another subfolder under views. i would make everything direct children of that for now. I would rename value.coffee to inline-display.coffee or something like that.

I like the answer for number 4. That sounds good to me.