Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the ability to explain error at point #1122

Merged
merged 1 commit into from Oct 7, 2016

Conversation

@fmdkdd
Copy link
Member

commented Sep 29, 2016

Following the discussion in flycheck/flycheck-rust#35, @lunaryorn was open to add an "explain error at point" functionality to flycheck. This pull request is a sketch implementation for that feature.

Background

In rust-lang, the compiler can produce clear (and lengthy!) explanations for error messages. For instance:

This error occurs when an attempt is made to use a variable after its contents
have been moved elsewhere. For example:

struct MyStruct { s: u32 }

fn main() {
    let mut x = MyStruct{ s: 5u32 };
    let y = x;
    x.s = 6;
    println!("{}", x.s);
}

Since MyStruct is a type that is not marked Copy, the data gets moved out
of x when we set y. This is fundamental to Rust's ownership system: outside
of workarounds like Rc, a value cannot be owned by more than one variable.
[...]

These explanations are quite useful, especially for beginners, and we should encourage users to look them up. By default, the rust compiler will emit the following message when there are errors with explanations:

run `rustc --explain E0499` to see a detailed explanation

We used to have this message as well in the echo area and the error list, but not since we switched to the new JSON parser in #1036, since this specific line is not part of the JSON output.

To remedy that, I added a function to display the explanation for the error under point in an help buffer, in flycheck/flycheck-rust#30. But I don't think many rust users noticed, unless they looked at the source or keep in touch with our pull requests.

In flycheck/flycheck-rust#35 I proposed to add a default keybinding to make this function more discoverable, and @lunaryorn suggested to make the functionality generic, hence this PR.

Sketch implementation

This commit adds a new optional checker property, error-explainer, which should be a function that returns the explanation (as a string) for a given error (a flycheck-error object).

It also adds an interactive function flycheck-explain-error-at-point (bound to C-c ! r by default), which gets the error under point, calls the error-explainer (if any) to get an explanation string, then calls a new function flycheck-display-error-explanation to open an help buffer containing the explanation string.

Discussion points

I open the PR now so that we can discuss the merits of the feature, as well as the implementation strategy. It already works for Rust as is, but please overlook the specifics of the code yet, as much of the design isn't settled yet.

Here are some points I would like some feedback on:

  1. Are there any other checkers that could use this functionality? If not, then maybe staying with the explain-error function in flycheck-rust is best to avoid technical debt.
  2. At the moment, the code will only try to explain the first error under point. There could of course be multiple errors under point, so how do we choose which error to explain? As a first step, we could try to call the explainer on all the errors under point, and return the first explanation that comes. I don't have an example in rust where different explanations appear under point, but with multiple checkers enabled and multiple explainer functions, this could be an usability issue.
  3. Should this functionality also be present in the error list? It seems to me it should, and there wouldn't be any of the above ambiguity in the list.
flycheck.el Outdated
@@ -834,6 +834,7 @@ This variable is a normal hook. See Info node `(elisp)Hooks'."
(define-key map "h" #'flycheck-display-error-at-point)
(define-key map "H" #'display-local-help)
(define-key map "i" #'flycheck-manual)
(define-key map "r" #'flycheck-explain-error-at-point)

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Oct 1, 2016

Contributor

We can use x here and drop the original binding for C-c ! x, in my opinion. I think explain is much more useful so should get a better binding.

flycheck.el Outdated
(explanation (and explainer (funcall explainer first-error))))
(cond
((null first-error) (message "No errors at point"))
((null explainer) (message (format "No explainer for %S checker" checker)))

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Oct 1, 2016

Contributor

Do we need these messages? Why not just show the explanation or nothing if there is none?

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Oct 1, 2016

Author Member

Returning nil or the explanation was my intention; it seems more coherent with how most functions behave in Emacs. I used those messages for debugging :)

flycheck.el Outdated
(with-help-window (get-buffer-create "*flycheck-explain-error*")
(princ explanation)))

(defun flycheck-rust-error-explainer (error)

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Oct 1, 2016

Contributor

Please move this function down to the rust syntax checker, to keep everything related to Rust at one place.

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Oct 1, 2016

Author Member

I admit I had no idea where to put it!

flycheck.el Outdated
"Display the given error explanation in a help buffer."
(message explanation)
(with-help-window (get-buffer-create "*flycheck-explain-error*")
(princ explanation)))

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Oct 1, 2016

Contributor

I'm not sure whether we need both, a *message* and a dedicated help window.

Maybe it's better to just pop up the buffer?

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Oct 1, 2016

Author Member

message was for debugging while trying to find the right way to write in the help window.

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2016

@fmdkdd Thanks a lot for this change, I really like the approach.

I'm unsure about the user interface, though, see my comments.

flycheck.el Outdated
@@ -832,11 +832,11 @@ This variable is a normal hook. See Info node `(elisp)Hooks'."
(define-key map "e" #'flycheck-set-checker-executable)
(define-key map "?" #'flycheck-describe-checker)
(define-key map "h" #'flycheck-display-error-at-point)
(define-key map "x" #'flycheck-explain-error-at-point)

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Oct 1, 2016

Contributor

Oh dear, I'm so sorry 😞 Thanks for the change, but I messed things up: Actually I meant "e", not "x".

I'm terribly sorry for the nuisance but could you restore the x binding, drop the current e binding, and use e for explain? I think that's really the best binding, and e isn't terribly useful currently.

Really sorry to cause extra work 😞

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Oct 1, 2016

Author Member

No problem. But should we rebind set-checker-executable somewhere else? It is documented in doc/user/syntax-checkers.rst after all.

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Oct 1, 2016

Contributor

I don't think that's required; just remove the separate binding in the docs. It's ok if it's only available under M-x.

flycheck.el Outdated
@@ -8537,6 +8570,13 @@ Relative paths are relative to the file being checked."
:safe #'flycheck-string-list-p
:package-version '(flycheck . "0.18"))

(defun flycheck-rust-error-explainer (error)
"Return an explanation text for the given `flycheck-error' ERROR."
(let ((error-code (and error (flycheck-error-id error))))

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Oct 1, 2016

Contributor

Why the and guard for error? Can error be nil here?

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Oct 1, 2016

Author Member

Right. We don't call the explainer if there is no error.

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Oct 1, 2016

This loogs great! (LGTM :). I'll add a few comments directly to the code.

flycheck.el Outdated
"Display an explanation for the first error at point."
(interactive)
(let* ((errors-at-point (flycheck-overlay-errors-at (point)))
(first-error (car errors-at-point))

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Oct 1, 2016

Member

Do we want to explain just the first error, or all of them?

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Oct 2, 2016

Author Member

Well that was one of my questions as well. Currently I have no examples for rust where there are multiple explanations at point, but trying to get all the explanations might not make the code logic more complex. We would need to find a way to present them all to the user though.

flycheck.el Outdated
(checker (and first-error (flycheck-error-checker first-error)))
(explainer (and checker
(flycheck-checker-get checker 'error-explainer)))
(explanation (and explainer (funcall explainer first-error))))

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Oct 1, 2016

Member

This sequence looks like it would benefit from a -when-let, no? :)

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Oct 2, 2016

Author Member

Even better, -when-let* will take care of the (and ... checks. Though it seems when-let from subr-x does the same thing. Should we prefer the dash version?

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Oct 2, 2016

Member

Yup, that's what I meant :) when-let is emacs 25 only, whereas -when-let* is in dash.

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Oct 2, 2016

Author Member

Ok, thanks for the tip :)

@@ -5722,6 +5752,7 @@ SYMBOL with `flycheck-def-executable-var'."
(let ((command (plist-get properties :command))
(parser (plist-get properties :error-parser))
(filter (plist-get properties :error-filter))
(explainer (plist-get properties :error-explainer))

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Oct 1, 2016

Member

Sometimes I wish we has a let-plist in the style of let-alist :)

flycheck.el Outdated
(defun flycheck-rust-error-explainer (error)
"Return an explanation text for the given `flycheck-error' ERROR."
(let ((error-code (flycheck-error-id error)))
(when error-code

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Oct 1, 2016

Member

Same here, -when-let could help :)

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Oct 2, 2016

Author Member

Indeed!

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Oct 1, 2016

One more high-level comment: I wonder whether explainers should really return strings. Couldn't they just insert directly into the help buffer? This would save the roundtrip through a string.

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2016

I don't think that the round trip caused harm, and I'd rather like to avoid mutation unless it's absolutely necessary. Also returning structured data let's us swap the UI for explanations, e.g. iterate over all explanations at point, for instance. That's impossible if we'd just drop the explanations in the buffer.

@cpitclaudel

This comment has been minimized.

Copy link
Member

commented Oct 1, 2016

@lunaryorn Sounds good :)

@fmdkdd

This comment has been minimized.

Copy link
Member Author

commented Oct 2, 2016

I've updated the code to get the first error at point that has an error-explainer, that way the function explain-error will do its best to produce an explanation at point.

We could also collect all the explanations at point and keep the first one (but that would be wasteful, as in the case of rust-lang, we call rustc to get each explanation), or present all the explanations in a list (but that would probably too complex for a situation that doesn't arise in practice yet).

Besides adding docstrings and a line to CHANGES, I might still want to add the ability to get an explanation for an error in the error list. Should we add an extra column for explanations for all the errors that can be explained? The column would contain a button explain that would get the explanation and trigger the help buffer.

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2016

@fmdkdd Why not just add the same binding to the error list? I.e. C-c ! e and e (for simplicity) to explain the error in the row that point is on?

@lunaryorn
Copy link
Contributor

left a comment

Totally forgot about the documentation: Would you mind to add documentation for the new command to the manual? I guess the Interact with errors section would be the best place. Maybe add an "Explain errors" after the "Display errors" subsection?

flycheck.el Outdated
A function to return an explanation text for errors
generated by this checker.

TODO: describe

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Oct 2, 2016

Contributor

That needs to be filled I guess?

flycheck.el Outdated
POS defaults to `point'."
(interactive)
(-when-let*
((error (tabulated-list-get-id pos))

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Oct 3, 2016

Contributor

Move this to the line above. This kind of wrapping is really unusual for let-style bindings.

flycheck.el Outdated
(car (--filter (flycheck-checker-get (flycheck-error-checker it)
'error-explainer)
(flycheck-overlay-errors-at (point)))))
((first-error (--first (flycheck-checker-get (flycheck-error-checker it)

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Oct 3, 2016

Contributor

Likewise.

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Oct 3, 2016

Contributor

Please try to write this with functions from seq.el. I'd like to avoid dash in favour of built-in libraries, and I'm no particular fan of the macro forms.

flycheck.el Outdated
(car (--filter (flycheck-checker-get (flycheck-error-checker it)
'error-explainer)
(flycheck-overlay-errors-at (point)))))
((first-error (--first (flycheck-checker-get (flycheck-error-checker it)

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Oct 3, 2016

Contributor

Please try to write this with functions from seq.el. I'd like to avoid dash in favour of built-in libraries, and I'm no particular fan of the macro forms.

(defun flycheck-rust-error-explainer (error)
"Return an explanation text for the given `flycheck-error' ERROR."
(-when-let (error-code (flycheck-error-id error))
(with-output-to-string

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Oct 3, 2016

Contributor

Oh, that's nice… didn't know about that one 👍

@fmdkdd

This comment has been minimized.

Copy link
Member Author

commented Oct 3, 2016

I've added a button on the error-id in the error list, as well as a binding on e. Adding the link on error-id seems the more logical (the ID is used to get the explanation) while not disturbing the current format (since for the moment errors are only useful in the rust checkers).

I updated the corresponding test as well. But I can revert if we only want the binding and not the button on error-id.

Let me know if I should add some tests as well.

flycheck.el Outdated
;; Error ID has a button to explain the error
(list id-str
'type 'flycheck-error-list-explain-error
'face 'flycheck-error-list-id)

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Oct 3, 2016

Member

Maybe we could extend -error-list-make-cell? Or just drop the full -error-list-make-cell function?

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Oct 3, 2016

Author Member

Yeah I wasn't sure here. I can make the change either way, just tell me which way to try first.

flycheck.el Outdated
id-str 'flycheck-error-list-id id-str)
;; Error ID has a button to explain the error
(list id-str
'type 'flycheck-error-list-explain-error

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Oct 3, 2016

Member

Should we only add buttons when we have an error explainer for the corresponding checker?

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Oct 3, 2016

Author Member

That might be more intuitive yes. But it would require to check the existence of the explainer for all errors when constructing the list. For the moment there's no harm as it will just behave the same as calling -explain-error-at-point: if there's no explainer, it will do nothing.

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Oct 3, 2016

Member

Is that a costly check? Having buttons that do nothing might be confusing :/

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Oct 3, 2016

Author Member

Checking the explainer is just checking the nullness of the property on the error checker, so no, it shouldn't be costly. But it seems more confusing to have the button sometimes link to the error (when there is no explainer) and sometimes to the explanation (when there is one).

Maybe a compromise would be to make the button 'active' somehow (using faces?) when there is an explainer, so the user can know just by looking at the error list which errors can be explained?

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Oct 3, 2016

Author Member

Also, the presence of an explainer does not necessarily means there will be an explanation (the explainer may return nil). In that case, checking the presence of the explanation might be costly as it will most probably call external processes (it does for rust).

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Oct 3, 2016

Member

Aaaah, I get it now. I didn't realize that the button wasn't getting a link face (the one with a blue underline), sorry. My proposal was to remove that face and make the button inactive when there was no explainer… but now I realize that there is currently no face :)

I understand now; sorry for the back and forth. I think the current implementation is fine, and we can in th future possibly add a hyperlink to checkers that have an explainer.

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Oct 3, 2016

Author Member

In these instances it would be useful to be able to insert pictures in the review ;)

In any case, I've added a different face for error-id when an explainer is present. I've used a released-button box style, but an hyper link might be good as well. If it's too much, I can revert the commit as well.

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Oct 4, 2016

Contributor

@fmdkdd @cpitclaudel You lost me somewhere in the midst of your exchange 😊

The TL;DR is that the error ID column is now a proper button if an explainer is present and a plain text otherwise?

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Oct 4, 2016

Author Member

@lunaryorn Previously it was a button that linked to the error, as are all the cells in the error list really. They all use the -error-list button type that has -error-list-button-goto-error as an action.

What I did was add another button type (-error-list-explain-error) with the action -error-list-button-explain-error, but without changing the face. So previously the error id linked to the error, and now it linked to the explanation (by calling -display-error-explanation).

Without any explainer present, the button would do nothing, and I thought that was what @cpitclaudel was finding confusing. So I've then added a different face (-error-list-id-with-explainer) to error ids when an explainer is present; by default it looks like a clickable button.

Here is how it looks when the explainer is present (we can actually just drop images and Github will host them; I didn't notice that before):

screen

It's just a face, so we can tweak it. Or we can drop it altogether.

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Oct 4, 2016

Contributor

@fmdkdd I like it, thanks for taking the time to explain 👍

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2016

@fmdkdd Can you rebase onto master and force-push, to get the Travis CI build green again?

@@ -158,3 +158,15 @@ You can put errors into the kill ring with `C-c ! w`:
M-0 M-x flycheck-copy-errors-as-kill

Like `C-c ! w` but do not copy the error messages but only the error IDs.

Explain errors

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Oct 4, 2016

Contributor

Can you move the section before the "kill errors" section? I think "explaining" errors is the much more important use case as it offers immediate insight as to the cause of an error, whereas killing the error ID/message for use with Google is only the last resort, really.

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Oct 4, 2016

Author Member

I put after since it's only useful for the rust checkers at the moment, but I will move it.

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Oct 4, 2016

Contributor

@fmdkdd Yup, but it's definitely a feature that we should grow! I'm loving it.

@@ -137,9 +137,8 @@
(it "has the error ID in the 4th cell"
(expect (aref cells 3) :to-equal
(list "W1"
'type 'flycheck-error-list
'face 'flycheck-error-list-id
'help-echo "W1")))

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Oct 4, 2016

Contributor

Help-echo is gone?

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Oct 4, 2016

Author Member

Previously it was the error id, now it's "mouse 2, RET: explain error". Since this text was not tested for the other cells where it appears (line, column, level), I removed it here as well.

This comment has been minimized.

Copy link
@cpitclaudel

cpitclaudel Oct 4, 2016

Member

Hmm. We just added that help-echo feature in #1101, so I'm not sure about removing it :)

This comment has been minimized.

Copy link
@lunaryorn

lunaryorn Oct 4, 2016

Contributor

@fmdkdd Please restore the former help echo which showed the whole error ID. I did serve a not so unimportant purpose on the error ID column; we must not remove that.

This comment has been minimized.

Copy link
@fmdkdd

fmdkdd Oct 4, 2016

Author Member

Alright, I just reverted to show the error ID as the help-echo text and reverted the test as well.

@fmdkdd fmdkdd force-pushed the add-checker-explainer branch from 1dd61de to 0931bc5 Oct 4, 2016

@fmdkdd

This comment has been minimized.

Copy link
Member Author

commented Oct 7, 2016

@lunaryorn are we good to go? To me the PR is complete, including documentation and a line in CHANGES. Let me know if it needs further work.

@lunaryorn

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2016

@fmdkdd Of course, LGTM.

Sorry I thought I had already given my LGTM, but apparently I forgot 😢

Please go ahead and squash-merge 😊

Add the ability to explain errors at point
This commit adds the `:error-explainer` checker property, an optional
function that can be used to produce an explanation message for a given
`flycheck-error` object.

The user can call a new interactive function
`flycheck-explain-error-at-point` to display an explanation for the
error under the cursor using the `:error-explainer` function of the
error's checker.  The error list also gained support for the
explanation: clicking on the error ID displays the explanation.

This commits adds initial error explainer functions to the `rust` and
`rust-cargo` checkers using `rustc --explain`.

@fmdkdd fmdkdd force-pushed the add-checker-explainer branch from 50c1ec5 to 52eb56a Oct 7, 2016

@fmdkdd

This comment has been minimized.

Copy link
Member Author

commented Oct 7, 2016

@lunaryorn I squashed manually to set the commit and preserve the signature. You can merge now.

@fmdkdd fmdkdd merged commit 5e16cfc into master Oct 7, 2016

4 checks passed

approvals/lgtm this commit looks good
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@fmdkdd fmdkdd deleted the add-checker-explainer branch Oct 7, 2016

@fmdkdd

This comment has been minimized.

Copy link
Member Author

commented Oct 7, 2016

Oh well, adding the previous comment trigerred LGTM to refresh and let me merge.

Fantastic! I'm thrilled this landed. Great work reviewing @lunaryorn and @cpitclaudel, as usual 👍

lunaryorn added a commit that referenced this pull request Oct 7, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.