Skip to content

Commit

Permalink
Clamp Position::character to line length
Browse files Browse the repository at this point in the history
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 #18240
  • Loading branch information
krobelus committed Oct 5, 2024
1 parent 5982d9c commit 729b241
Show file tree
Hide file tree
Showing 6 changed files with 52 additions and 18 deletions.
14 changes: 2 additions & 12 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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" }
# la-arena = { path = "lib/la-arena" }
# lsp-server = { path = "lib/lsp-server" }

Expand Down Expand Up @@ -96,7 +96,7 @@ test-fixture = { path = "./crates/test-fixture" }
test-utils = { path = "./crates/test-utils" }

# In-tree crates that are published separately and follow semver. See lib/README.md
line-index = { version = "0.1.1" }
line-index = { version = "0.1.2" }
la-arena = { version = "0.3.1" }
lsp-server = { version = "0.7.6" }

Expand Down
16 changes: 13 additions & 3 deletions crates/rust-analyzer/src/lsp/from_proto.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,19 @@ pub(crate) fn offset(
.ok_or_else(|| format_err!("Invalid wide col offset"))?
}
};
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())
})?;
let (text_size, clamped_length) =
line_index.index.offset_clamped(line_col).ok_or_else(|| {
format_err!(
"Invalid offset {line_col:?} (line index length: {:?})",
line_index.index.len()
)
})?;
if let Some(clamped_length) = clamped_length {
tracing::error!(
"Position {line_col:?} column exceeds line length {}, clamping it",
u32::from(clamped_length),
);
}
Ok(text_size)
}

Expand Down
2 changes: 1 addition & 1 deletion lib/line-index/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "line-index"
version = "0.1.1"
version = "0.1.2"
description = "Maps flat `TextSize` offsets to/from `(line, column)` representation."
license = "MIT OR Apache-2.0"
repository = "https://github.com/rust-lang/rust-analyzer/tree/master/lib/line-index"
Expand Down
16 changes: 16 additions & 0 deletions lib/line-index/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,22 @@ impl LineIndex {
self.start_offset(line_col.line as usize).map(|start| start + TextSize::from(line_col.col))
}

/// Transforms the `LineCol` into a `TextSize`. If the column exceeds the line length,
/// the line, it is clamped to that.
pub fn offset_clamped(&self, line_col: LineCol) -> Option<(TextSize, Option<TextSize>)> {
self.start_offset(line_col.line as usize).map(|start| {
let next_newline =
self.newlines.get(line_col.line as usize).copied().unwrap_or(self.len);
let line_length = next_newline - start;
let col = TextSize::from(line_col.col);
if col > line_length {
(start + line_length, Some(line_length))
} else {
(start + col, None)
}
})
}

fn start_offset(&self, line: usize) -> Option<TextSize> {
match line.checked_sub(1) {
None => Some(TextSize::from(0)),
Expand Down
18 changes: 18 additions & 0 deletions lib/line-index/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,3 +195,21 @@ fn test_every_chars() {
}
}
}

#[test]
fn test_offset_clamped() {
macro_rules! validate {
($text:expr, $line:expr, $col:expr, $expected:expr) => {
let line_index = LineIndex::new($text);
let line_col = LineCol { line: $line, col: $col };
assert_eq!(line_index.offset_clamped(line_col), $expected);
};
}

validate!("", 0, 0, Some((TextSize::new(0), None)));
validate!("", 0, 1, Some((TextSize::new(0), Some(TextSize::new(0)))));
validate!("\n", 1, 0, Some((TextSize::new(1), None)));
validate!("\nabc", 1, 1, Some((TextSize::new(2), None)));
validate!("\nabc", 1, 3, Some((TextSize::new(4), None)));
validate!("\nabc", 1, 4, Some((TextSize::new(4), Some(TextSize::new(3)))));
}

0 comments on commit 729b241

Please sign in to comment.