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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support errors found in other files #1427

Merged
merged 1 commit into from Mar 12, 2018

Conversation

Projects
None yet
8 participants
@purcell
Contributor

purcell commented Mar 11, 2018

This change revives the abandoned PR #1191 by @TyOverby, which in turn built on @chrisdone's PR #972 to address #966. There was some excellent feedback on #1191 by @fmdkdd in this comment, which this PR has incorporated.

  • Include errors (but not warnings) from other files in the error list
  • When clicking on the error in the error list, jump to the other file
  • Having jumped, force a re-check if and only if the other file's
    buffer does not already have that error listed
  • Display overlays for errors from other files on the first line
  • When showing such errors in the echo area, show the filename too

The hiding of other-file errors is an ongoing source of pain for Flycheck users in some language environments (Haskell, Elm, Rust), so I'm hoping we can get something merged soon.

Having spent a few hours on this, these changes seem to work well in local testing and feel quite natural to me - I'm keen to hear any further feedback! 馃帀

@CLAassistant

This comment has been minimized.

CLAassistant commented Mar 11, 2018

CLA assistant check
All committers have signed the CLA.

@purcell purcell force-pushed the other-file-errors branch 2 times, most recently from ba19b00 to 0fe7cac Mar 11, 2018

@purcell purcell requested a review from cpitclaudel Mar 11, 2018

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Mar 11, 2018

That moment when you get a PR from Steve Purcell 馃槏 馃 鉂わ笍
This looks superb. I should have time to do a proper review tomorrow. Thanks a lot!

@purcell

This comment has been minimized.

Contributor

purcell commented Mar 11, 2018

That moment when you get a PR from Steve Purcell

In theory I'm an admin here, but I haven't been active for ages. :-)

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Mar 11, 2018

In theory I'm an admin here

I know :) Doesn't reduce the excitement to review your code ^^

@purcell

This comment has been minimized.

Contributor

purcell commented Mar 11, 2018

I know :) Doesn't reduce the excitement to review your code ^^

Please lower your expectations. ;-)

@purcell purcell force-pushed the other-file-errors branch 3 times, most recently from aca2f10 to f67610f Mar 11, 2018

@purcell

This comment has been minimized.

Contributor

purcell commented Mar 11, 2018

I made a few further tweaks such that only errors from other files are shown, since in Elm - at least - warnings from other files would be displayed in the current file when not strictly necessary. In fact, a more conservative version of this change could choose to show errors from other files only if there are no errors listed for the current file. (I'd suggest it's not necessary to be so conservative.)

@purcell purcell force-pushed the other-file-errors branch 3 times, most recently from 3dccca5 to 108c7c6 Mar 11, 2018

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Mar 11, 2018

That looks very exciting! Thank you @purcell for working on this.

@cpitclaudel

cpitclaudel approved these changes Mar 11, 2018 edited

Thanks, this looks really great.

I've made a few comments here and there. The main thing I'm unsure about is overlays on the first line of the file. The nice bit about that trick is that it reuses the existing navigation infrastructure quite smoothly, so I could live with it :)

It might be nice to add a few tests for navigation (previous/next-error) with other files, too.

