Include diagnostics when triggering codeAction request on save #1896
Conversation
Some servers might apply different logic depending on provided diagnostics. For example theia server might not want to apply destructive action like removing import statements when there are error diagnostics. While testing this new logic, uncovered a bug where we didn't include diagnostics "touching" the selection region which also affected the case when the we get diagnostics for the whole document range on save which didn't include diagnostics that were at the end of the document. Fixed by using `Range`s for intersection checking rather than ST's native `Region`.
| if region.intersects(candidate): | ||
| # Perform intersection checks on Range since it's inclusive unlike Region which is exclusive. | ||
| diagnostic_range = region_to_range(self.view, candidate) | ||
| if requested_range.intersects(diagnostic_range): | ||
| covering = covering.cover(candidate) | ||
| intersections.append(diagnostic) | ||
| if intersections: |
There was a problem hiding this comment.
This whole function (and related) could probably be refactored to work on Range exclusively but that's too much of a job for now.
There was a problem hiding this comment.
And another thing that came through my mind when working on this...
It's not entirely correct to get diagnostics from the SessionBuffer for the code-actions-on-save functionality since diagnostics can be delayed by our debouncing logic (diagnostics_delay_ms and co.). I believe we should instead be getting them from the new DiagnosticsManager. Would only have to figure out if it's safe since it doesn't store version of the diagnostics and we don't want to send outdated diagnostics.
There was a problem hiding this comment.
My idea was to convert LSP ranges to ST regions as soon as possible, and work with ST regions as much as possible. And then remove the Range/Point classes. I think this can also be solved by calling diagnostics_touching_point somewhere alongside diagnostics_intersecting_region if the region turns out to be (n, n) instead of (n, m) with n < m.
It seems like you prefer working with LSP ranges instead. That's fine, but we should make a decision going forward. I think both have their pros and cons.
Pros of using ST regions:
- Less maintenance
- ST API functions expect ST regions
- Converting an LSP range into an ST region depends on the change_count of the view... so we must do that at the point in time where the LSP range's change_count is still equal to the view's change_count
Cons of using ST regions:
- Has different behavior for single points and regions (n, m) with n < m
Pros of using LSP ranges:
- Has nice behavior whether regions are points or not
- When sending back diagnostics for code actions, there is no need to convert ST regions to LSP ranges
Cons of using LSP ranges:
- A bit more maintenance
- Requires conversion to an ST region when wanting to use it in an ST API function. The conversion depends on the view's change_count
There is much confusion about the implementation of ST regions, and I don't totally agree with the ideas/concepts, but it is what it is. sublimehq/sublime_text#3837
Before I think about it more, are you suggesting that this is only an issue when region we are checking against is (n, n)? Because it's not. If we have selection region (1, 5) then this doesn't intersect with diagnostic at (5, 6) if we use Region for checking. |
|
Changed to use |
Some servers might apply different logic depending on provided
diagnostics. For example theia server might not want to apply
destructive action like removing import statements when there are
error diagnostics.
While testing this new logic, uncovered a bug where we didn't include
diagnostics "touching" the selection region which also affected the
case when getting diagnostics for the whole document range on save,
which didn't include diagnostics that were at the end of the document.