Implement Span::line() and Span::column() for proc-macro server #21394
Conversation
rustbot
added
the
S-waiting-on-review
|
Note that the implementations here are still unused by rust-analyzer as we are currently hashing out the concrete protocol for the bidirectional stuff. So this landing on stable will still take a while. Additionally, Will also need a rebase. |
crates/load-cargo/src/lib.rs
Outdated
| let text = db.file_text(file).text(db); | ||
| let line_index = ide_db::line_index::LineIndex::new(text); | ||
| let line_col = | ||
| line_index.try_line_col(ide_db::line_index::TextSize::new(offset)).unwrap(); |
There was a problem hiding this comment.
This unwrap can technically be hit if the proc-macro server is buggy so we should handle this failure instead of panicking within rust-analyzer
| fn line_column(&mut self, file_id: u32, offset: u32) -> Option<(usize, usize)> { | ||
| let req = bidirectional::BidirectionalMessage::SubRequest( | ||
| bidirectional::SubRequest::LineColumn { file_id, offset }, | ||
| ); | ||
|
|
||
| if req.write::<_, C>(&mut self.stdout.lock()).is_err() { | ||
| return None; | ||
| } | ||
|
|
||
| let msg = match bidirectional::BidirectionalMessage::read::<_, C>( | ||
| &mut self.stdin.lock(), | ||
| self.buf, | ||
| ) { | ||
| Ok(Some(msg)) => msg, | ||
| _ => return None, | ||
| }; | ||
|
|
||
| match msg { | ||
| bidirectional::BidirectionalMessage::SubResponse( | ||
| bidirectional::SubResponse::LineColumnResult { line, column }, | ||
| ) => Some((line, column)), | ||
| _ => None, | ||
| } | ||
| } |
There was a problem hiding this comment.
This can be simplified now (you'll notice when rebasing atop latest master)
| fn line(&mut self, span: Self::Span) -> usize { | ||
| let file_id = span.anchor.file_id.file_id().index(); | ||
| let offset: u32 = span.range.start().into(); | ||
| self.callback.as_mut().and_then(|cb| cb.line_column(file_id, offset)).map_or(1, |(l, _)| l) | ||
| } | ||
|
|
||
| fn column(&mut self, _span: Self::Span) -> usize { | ||
| // FIXME requires db to resolve line index, THIS IS NOT INCREMENTAL | ||
| 1 | ||
| fn column(&mut self, span: Self::Span) -> usize { | ||
| let file_id = span.anchor.file_id.file_id().index(); | ||
| let offset: u32 = span.range.start().into(); | ||
| self.callback.as_mut().and_then(|cb| cb.line_column(file_id, offset)).map_or(1, |(_, c)| c) |
There was a problem hiding this comment.
This is incorrect, the range in a span is relative to the anchored item (the AstId).
Which I also just realized means we have done the same mistake in source_text cc @Shourya742
It would probably make more sense to just sent the entire span over the protocol here instead of taking it apart.
There was a problem hiding this comment.
If we want to send spans over the protocol, they need to be serde-compatible. This came up while I was implementing the other file-path related methods, and it’s essentially the only requirement for putting spans on the wire. Do we want to make Span serde-enabled?
There was a problem hiding this comment.
Alternatively we can deconstruct them into their parts and send all of that over and reconstruct. Point is we need to send the ast-id over too as the range is otherwise not useful.
There was a problem hiding this comment.
I will update the source root text.
rustbot
removed
the
S-waiting-on-review
4 participants
Add proper line/column resolution for proc-macro spans via a callback mechanism. Previously these methods returned hardcoded 1 values.
The implementation adds:
SubRequest::LineColumn and SubResponse::LineColumnResult to the bidirectional protocol
ProcMacroClientInterface::line_column() method
Callback handling in load-cargo using LineIndex
Server implementation in RaSpanServer that uses the callback
This is in service of resolving an issue with Slint: slint-ui/slint#685
The Slint side now implement this also as per:
slint-ui/slint@13ce62d
Owing to my limited experience with Rust (and rust-analyzer) this was created with help from Claude. I have attempted to steer and review to the best of my ability and attempted sufficient test coverage.