Use notifications for panics

[alygin]

After this discussion I think something should be done with the way panics are handled now. The main concerns are:

1. Panics are run-time errors but Linter is designed to handle compile-time messages.
2. The upcoming Linter changes will not allow atom-build-cargo to make panics pointing to the Rust standard library easily accessible to the users, at least by default (the messages panel will be hidden). And such cases are not rare since .unwrap(), .expect() and other methods produce pointers to the Rust's code.

Given that, I think the better solution would be to show panics as error notifications (by means of atom.notifications.addError(...)):

1. The title shows the panic's message.
2. The detail block shows the link to the exact point in the source code that caused the panic. If such code is unreachable, the link is not active.
3. The detail block shows the stack backtrace if it's available (collapsed by default).
4. The notification is dismissable by the user.
5. Such notifications must be automatically closed before every build.
6. Maybe, some limit should be set on the quantity of notifications that might be shown at once to prevent cluttering when many tests are failed.

The result will be something like this:

I would even suggest Atom Build itself to have an API that would allow to produce such notifications (with source code links and expandable details) to impose a standard on the providers, but it seems that it might lead to another package misuse, since 'build' literally means 'build', not 'run', but cargo also runs and tests the code and I'm not aware of how other build providers use the Atom Build's API.

[oli-obk]

it seems that it might lead to another package misuse, since 'build' literally means 'build', not 'run', but cargo also runs and tests

well... for one, the builds can panic if there's a rustc bug

I'm generally in favor, but we need a way to figure out the significant span. There's no point in offering the user such a message if the span points into Option::unwrap in the stdlib source.

[alygin]

for one, the builds can panic if there's a rustc bug

Cargo delineates the compiler output from run-time messages by emitting something like Running target/..., so we can separate compile-time panics from the run-time ones and handle them differently if we want.

But I think we should use error notifications for all the panics and feed Linter with only common rustc's messages (errors, warnings, helps and notes) for which we always know the correct span.

we need a way to figure out the significant span.

Unfortunately, I can't come up with a general solution given the lack of information we have (the panic message and /sometimes/ the stack trace). But we could help the user to analyse the panic by making lines of the stack trace bold if they relate to our code (we can figure it out by our crate's name). For instance, in the example above we could make line 6 bold since we know that btest is our crate. Multi-crate projects will not be so easy to handle and more research needed to generalise this solution.

[alygin]

Some difficulties arose during prototyping:

1. The notification's detail area doesn't support HTML. So, no links or bold fonts can be placed there without hacks. Luckily, the notification title allows HTML and we can put file link there. Instead of bold font we can use some symbols to mark the lines of interest, I chose >> for the prototype.
2. The notification's detail area doesn't support showing stack trace without adding some text before it. Atom itself puts error message and the location into details, leaving the title for general description like 'Failed to activate some package'.
3. The notification's stack block is not officially documented, but Atom uses it internally and it seems safe enough to use.

So, now my prototype shows this notification (the arrow in the title will bring us to the source code line that caused the panic):

It's not ideal, but that's all I can do without hacking into Atom's internals. A couple of changes could be done to make this notification follow the Atom's attitude to such messages:

1. Move the panic's message to details area from the title.
2. Use some standard phrase in the title, like 'A thread panicked'.

But I don't like such changes much because they eat space without adding useful information in our case.

Any thoughts? Does it worth implementing from your point of view?

[alygin]

Another possible solution is to show the standard phrase plus verbose file link in the title and the panic message in details:

I would prefer this version because it's easier for the user to understand how to jump to the source code from this notification. Plus, a panic message with some html symbols will not break the title.

[oli-obk]

i like the latter version, too

[alygin]

I've got the following implementation:

1. All the panics are parsed and shown as error notifications along with their stack traces.
2. The title block shows a common phrase 'A thread panicked at line {line} in {file}'.
3. When possible (see below), the line number and file name are wrapped by a link to our source code. Otherwise, it's just a text.
4. The detail block shows the panic message.
5. The stack block shows the stack trace when it is available.
6. There's a limit on stack traces number to show at once. I set it to 10. When this limit is exceeded, the error notification 'Too many panics, some are hidden' is shown and no more panics are added (though, they are all parsed).
7. The source code links can work in multi-crate environments.

I try to extract the source code link from the panic itself and from the stack trace. For now, extracting from the stack trace only works on Linux in debug mode (see this comment).

Here're various examples:

What is absent:

1. Atom notifications API doesn't allow to close notifications programmatically, so the user has to do it manually.
2. It's possible to reformat a stack trace to make it more compact. Unnecessary spaces can be removed. It might be a separate issue.
3. I didn't implement the marker for the stack trace lines that relate to our code. It's not so easy when we work in a multi-crate environment. I think it might be a separate issue as well.

After the pending fix is merged I could rebase and create a PR.

[alygin]

The PR is ready. I improved the notification on hidden panics a bit, it is now either 'One more panic is hidden' or 'X more panics are hidden'.

Unfortunately, I have no proper Linux machine at hand, so I checked source code link extraction from stack traces synthetically. So if someone could check how it works in real world, that would be great: Just run in debug mode something like the snippet below and see if it can find the proper place in your code:

let a: Option<u32> = None;
a.expect("value is set");

[alygin]

I've tested it on Linux. It extracts code links from stack traces correctly. The only bug I encountered is fixed.

[alygin]

If nobody has any suggestions or objections, I think this issue might be closed and all further improvements might be done as separate issues.

[oli-obk]

Great