Skip to content

Fix/Make proc_macro span_close() and span_open() more accurate after set_span() calls #149052

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.

Sign up for GitHub

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

Open
Keith-Cancel wants to merge 2 commits into
base: main
Choose a base branch
from
+42 −2
Open

Fix/Make proc_macro span_close() and span_open() more accurate after set_span() calls #149052

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b8cab94
refresh labels
Keith-Cancel Nov 25, 2025
d255ff6
Make `Group::span_open()` and Group::span_close()` more accurate afte…
Keith-Cancel Nov 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 42 additions & 2 deletions library/proc_macro/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ mod to_tokens;

use core::ops::BitOr;
use std::ffi::CStr;
use std::ops::{Range, RangeBounds};
use std::ops::{Bound, Range, RangeBounds};
use std::path::PathBuf;
use std::str::FromStr;
use std::{error, fmt};
Expand Down Expand Up @@ -880,7 +880,47 @@ impl Group {
/// tokens at the level of the `Group`.
#[stable(feature = "proc_macro_lib2", since = "1.29.0")]
pub fn set_span(&mut self, span: Span) {
self.0.span = bridge::DelimSpan::from_single(span.0);
let (open_chr, close_chr) = match self.0.delimiter {
Delimiter::Brace => (b'{', b'}'),
Delimiter::Bracket => (b'[', b']'),
Delimiter::Parenthesis => (b'(', b')'),
Delimiter::None => {
// None delimiters are the whole span for open and close
// same behavior as in the compiler.
self.0.span = bridge::DelimSpan::from_single(span.0);
return;
}
};

// No source text to check or source text is too small.
// just go back to default behavior.
let source = match span.0.source_text() {
Copy link
Member

@Veykril Veykril Nov 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like quite the expensive operation to call here. For one this goes through the bridge (which is expensive for rust-analyzer, especially this call, as once we implement it will always do an RPC roundtrip), but that aside, this also always allocates a new string.

Copy link
Contributor

@petrochenkov petrochenkov Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The allocation and the further bridge calls to subspan can be avoided by moving this whole logic into the compiler's side of the bridge.

Copy link
Contributor

@petrochenkov petrochenkov Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative solution is to just add fn set_span_(open,close) to the API.
Typically the user of set_span knows whether the passed span actually contains the group's source text, and if it's known, then set_span_(open,close) can be called as well.
I don't suspect it happens often, I'd expect set_span to be set to something semi-related from the macro input.

If we do this, then #149229 probably shouldn't be merged.

Copy link
Contributor

@petrochenkov petrochenkov Nov 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do this, then #149229 probably shouldn't be merged.

Or it can be merged if set_span_open changes only the opening span, set_span_close changes only the closing span, and set_span changes both to the same value.

This would also solve the question of what goes back to the compiler when proc_macro::Span is converted to compiler::Span.

Copy link
Author

@Keith-Cancel Keith-Cancel Nov 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do this, then #149229 probably shouldn't be merged.

Or it can be merged if set_span_open changes only the opening span, set_span_close changes only the closing span, and set_span changes both to the same value.

This would also solve the question of what goes back to the compiler when proc_macro::Span is converted to compiler::Span.

A set_span_open() and set_span_close(), only effecting their respective fields and span set_span() overwriting both seems fine. Also if we want to add those API or similar API, #149229 makes even more sense because the entire field becomes more awkward with those proposed API.

Although for a public API we probably want that API to be set_span_open_close(Span, Span) to at least to make sure the spans are from the same file.

Copy link
Author

@Keith-Cancel Keith-Cancel Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The allocation and the further bridge calls to subspan can be avoided by moving this whole logic into the compiler's side of the bridge.

That is a good point and certainly do able.

Copy link
Contributor

@petrochenkov petrochenkov Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to at least to make sure the spans are from the same file

That also requires going through the bridge though.

Some(s) if s.len() >= 2 => s,
_ => {
self.0.span = bridge::DelimSpan::from_single(span.0);
return;
}
};
let src = source.as_bytes();

// Make sure that the last and first characters match up
if src[0] != open_chr || src[src.len() - 1] != close_chr {
self.0.span = bridge::DelimSpan::from_single(span.0);
return;
}

// Compute the spans.
let Some(open) = span.0.subspan(Bound::Included(0), Bound::Excluded(1)) else {
self.0.span = bridge::DelimSpan::from_single(span.0);
return;
};

let Some(close) = span.0.subspan(Bound::Included(src.len() - 1), Bound::Unbounded) else {
self.0.span = bridge::DelimSpan::from_single(span.0);
return;
};

self.0.span = bridge::DelimSpan { open, close, entire: span.0 };
}
}

Expand Down
Loading