Skip to content

Position.character is not clamped to line length #18240

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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
krobelus opened this issue Oct 5, 2024 · 1 comment · Fixed by #18243
Closed

Position.character is not clamped to line length #18240

krobelus opened this issue Oct 5, 2024 · 1 comment · Fixed by #18243
Labels
A-lsp LSP conformance issues and missing features

Comments

@krobelus
Copy link
Contributor

krobelus commented Oct 5, 2024

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() doesnt' implement this

    let text_size = line_index.index.offset(line_col).ok_or_else(|| {
        format_err!("Invalid offset {line_col:?} (line index length: {:?})", line_index.index.len())
    })?;

I hit this because rust-analyzer crashes with the kakoune-lsp client because -- if there is no code action in the selection -- this client falls back to requesting code actions for the whole line by simply specifying Position::character=1e6.
I worked around the crash by sending the actual line length instead.

I'm not yet sure what we should do, following the protocol can help but the downside is that we're masking potential bugs

I haven't yet found a case where a client inherently needs this behavior, so it doesn't seem important.

@Veykril Veykril added the A-lsp LSP conformance issues and missing features label Oct 5, 2024
@Veykril
Copy link
Member

Veykril commented Oct 5, 2024

We can always clamp and just log an error when that happens instead of returning one

krobelus added a commit to kakoune-lsp/kakoune-lsp that referenced this issue Oct 5, 2024
…whole line

When the selections don't contain a code action, we fall back to
requesting code actions for the whole line. This may be unexpected
but it is our way of making up for the lack of per-line lightbulbs.

Instead of computing the actual line end, use EOL_OFFSET=1e6.
This makes rust-analyzer panic with "Bad offset: range 0..97493 offset
1017440" in certain scenarios.  LSP says on Position::character:

> /// If the character value is greater than the line length it
> /// defaults back to the line length.

Work around this until we can expect this to be fixed.

Ref rust-lang/rust-analyzer#18240
krobelus added a commit to krobelus/rust-analyzer that referenced this issue Oct 5, 2024
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 the lockfile (lib/README.md doesn't say how).

Fixes rust-lang#18240
krobelus added a commit to krobelus/rust-analyzer that referenced this issue Oct 5, 2024
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 the lockfile (lib/README.md doesn't say how).

Fixes rust-lang#18240
krobelus added a commit to krobelus/rust-analyzer that referenced this issue Oct 5, 2024
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 the lockfile (lib/README.md doesn't say how).

Fixes rust-lang#18240
krobelus added a commit to krobelus/rust-analyzer that referenced this issue Oct 15, 2024
krobelus added a commit to krobelus/rust-analyzer that referenced this issue Oct 15, 2024
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.

Technically it should be a warning, not an error but warning is not shown by default so keep
it at error I guess.

Fixes rust-lang#18240
krobelus added a commit to krobelus/rust-analyzer that referenced this issue Oct 15, 2024
bors added a commit that referenced this issue Oct 18, 2024
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
@bors bors closed this as completed in 3bda0cc Oct 18, 2024
krobelus added a commit to krobelus/rust-analyzer that referenced this issue Oct 18, 2024
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.

Technically it should be a warning, not an error but warning is not shown by default so keep
it at error I guess.

Fixes rust-lang#18240
Wilfred pushed a commit to Wilfred/rust-analyzer that referenced this issue Oct 22, 2024
Wilfred pushed a commit to Wilfred/rust-analyzer that referenced this issue Oct 22, 2024
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.

Technically it should be a warning, not an error but warning is not shown by default so keep
it at error I guess.

Fixes rust-lang#18240
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lsp LSP conformance issues and missing features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants