Flycheck: Displaying errors from other dependent buffers

[chrisdone]

Sorry to bring up an old issue (#829). I'd assumed there was a way in flycheck to display messages for other files. I'm writing a mode for Haskell, Intero, which uses flycheck as its main means of type-checking. It's basically a fork of GHCi and uses that to get very quick feedback.

I've been trying it out and it works nice, but sometimes if I change a type in e.g. Types.hs and switch to Packages.hs, the packages module no longer type checks because Foo.hs doesn't type check, but flycheck doesn't indicate it. A file that imports an invalid file is by definition not valid. It misleads me.

I assumed there must be some way and asked @dysinger who's been using flycheck for a while. He said when he wants that, he hits C-c C-l which uses the haskell-mode interactive mode session. I've no intention of abandoning flycheck due to this issue. I'd like to ride on the waves of Flycheck goodness rather than re-implementing this great functionality.

So my current thinking is that in my checker Elisp:

• There's an error, but it comes from a file that this file imports.
• Generate a new error on line 1 column 1 that says "Error in another file: X.hs"

At this point the user knows to switch to X.hs and then flycheck will show the errors for it. At least this way there is a clear process when things go wrong. It looks like this:

But it's a but unsatisfying. Ideally, there'd be an additional column like "File" that becomes visible when there're multiple files. Then when I'm trying to check "X.hs" I can quickly jump from there to the problem in "Y.hs" to fix the issue. It's sort of a mid-level functionality:

• Flycheck currently and, from reading this issue backlog (#829), only intends on, dealing with just this buffer
• In the middle, I want to check just one file, not the whole project, but I do care about dependencies.
• Compilation mode or a stack build deals with the whole project.

With haskell-interactive-mode, I do get this middle road solution. But that mode re-implements basically all the functionality in flycheck (line overlays, fringe displays, list of errors). A practical piece of work, considering at the time I wrote that code, flycheck didn't support generic checkers that could talk to a process. But now in my old age, I don't feel like re-implementing things from scratch if I can help it.

I took a look at flycheck-error-list-highlight-errors and fiddled around, I'm pretty sure I can add another column with this information.

Here's what I managed to cobble together by patching a few key places in flycheck.el. Here's a regular error:

Here's an error from another module:

You can just click the error to go to the other module.

Here's the diff (formatting on the (when ..) excluded so that it's shorter to read):

diff --git a/flycheck.el b/flycheck.el
index 4aabe9c..c1bb80f 100644
--- a/flycheck.el
+++ b/flycheck.el
@@ -748,2 +748,8 @@ This variable is a normal hook.  See Info node `(elisp)Hooks'."

+(defface flycheck-error-list-buffer-name
+  '((t :inherit buffer-menu-buffer))
+  "Face for buffer names in the error list."
+  :group 'flycheck-faces
+  :package-version '(flycheck . "0.16"))
+
 (defface flycheck-error-list-column-number
@@ -2921,2 +2927,3 @@ Return the created overlay."
   ;; We erase the highlighting later on in this case
+  (when (eq (flycheck-error-buffer err) (current-buffer))
   (pcase-let* ((`(,beg . ,end) (flycheck-error-region-for-mode
@@ -2941,2 +2948,3 @@ Return the created overlay."
     overlay))
+    )

@@ -3097,3 +3105,4 @@ the beginning of the buffer."
 (defconst flycheck-error-list-format
-  [("Line" 4 flycheck-error-list-entry-< :right-align t)
+  [("Buffer" 6)
+   ("Line" 4 flycheck-error-list-entry-< :right-align t)
    ("Col" 3 nil :right-align t)
@@ -3168,2 +3177,3 @@ Return a list with the contents of the table cell."
          (level-face (flycheck-error-level-error-list-face level))
+         (error-buffer (flycheck-error-buffer error))
          (line (flycheck-error-line error))
@@ -3174,3 +3184,6 @@ Return a list with the contents of the table cell."
     (list error
-          (vector (flycheck-error-list-make-number-cell
+          (vector (flycheck-error-list-make-cell
+                   (format "%s" error-buffer)
+                   'flycheck-error-list-buffer-name)
+                  (flycheck-error-list-make-number-cell
                    line 'flycheck-error-list-line-number)

The subtle invariant in generic checkers is that if you return a :buffer in the error struct which is not the current buffer, then obviously you have to have opened that file on flycheck's behalf. The diff for intero becomes:

+          (if (string= (buffer-file-name buffer)
+                       file)
+              (setq messages
+                    (cons (flycheck-error-new-at
+                           line column type msg
+                           :checker checker
+                           :buffer buffer
+                           :filename file)
+                          messages))
+            (when (eq type 'error)
+              (setq messages
+                    (cons (flycheck-error-new-at
+                           line column type msg
+                           :checker checker
+                           :buffer (find-file-noselect file))
+                          messages))))))

Obviously, this doesn't implement the flycheck-next-error-pos feature, which could be handy to quickly jump to the error in the other buffer. But I'm just trying to demonstrate to myself that this is a relatively straight-forward addition.

I think that actually a more comprehensive patch would support the "Buffer" column optionally, by changing flycheck-error-list-format to a defvar and then when generating the error list, it would re-initialize the tabulation mode with the additional column. Alternatively, probably better, it'd just be a customization option. Then users like me who use flycheck as a glorified on-the-fly compilation, could enable that option. Meanwhile the regular user just checks the syntax of their file and doesn't bother with such an option.

@lunaryorn Are you interested in taking this somewhere further? Basically, I'm going to make this patch in any case, because I'm prepared to provide users a pre-packaged works-out-of-the-box Haskell environment. I'm totally fine with importing a whole library into the codebase and patching it/rebasing upon it. That's not an aggressive way, more in an appreciative "cheers, and we'll add this little bit for our use-case" way. _But_, from about an hour and a half of twiddling while writing this comment I've sort of convinced myself it isn't even a big change to the codebase.

Feel free to close off-hand, but any tips to achieve what I want in the codebase would be appreciated. 😄 👍

[lunaryorn]

@chrisdone Wow, first of all, thank you very much for sharing your thoughts with me, and for taking the effort to demonstrate that it really works and adds value to Flycheck.

I like the idea of going on a middle road: We should not and cannot promise to check all files, but opening Flycheck to at least show all errors that were reported regardless from where they are in the error list is a good idea imho. I think we should definitely take this further, and I think I'd like to see Flycheck head in this direction.

I'd love to see a pull request that goes into this direction 😍 I think there are still some points that we need to discuss, but overall I think the way you presented it this is a great thing to have in Flycheck.

Would you open a pull request and go through a review with me?

[chrisdone]

Great! Thanks for reading my long post. :joy: I'll work on a PR tomorrow!

[lunaryorn]

@chrisdone All the more thanks for _writing_ it. It's awesome to see how much thought and effort you put into this. Thanks for helping to make Flycheck better!

[fmdkdd]

Implemented in #1427. :tada: