Add ability to set end column and line on error #1400
Conversation
|
|
|
Another thing to add would be special regexes to make it easy to use these when defining a generic checker. Not sure how it's already implemented for line and column right now. |
|
Thanks, this looks very exciting :) I'm hoping to have time to review this within the week, but it may have to wait until next weekend. Hopefully @fmdkdd or another team member can beat me to it ^^ |
There was a problem hiding this comment.
OK, I did a read-through. I like this code :) I like that there isn't too much of it, either.
I think we can still polish the APIs a bit further, though.
The main question is: what do we consider a complete flycheck-error object? Should it have all its fields set? Should it even have the end-line and start-line set?
Right now the current code puts in the object exactly what the checker gave it, lazily computing and caching start-line, end-line, and region — right? This approach mostly makes sense to me, but I think we also ought to more aggressively populate end-line when it's cheap to do so (that is, when we don't have an end-line but we do have a start-line), so that we don't have to null-check it.
Do I recall correctly that the reason to not aggressively compute the line and column numbers is that they are only needed in the error list?
Do we have a similar reason to not aggressively compute the region when we have start and end lines and start and end columns?
I think we also ought to document somewhere what happens when you have an end line but no end column (we should disallow that case, I think).
Thanks for working on this. I think we're almost there, and I'm excited to use this. The tide checker can certainly benefit of this, and so can F* and certainly a bunch of others.
flycheck.el
Outdated
| `end-line' (optional) | ||
| The last line number the error refers to, as number. | ||
|
|
||
| This is used if the error spans multiple lines. |
There was a problem hiding this comment.
I'd just say here that this default to being equal to the start-line
flycheck.el
Outdated
| This is used if the error spans multiple lines. | ||
|
|
||
| `end-column' (optional) | ||
| The last column number the error refers to, as number |
flycheck.el
Outdated
| `end-column' (optional) | ||
| The last column number the error refers to, as number | ||
|
|
||
| This is used to highlight the error more precisely. |
There was a problem hiding this comment.
I'd rather emphasize the fallback behavior, by pointing to flycheck-highlighting-mode.
flycheck.el
Outdated
| `region' (optional) | ||
| The region the error refers to as a cons of byte offsets | ||
| from the start of the file. If this is given then the line | ||
| and column information can be computed from it. |
There was a problem hiding this comment.
The second sentence is a bit misleading. I'd say that the line-start, line-end, column-start-column-end are equivalent to this property.
We should make sure that we don't run into issues with narrowing, btw. Caching the offsets is good, but we should make sure that we widen appropriately before computing them or using them
flycheck.el
Outdated
| (setf (flycheck-error-start-column err) (current-column)) | ||
| (goto-char (cdr (flycheck-error-region err))) | ||
| (setf (flycheck-error-end-line err) (line-number-at-pos)) | ||
| (setf (flycheck-error-end-column err) (current-column))))))) |
There was a problem hiding this comment.
I wonder if we could refactor this to share some code with flycheck-error-line-region etc
flycheck.el
Outdated
| (push | ||
| (flycheck-error-new-at | ||
| .line_start | ||
| .column_start | ||
| (- .column_start 1) |
There was a problem hiding this comment.
Was that an existing error in the Rust checker?
There was a problem hiding this comment.
I think this was an existing bug that nobody caught because the highlighting mode meant it still highlighted the correct region.
flycheck.el
Outdated
| (or (cdr (assq 'column_start (car .spans))) | ||
| primary-column) | ||
| (- (or (cdr (assq 'column_start (car .spans))) | ||
| primary-column) 1) |
There was a problem hiding this comment.
Same here: was this a bug in the checker definition?
flycheck.el
Outdated
| (flycheck-error-region-for-mode err 'columns))) | ||
| (`lines (flycheck-error-line-region err)) | ||
| (_ (error "Invalid mode %S" mode)))))) | ||
| (setf (flycheck-error-region err) region)))) |
There was a problem hiding this comment.
This is a bit of a change in behavior — it's legit to cache the region when we have a all 4 fields (beginning and end of line and column), but it cases like symbols or sexp, we don't currently cache the result.
So maybe we could make flycheck-error-exact-region set the region field (and return it), and there would be no more need for the final setf.
flycheck.el
Outdated
| :end-column (flycheck-string-to-number-safe end-column) | ||
| :region (when (or region-start region-end) | ||
| (cons (flycheck-string-to-number-safe region-start) | ||
| (flycheck-string-to-number-safe region-end)))))))) |
There was a problem hiding this comment.
Should we add region-start and region-end patterns if we don't have use cases for them yet? I wouldn't expect many CLI tools that only provide a text-based interface to provide regions.
flycheck.el
Outdated
|
|
||
| (defun flycheck-error-line (err) | ||
| (flycheck-error-calc-line-col-from-region err) | ||
| (flycheck-error-start-line err)) |
There was a problem hiding this comment.
Shouldn't we have accessors of this style for all 4 fields?
There was a problem hiding this comment.
I only added this to make sure I wasn't breaking the "public" API of the error struct.
The rust checker would benefit as well. We should open an issue to update these checkers after this is merged. |
|
Thanks for the comments, I'll try and respond this week! I would say that the reason I lazily compute everything is because that's what we discussed. My instinct is to precompute everything on error construction because it simplifies the logic by ruling out invalid states, but you thought from an efficiency standpoint the lazy approach was better. |
Yeah, I expected that :) Confusingly, I can't find traces of that previous conversation here on Github, probably because it was attached to old commits? I'm not rejecting that approach, just trying to refine it :)
Right, I think I remember. Reading through the code, I also think there's one thing that (might) force us to be lazy, namely Another factor is lazy overlay display (only adding overlays to the visible portion of the buffer). We don't do it currently; we might do it in the future. My natural tendency is to ignore that issue entirely and design the current patch for what we currently have. So, as the code is currently, lazy-computing lines and columns will save time for checkers that return a region, but lazy-computing the region won't save time in any case, because we always need a region for overlays. Amusingly, though, caching the region after computing it also doesn't buy us anything, because we only need it once, to create an overlay. Conversely, caching lines and columns is worth it, because we can display the error list multiple times. I also think your concern about consistency (ruling out invalid states) is valid. This suggests to me that the right way to think about this is that there are two types of locations: regions, and line-column pairs. We need to convert from line-column pairs to regions to display overlays, and we need to convert from regions to lc pairs to display the error list. Additionally, lc pairs can be partial (a checker can give us only a beginning line and a beginning column, and no end-line and end-column, or even just a line). So, what about the following: let's give errors two fields, coordinates (two line-column pairs) and region. We then maintain two invariants: A nice property is that for checkers that return partial coordinates, computing the region will also require filling the missing coordinate bits, so really we'll only hit the lazy case when the checker returned a region. I think this is a very small change, and I think it'll make things clearer (and the invariant is fairly easy to state, too). What do you think @jbaum98 ? |
|
I really like that idea, I think that makes a lot of sense. I'll make the change and the other details sometime this week. Thanks for the thoughtful feedback! |
|
It seems like right now there are actually two different uses for the error region. The first is to add the overlay and is exposed through What we discussed above includes always precomputing the error region, which means that It seems like the old behavior is kind of strange to begin with, so I think we should unify the two and just have a single region we use for overlaying and jumping. Is there a reason we should worry about this change in behavior though? |
|
Great point. I can't think of a case where this would make a significant difference, though, because in practice we're looking at the first character of the region. I'd expect that to be the same for most highlighting modes that return non-nil. In any case, that's not a change I'd worry about at all. |
|
I think I've got it working. I didn't update the documentation because I wasn't sure where to put what, so let me know about that. I also took away the regexes and rust checker update, but we can add that after. |
flycheck.el
Outdated
|
|
||
| (cl-defun flycheck-region-from-cons ((start-pos . end-pos)) | ||
| "Create a region from a cons cell of `(START-POS . END-POS)`." | ||
| (flycheck-region-new start-pos end-pos)) |
There was a problem hiding this comment.
I think the usual representation of ranges in Emacs is a simple cons. Unless we need something fancier, it might be better to stick with that?
flycheck.el
Outdated
| (cl-defstruct (flycheck-coords | ||
| (:constructor flycheck-coords-new (start-line end-line start-col end-col))) | ||
| "Structure representing the location of an error as lines and columns." | ||
| start-line end-line start-col end-col) |
There was a problem hiding this comment.
I'd call this flycheck-error-coords :)
flycheck.el
Outdated
| (let ((coords (flycheck-region-to-coords (flycheck-error-region err) | ||
| (flycheck-error-buffer err)))) | ||
| (setf (flycheck-error-coords err) coords) | ||
| coords))) |
There was a problem hiding this comment.
Advice is typically reserved for user code, or desperate situations; let's find a different way to do this.
There was a problem hiding this comment.
The reason I did this is because I want to override the default getter for flycheck-error-coords. Another way to do this would be to just redefine flycheck-error-coords and use cl-struct-slot-offset to access the coords, but that seemed to be worse because it relies on the slot position explicitly and not really different. What do you think I should do here instead?
flycheck.el
Outdated
| (gv-define-simple-setter flycheck-error-column | ||
| (lambda (err x) | ||
| (setf (flycheck-coords-start-col (flycheck-error-coords err)) x) | ||
| (setf (flycheck-coords-end-col (flycheck-error-coords err)) x))) |
There was a problem hiding this comment.
We should update the error region here, right?
There was a problem hiding this comment.
Will this make everything work with error filters, because the way they work is by modifying errors through setf? Is this maybe a drawback to our new practice of precomputing the region; ie. when we change the line and column we need to recompute?
There was a problem hiding this comment.
Also, should we be exposing an API for modifying start-line and end-line etc that will also update the region? Right now the only way for them to do this is to modify it within the coords, which won't update the error region. Should there be flycheck-error-start-line getters and setters to create the illusion that the start-line and others are stored directly on the error?
flycheck.el
Outdated
| "Convert coordinates to a full set of coordinates and a set of buffer-offsets. | ||
|
|
||
| Returns a cons-cell of `(FULL-COORDS . REGION)` where FULL-COORDS | ||
| is a set of coordinates where non are nil, and REGION is a |
|
@cpitclaudel just giving you a ping, any idea when you can take a look at this? |
|
Sorry for the delay; this has been very close to the top of my priority list or the whole week-end. |
|
Hey @jbaum98, I've finally managed to set aside enough time to look at this seriously, including writing new tests and adjusting existing ones, and updating docstrings. There were a few things left to solve, which we had discussed:
I've pushed two commits on top of your branch to address these points (it felt only fair that I should do that work, given how long it's taken me to get back to this); see https://github.com/flycheck/flycheck/tree/1400-error-ranges . I have not tested it much beyond ensuring that the unit tests pass. Would you like to review these two patches? I could open a separate PR with your commits plus mine, or more simply you could pull these two commits into your branch and review them here. (Btw, I changed flycheck-error-coords back to flycheck-coords; the former name made everything too verbose. Sorry for asking you to change it previously). |
|
Great! I'll pull them and review them here when I get the chance. Probably sometime this week after Monday. |
|
Thanks a lot. |
|
@cpitclaudel So sorry that it took so long. I looked at your changes and they look great. I squashed to make the rebase easier and rebased from master. |
|
Thanks! I have one more small fix to push, and then we'll ask @fmdkdd for a final proofread :) I should have time this weekend or early next week. |
Ahead of that, it would be good to fix the errors reported by the CI (it seems there are formatting errors at the moment). |
This change allows checkers to position error overlays precisely, either using start and end columns, or using an explicit region. In addition to flycheck-error-new-at, kept for backwards compatibility, clients can now use flycheck-error-new-with-coords and flycheck-error-new-with-region. No changes should be requires in existing checkers: for compatibility, the old flycheck-error-line and flycheck-error-columns getters now return the start line and column, and the corresponding setters reset the end line and column. Because converting between buffer offsets and line/column pairs is costly, the calculations are done lazily, using accessor functions that cache their results. All coordinates are stored in a new flycheck-coords structure. We originally thought that we could compute buffer offsets eagerly when given line/column pairs, but many checkers start by creating an error object and then adjust individual line-column values (e.g. using flycheck-increment-error-columns).
|
@fmdkdd @cpitclaudel What's the status of this? It looks the CI is all good. Is there anything else we need to do before merge? |
|
I didn't forget about this :) Sorry for not posting more updates. I've been running this as my main Flycheck since our last discussion, but I did run into a subtle issue that I didn't expect. The issue is that this change breaks packages that depend on Flycheck, like flycheck-ocaml and others, until they are recompiled — but package.el doesn't recompile dependent packages when installing an updated dependency. I described the issue at https://lists.gnu.org/archive/html/emacs-devel/2018-07/msg00436.html . I haven't found a fully satisfactory answer yet :/ |
|
So, when we added the How chaotic would it be if we merged the PR now? Are we talking major breakage of many packages? Is there a way to approximate how many packages would be affected? (Using i.e., Github search?) In any case, the fix is to, at worst, recompile all packages? We could provide a small Elisp snippet that does that. Recompiling doesn't take much time, and doesn't have other downsides. If we can avoid any breakage, then all for the better. But the fix has to be reasonable, and shouldn't delay this PR into oblivion. |
😨 indeed.
Right, adding at the end is a neat trick, as long as we don't have external packages passing errors to us, in which we expect a group to appear. This trick won't work with positions, though.
Chaotic enough that I noticed it — the package that broke for me was merlin. My own dafny and fstar packages would break, too. Any package that accesses an error position would break too. And any package that passes us errors with positions, which is any external checker that uses a subprocess.
Indeed. The issue is cleanly detecting the issue.
Agreed (sorry @jbaum98, it's indeed taking forever!). My current thinking is that we may be able to add code at the end of Flycheck.el that recompiles packages that depend on it if needed. But that will only work with package.el. With other package managers, people will have to trigger a recompile themselves, so we'll likely want to have some way of detecting the problem. |
|
It sounds like though, we should have one breaking PR that addresses this specifically by
Then, once we've landed that PR we can freely land this PR and any other we like without fear of further breakage. Mainly, I think the issue of having accidentally exposed implementation details that make it hard to change stuff is its own issue and discussion/fixes for it should be in its own PR. |
|
I agree. But part of me was hoping for a simple fix, hance the discussion on emacs-devel first. |
|
Could this be a good opportunity to do something related to #774 also? If we're already committed to a sort of breaking change? |
|
Indeed, probably… but I'd prefer shipping your change soon instead of packaging it into something a lot larger. |
|
There's a cool function Simply put it somewhere on top-level and save into a variable that you already shown it in this session, or, for bonus points, run a predicate that can tell if the problem is present or not (e.g. check the timestamps of the This way you can roll out the change and gracefully inform users about the problem. |
|
@cpitclaudel I see you wrote a fabulous bit of elisp to recompile dependencies of flycheck: https://lists.gnu.org/archive/html/emacs-devel/2018-07/msg01159.html . If I understand correctly, you just need a way of detecting when to run this. Could you just do this if an old function is still defined? I'd think a feature test, e.g. |
|
Am I correct in that this patch is essentially feature-complete and tested, just not merged yet because of the dependency-problem? |
|
@Profpatsch The dependency problem is not small. If we can avoid breaking setups, we should try our best. @cpitclaudel Following @Wilfred suggestion, if a known obsolete function exists, then we need to recompile. If it doesn't, then we don't? Wouldn't that be a way to force the recompilation of dependencies only once? |
From what I understand it’s a general problem of Emacs’s statefulness. I’ve had other packages break symbols on updates, usually it fixed itself after a few days and another update. Not a giant problem in practice. |
|
Just wanted to bump this again. I know we were worried about breaking people's installs. Is that still an issue, and if so, is there anything I can do to make progress on it? Maybe I never fully understand the issue, but is there a reason why we can't tell people to recompile their bytecode and restart emacs? It's obviously not ideal, but if it seems intractable to preserve bytecode compatibility or to automatically recompile, I don't think it's worth not making these changes. Seeing as this PR has already waited a while though, I do think it makes sense to batch this with other breaking changes, like something related to #774, as we discussed about a year ago. |
|
@jbaum98 Agreed, I would like to merge this as well. AFAIK, there is no solution that would allow us to recompile all of Flycheck dependencies automatically. @cpitclaudel's snippet works only for I would be in favor of:
For the warning, we could do something like checking if any loaded feature starts with #774 sounds nice, and I'm sure the LSP folks could also benefit from that. I don't want to delay this PR much further though. @cpitclaudel How does that sound for you? |
|
I agree with you. Sorry for the low responsivity. Here is one more thought. Since Flycheck is large, maybe we could very kindly ask @purcell to do a no-change rebuild of Flycheck dependencies after we push our update? (:angel: :bowing_man:) |
I might be missing something, but I don't think that would achieve anything, since byte compilation happens in Emacs, not on the MELPA server. There's no way to force an artificially-new version of packages in MELPA, since the version comes solely from the last-modified time of the most-recently-updated packaged file. |
I was thinking that if MELPA marked packages depending on flycheck as updated, it would cause users to redownload them, and hence recompile them.
Ah, that's too bad. |
|
Does this patch help move cursor to the EOL errors?
Will checkers need to be rewritten? |
|
@amosbird I think the patch does address this issue. I also (if memory serves correctly) think that most checkers are already written to take advantage of columns, and it was the implementation of the primitives on which they relied that was failing to do so. So I think many checkers will get the better behavior "for free". That being said this code was written a while ago, and I think it ended up being written mostly by @cpitclaudel so he might be able to answer better than me. |
|
This will be very useful in flycheck-stan (https://github.com/stan-dev/stan-mode). I’m looking forward to it being merged. |
|
OK, I've refreshed a simplified version of this PR that shouldn't have backwards-compatibility issues; see #1676) |
|
That's great! After merging, that would be a good time to finally make a release. |
|
Is there any reason to keep this one open now that #1676 has been merged? |
|
I don't think so :) I'm working on further tweaks (adding error patterns for end-line and end-column and updating checkers) in a separate branch |
Addresses #527. Adds the ability to specify an error using buffer offsets.