flycheck.el Outdated
(if id (format "%s [%s]" message id) message)))
(filename (flycheck-error-filename err)))
(concat (when (and filename (not (equal filename (buffer-file-name))))
(format "In \"%s\": " (file-relative-name filename default-directory)))

This comment has been minimized.

@cpitclaudel

cpitclaudel Mar 11, 2018

Member

Maybe add a newline after the colon?

This comment has been minimized.

@cpitclaudel

cpitclaudel Mar 11, 2018

Member

Should we show the path relative to the default directory, or to the current file? (I'm not sure whether default-directory here is the checker's :working-directory or the current buffer's folder)

This comment has been minimized.

@purcell

purcell Mar 11, 2018

Contributor

I'll add the newline and check whether the default directory is the right one here.

This comment has been minimized.

@purcell

purcell Mar 11, 2018

Contributor

I checked and the default-directory is the correct one, i.e. it's the directory of the current buffer's file.

flycheck.el Outdated
(let ((file-name (flycheck-error-filename err)))
(and buffer-file-name file-name
(not (flycheck-same-files-p buffer-file-name file-name))
(eq 'error (flycheck-error-level err)))))

This comment has been minimized.

@cpitclaudel

cpitclaudel Mar 11, 2018

Member

Should we make the error level for other files customizable? It might be best to wait until the feature is requested, though.

This comment has been minimized.

@purcell

purcell Mar 11, 2018

Contributor

I agree we should wait for a request.

flycheck.el Outdated
(defun flycheck-relevant-error-other-file-p (err)
"Determine whether ERR is a relevant error for another file."
(let ((file-name (flycheck-error-filename err)))
(and buffer-file-name file-name

This comment has been minimized.

@cpitclaudel

cpitclaudel Mar 11, 2018

Member

Do we really need to have a a buffer-file-name here?

This comment has been minimized.

@purcell

purcell Mar 11, 2018

Contributor

Yes, because buffer-file-name can be nil, in which case it's not safe to call the function which checks that the files are the same.

This comment has been minimized.

@cpitclaudel

cpitclaudel Mar 11, 2018

Member

Right, but we could also not call that function in that case (if I'm editing a scratch file and that file doesn't compile because of an error in an import, I might still be interested in that error)

This comment has been minimized.

@purcell-nec

purcell-nec Mar 11, 2018

Ah, I see! I think you're right. Will fix that in a few hours - currently at work. :-)

This comment has been minimized.

@cpitclaudel
(and buffer-file-name file-name
(flycheck-same-files-p file-name buffer-file-name))
;; This is a significant error from another file
(flycheck-relevant-error-other-file-p err))

This comment has been minimized.

@cpitclaudel

cpitclaudel Mar 11, 2018

Member

This logic feels a bit convoluted: flycheck-relevant-error-other-file-p is used in two places: here it simplifies to (eq 'error (flycheck-error-level err)), and鈥 (see below)

This comment has been minimized.

@purcell

purcell Mar 11, 2018

Contributor

See comment below.

This comment has been minimized.

@cpitclaudel

cpitclaudel Mar 11, 2018

Member

OK, makes sense. Thanks!

(pcase-let* ((`(,beg . ,end) (flycheck-error-region-for-mode
err (or flycheck-highlighting-mode 'lines)))
(pcase-let* ((`(,beg . ,end)
(if (flycheck-relevant-error-other-file-p err)

This comment has been minimized.

@cpitclaudel

cpitclaudel Mar 11, 2018

Member

(continued) 鈥ere it simplifies to having different file names, since the error has already been filtered. I wondered if it'd be better to inline it in both places and simplify accordingly.

This comment has been minimized.

@purcell

purcell Mar 11, 2018

Contributor

No, it doesn't simplify to that - either the error's file or the current buffer's file name could be empty, and duplicating that logic from flycheck-relevant-error-file-p therefore didn't feel right here. So I extracted flycheck-relevant-error-other-file-p, with the result that that function checks the presence of those two filenames - in the context in which it is called in flycheck-relevant-error-file-p that check is redundant.

flycheck.el Outdated
;; Display overlays for other-file errors on the first line
(cons (point-min) (save-excursion
(goto-char (point-min))
(end-of-line) (point)))

This comment has been minimized.

@cpitclaudel

cpitclaudel Mar 11, 2018

Member

Maybe point-at-eol?

This comment has been minimized.

@purcell

purcell Mar 11, 2018

Contributor

Nice!

@@ -3940,7 +3965,8 @@ message to stretch arbitrarily far."
message checker-name)))
(defconst flycheck-error-list-format
`[("Line" 5 flycheck-error-list-entry-< :right-align t)
`[("File" 6)
("Line" 5 flycheck-error-list-entry-< :right-align t)

This comment has been minimized.

@cpitclaudel

cpitclaudel Mar 11, 2018

Member

It it worth adding a column to the error list, vs. adding the file name to the error message? Arguably it's nice to be able to sort by file, but in most cases it'll take space somewhat needlessly. Could we hide that column when all errors point to the same file?

This comment has been minimized.

@purcell

purcell Mar 11, 2018

Contributor

Yes, there are probably some things we could do here, but I'm also keen not to try to gold-plate this before it has seen use in the wild. My suggestion would be to go with this simple approach for now.

This comment has been minimized.

@cpitclaudel
;; If widening gets in the way of moving to the right place, remove it
;; and try again
(widen)
(goto-char pos)))

This comment has been minimized.

@cpitclaudel

cpitclaudel Mar 11, 2018

Member

I wonder why that function didn't just check whether (<= (point-min) pos (point-max)) (I know it's existing code).

This comment has been minimized.

@purcell

purcell Mar 11, 2018

Contributor

Hmmm... good point!

@purcell purcell force-pushed the other-file-errors branch from 108c7c6 to 3d84972 Mar 11, 2018

@purcell

This comment has been minimized.

Contributor

purcell commented Mar 11, 2018

I've made a few comments here and there. The main thing I'm unsure about is overlays on the first line of the file. The nice bit about that trick is that it reuses the existing navigation infrastructure quite smoothly, so I could live with it :)

Yeah, that's what I like about it too. It's simple, and it makes those other-file errors obvious, at least.

It might be nice to add a few tests for navigation (previous/next-error) with other files, too.

Yep, that would be nice for completeness. The existing tests still pass, which is encouraging. ;-)

@purcell

This comment has been minimized.

Contributor

purcell commented Mar 11, 2018

I've pushed those couple of little fixes.

(vector (flycheck-error-list-make-cell
(if filename
(file-name-nondirectory filename)
"")

This comment has been minimized.

@cpitclaudel

cpitclaudel Mar 11, 2018

Member

I think this reduces to (file-name-nondirectory (or filename ""))鈥 not sure it reads better.

This comment has been minimized.

@purcell-nec

purcell-nec Mar 11, 2018

It has the same effect and is more concise, but it calls file-name-nondirectory unnecessarily on "", so I'd say keep the if. (This came from the original PR, and I didn't find a better way to express it.)

Support errors found in other files
- Include errors (but not warnings) from other files in the error list
- When clicking on the error in the error list, jump to the other file
- Having jumped, force a re-check if and only if the other file's
  buffer does not already have that error listed
- Display overlays for errors from other files on the first line
- When showing such errors in the echo area, show the filename

@purcell purcell force-pushed the other-file-errors branch from 3d84972 to aa7e6ab Mar 12, 2018

@purcell

This comment has been minimized.

Contributor

purcell commented Mar 12, 2018

I've tried really hard to find somewhere to add relevant tests, but gave up. With the PR in its current form, there's no real effect on error navigation, either in-buffer, or when clicking on items in the error list. It's not clear how best to set up a situation where errors are generated for a depended-upon file -- from what I can see, this isn't the case for the language resources used by existing tests, and there's no existing coverage of the flycheck-relevant-error-p. So I'm feeling a bit defeated on that front, and am inclined to merge regardless. :-/

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Mar 12, 2018

am inclined to merge regardless. :-/

So am I :) This is a great PR, and I've nitpicked enough at this point ^^ @fmdkdd, thoughts/comments? I'll merge tonight from home otherwise.

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Mar 12, 2018

@cpitclaudel I haven't had a chance to take a look yet. I should be able to do so in the next 24h.

@cpitclaudel

This comment has been minimized.

Member

cpitclaudel commented Mar 12, 2018

No pressure :) I just meant to confirm that I was ready for merging on my end.

@purcell

This comment has been minimized.

Contributor

purcell commented Mar 12, 2018

Alrighty, I'm going for it. If there are any resulting issues which appear related, please CC me into them!

@purcell purcell merged commit 2a00e5c into master Mar 12, 2018

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

purcell added a commit to commercialhaskell/intero that referenced this pull request Mar 13, 2018

@purcell

This comment has been minimized.

Contributor

purcell commented Mar 13, 2018

Sorry @fmdkdd - if you end up taking a closer look and find anything that bugs you, just follow up here with any feedback and I'll be happy to tinker further.

@purcell purcell deleted the other-file-errors branch Mar 13, 2018

@fmdkdd

That looks great to me. I made just a small comment.

Adding error overlays from other files on the first line actually breaks the integration tests for Rust (and would probably affect other project-based checkers depending on the tests we have). If we want to keep that solution, we'll have to update the tests infrastructure to consider that case.

What happens if we don't add these overlays? The errors from other files still appear in the error list, and in the modeline, and are caught by next-error, right? Would it be that confusing to not display the squiggles in that case?

In any case, I'm excited to have that feature finally land. I would have moved the changelog item much further up ;)

(goto-char pos)
(user-error "No more Flycheck errors"))))
(-if-let* ((pos (flycheck-next-error-pos n reset)))
(let ((err (get-char-property pos 'flycheck-error)))

This comment has been minimized.

@fmdkdd

fmdkdd Mar 13, 2018

Member

Isn't that let superfluous since we already have -if-let* above?

This comment has been minimized.

@purcell

purcell Mar 13, 2018

Contributor

Yes, good catch. Fixed in 0ff3c72.

purcell added a commit that referenced this pull request Mar 13, 2018

@purcell

This comment has been minimized.

Contributor

purcell commented Mar 13, 2018

Adding error overlays from other files on the first line actually breaks the integration tests for Rust (and would probably affect other project-based checkers depending on the tests we have). If we want to keep that solution, we'll have to update the tests infrastructure to consider that case.
What happens if we don't add these overlays? The errors from other files still appear in the error list, and in the modeline, and are caught by next-error, right? Would it be that confusing to not display the squiggles in that case?

I personally think that the overlay on the first line is the single most important part of this change. Flycheck is most commonly used without the error-list, from what I've observed of my own and others' usage, and in that mode of usage it's not sufficient to see only the error count in the modeline: you need an overlay to jump to, and to trigger the display of the error in the echo area.

It's certainly a little ad-hoc to bundle errors onto the first line, but in practical use it seems fairly optimal.

Sorry it broke the Rust tests. Do they run automated somewhere? I'll happily see if I can run them locally and fix them up accordingly.

In any case, I'm excited to have that feature finally land. I would have moved the changelog item much further up ;)

I actually went ahead and did that, too, because it makes sense to me to mention it before readers zone out. :-)

aeggenberger added a commit to aeggenberger/flycheck that referenced this pull request Mar 14, 2018

fmdkdd added a commit that referenced this pull request Mar 14, 2018

Update Rust tests for errors from other files
Since GH-1427, errors from other files are included in the current
buffer.  So they must be included in the integration tests.

In the case of Rust, macro errors emit diagnostics with a phony file, so
we now filter these errors to prevent them from appearing in the buffer.
@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Mar 14, 2018

Do they run automated somewhere? I'll happily see if I can run them locally and fix them up accordingly.

No, unfortunately they don't run automated anywhere. I've fixed them in #1433, since it was a good opportunity to refresh them.

you need an overlay to jump to

Ah that's the crucial point. So next-error does not work without that overlay. In that case then I agree that putting the error on the first line is acceptable.

Thank you again for pushing this :)

@chrissound

This comment has been minimized.

chrissound commented May 20, 2018

Can anybody point me to how to enable this functionality? Is it meant to show up in the default list from flycheck-list-errors?

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented May 20, 2018

@chrissound It is enabled by default, if you update Flycheck from MELPA (not stable). Errors from other files will appear on the first line of your buffer (and in the error list), provided that 1) the checker actually emits errors for other files, and 2) you have no errors in the current buffer (if you have only warnings or info messages in the current buffer, errors from other files should appear).

@ntc2

This comment has been minimized.

ntc2 commented May 21, 2018

@fmdkdd said:

Adding error overlays from other files on the first line actually breaks the integration tests for Rust (and would probably affect other project-based checkers depending on the tests we have). If we want to keep that solution, we'll have to update the tests infrastructure to consider that case.
What happens if we don't add these overlays? The errors from other files still appear in the error list, and in the modeline, and are caught by next-error, right? Would it be that confusing to not display the squiggles in that case?

@purcell said:

I personally think that the overlay on the first line is the single most important part of this change. Flycheck is most commonly used without the error-list, from what I've observed of my own and others' usage, and in that mode of usage it's not sufficient to see only the error count in the modeline: you need an overlay to jump to, and to trigger the display of the error in the echo area.

Some feedback after using this feature for a few days now: I like having the overlay on the first line for errors in other files, but I'd like to ability to jump to the overlay (with flycheck-{previous,next}-error) without actually jumping to the other file containing the error. I find it jarring/surprising when I'm editing one file, I try to jump to the next error, and suddenly I'm in another file. I think it would be less surprising if the first jump brought to the overlay, and I then had to jump again to actually change files.

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented May 22, 2018

I find it jarring/surprising when I'm editing one file, I try to jump to the next error, and suddenly I'm in another file.

Well, maybe it's because you are not used to it yet? ;)

I think it would be less surprising if the first jump brought to the overlay, and I then had to jump again to actually change files.

We could add an option along these lines: if it's an error from another file, jump to the in-buffer overlay, /unless/ the cursor is already on the overlay, and in that case we jump to the other file instead.

Do you want to make a PR?

@ntc2

This comment has been minimized.

ntc2 commented May 22, 2018

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented May 22, 2018

Do you have a pointer to some docs on developing and testing Emacs packages in general, or flycheck specifically?

We do. The Emacs Lisp manual inside Emacs is also invaluable if you want to brush up on Elisp programming.

For this specific feature, you want to look at flycheck-jump-to-error in flycheck.el. You can use flycheck-overlay-errors-at to get the errors under the cursor, and compare that to the error you need to jump to.

If you want to add tests, you should add them in specs/test, maybe under new file test-error-navigation.el.

Do not hesitate to ask further questions.

ntc2 added a commit to ntc2/flycheck that referenced this pull request Jun 2, 2018

Add option `flycheck-jump-to-other-file-immediately`
This options controls whether Flycheck error navigation jumps to other
files immediately when the selected error is in another file. This is
related to PR flycheck#1427 that added overlays on the first line of the
current file when errors in other files prevent checking the current
file.

When the new option `flycheck-jump-to-other-file-immediately` is
nil (it defaults to non-nil), error navigation in the current file
moves the cursor to first-line overlay, and then you have to jump
again to actually go to the other file containing the error. The idea
is that it's surprising for Flycheck error navigation to change files,
and stopping on the first-line overlay first makes it less surprising.

There is more context for this commit here:
flycheck#1427 (comment).
@ntc2

This comment has been minimized.

ntc2 commented Jun 2, 2018

@fmdkdd I just submitted PR #1473 in relation to our discussion.

I did not add any tests, because I don't understand how the tests work, and above @purcell mentioned he couldn't figure out where to add tests for his new first-line overlays feature. I'm testing locally with two Haskell files, where one imports the other, and the imported file doesn't type check.

As part of my testing, I found a bug in flycheck-first-error / flycheck-next-error-pos, and I'm looking into fixing that separately. The problem is that flycheck-next-error-pos assumes that the first position in the file, (point-min), is not part of an error overlay, but that's untrue for the new first-line error overlays. So, tests for the new first-line error overlays might be a good idea.

I didn't ask very clearly, but when I asked about docs for developing Emacs packages, what I wanted to know was how to run my locally modified version of the package, preferably without affecting my global Emacs installation. I found good docs for that in this Stack Overflow answer, and ended up using the emacs -L <path to my local flycheck repo> approach. I would like to add a pointer to this in the contributor docs, if you're OK with that.

ntc2 added a commit to ntc2/flycheck that referenced this pull request Jun 2, 2018

Fix `flycheck-first-error` for first-line overlays
The first-line error overlays added in PR flycheck#1427 weren't handled
correctly by `flycheck-first-error`, due to a bug in
`flycheck-next-error-overlay-pos`, where that function assumed that
`point-min` would not be part of an error overlay in the `reset` case.

This bug and fix is unrelated to PR flycheck#1473, except that I ran into the
bug while testing flycheck#1473.
@ntc2

This comment has been minimized.

ntc2 commented Jun 2, 2018

@fmdkdd I fixed the flycheck-first-error problem I mentioned above (fix: 3f0788f) and a similar flycheck-next-error problem (fix: d174f55) that I discovered after that. Both are related to the new other-file overlays introduced by #1427, so more evidence that some tests would be good.

The current specification for how Flycheck error navigation (flycheck-{next,previous,first}-error) works with relation to other-file error overlays is:

  • if the cursor is already on an other-file overlay, then that overlay is considered to be the next/previous overlay, and jumping to it jumps to corresponding error in the other file.

  • if the cursor is not already on an other-file error overlay, then the next/previous/first overlay is the next/previous in relation to the cursor, or first, respectively. If the overlay determined in this way is an other-file overlay, then what happens when you jump to it is determined by the setting of flycheck-jump-to-other-files-immediately: if that setting is non-nil, the default, then jump to the error in the other file; if that setting is nil, then jump to the other-file overlay in the current file.

@fmdkdd

This comment has been minimized.

Member

fmdkdd commented Jun 4, 2018

I would like to add a pointer to this in the contributor docs, if you're OK with that.

That's a good idea, please do. I've used emacs -l/-L in for reproducing bugs as well. The file test/init.el can also be used to launch an Emacs session with Flycheck, without impacting the local Emacs configuration.

For the rest, I'll move the discussion to #1427 directly.

@raxod502

This comment has been minimized.

Contributor

raxod502 commented Jul 16, 2018

what I wanted to know was how to run my locally modified version of the package, preferably without affecting my global Emacs installation

Another option for this is using straight.el. The "without affecting my global Emacs installation" part is covered because you can either:

  1. Just check out the feature branch, and run your usual Emacs configuration, or
  2. Check out the feature branch, and then load the package from emacs -Q,

and in neither case is any change to your ~/.emacs.d necessary.

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