Clamp Position::character to line length #18243
Conversation
rustbot
added
the
S-waiting-on-review
| ) | ||
| })?; | ||
| if let Some(clamped_length) = clamped_length { | ||
| tracing::error!( |
There was a problem hiding this comment.
If LSP allows that, should we warn at it?
The answer probably lies in whether there are some client that deliberately sends longer line lengths. But if not, then maybe we don't want to merge this...
There was a problem hiding this comment.
The answer probably lies in whether there are some client that deliberately sends longer line lengths
Indirectly yes. But the more direct answer is in the specification (unless we want to deliberately deviate from that which would be surprising).
Ideally I'd prefer following the spec today but also remove this feature from the spec later (assuming no client needs it).
Since it's allowed by the spec I agree we shouldn't error. But I think a warning would be appropriate; if nothing else it will help us find out which client wants to use this feature.
Assuming we want to warn, we can define our own LSP protocol extension that clients can opt into.
Say we call it strict-mode.
Then we can suppress the warning by default and only print it if strict mode is enabled.
LSP is sometimes a bit underspecified, meaning that servers and clients have to implement multiple different ways the protocol could be interpreted. We could use a strict mode as a means to eventually simplify the spec..
I'd also be fine with simply dropping the warning but I guess I have a slight preference for keeping it.
This crash is not necessarily an important issue but I believe I've hit it occasionally over a few years and didn't notice the crash because my client restarts rust-analyzer automatically on crash. I got better error propagation set up now.
There was a problem hiding this comment.
I'd say lets keep the log for now, and see if any client is being spammy with this or not
We'll need to split the PR, iirc our CI auto publishes the library folder if the versions change so we need the library change first, then a separate PR to bump the dependency. |
krobelus
force-pushed
the
clamp-position-character
branch
from
0c22997
to
729b241
Compare
Cargo.toml
Outdated
| @@ -40,7 +40,7 @@ debug = 2 | |||
| # chalk-ir = { path = "../chalk/chalk-ir" } | |||
| # chalk-recursive = { path = "../chalk/chalk-recursive" } | |||
| # chalk-derive = { path = "../chalk/chalk-derive" } | |||
| # line-index = { path = "lib/line-index" } | |||
| line-index = { path = "lib/line-index" } | |||
There was a problem hiding this comment.
Please split this PR up into two, one changing the lib/line-index crate, that we then merge and have a patch release done by. Then we can rebase this PR on top to use the new release (or swap things, have this PR contain the lib changes and do a follow up to bump the line-index dep usage)
lib/line-index/src/lib.rs
Outdated
| /// Transforms the `LineCol` into a `TextSize`. If the column exceeds the line length, | ||
| /// the line, it is clamped to that. |
There was a problem hiding this comment.
Might be good to describe the return types here then, its not immediately obvious what they mean
krobelus
force-pushed
the
clamp-position-character
branch
from
729b241
to
cf0c848
Compare
lib/line-index/src/lib.rs
Outdated
| pub fn lines(&self, range: TextRange) -> impl Iterator<Item = TextRange> + '_ { | ||
| pub fn lines_in_range(&self, range: TextRange) -> impl Iterator<Item = TextRange> + '_ { |
There was a problem hiding this comment.
This is a semver breaking change to the signatures so we'd need to release a 0.2.0 release then if we rename this function, should be fine to keep it as lines
krobelus
force-pushed
the
clamp-position-character
branch
from
cf0c848
to
d42a4e6
Compare
|
Thanks! |
|
Ah CI is still borked (not from your PR) |
Clamp Position::character to line length LSP says about Position::character > If the character value is greater than the line length it defaults back to the line length. but from_proto::offset() doesn't implement this. A client might for example request code actions for a whole line by sending Position::character=99999. I don't think there is ever a reason (besides laziness) why the client can't specify the line length instead but I guess we should not crash but follow protocol. Not sure how to update Cargo.lock (lib/README.md doesn't say how). Fixes #18240
|
💔 Test failed - checks-actions |
|
@bors r+ |
|
☀️ Test successful - checks-actions |
Clamp Position::character to line length 2/2 Completes #18243 I don't think I have permissions to target this on the other PR, so we'll need to rebase manually
…r=Veykril Clamp Position::character to line length 2/2 Completes rust-lang/rust-analyzer#18243 I don't think I have permissions to target this on the other PR, so we'll need to rebase manually
LSP says about Position::character
but from_proto::offset() doesn't implement this.
A client might for example request code actions for a whole line by sending
Position::character=99999. I don't think there is ever a reason (besides laziness) why the
client can't specify the line length instead but I guess we should not crash but follow protocol.
Not sure how to update Cargo.lock (lib/README.md doesn't say how).
Fixes #18240