Skip to content

Conversation

ChayimFriedman2
Copy link
Contributor

When working on Something Else (TM) (I left a hint in the commits :P), I noticed to my surprise that labels inside macros are not resolved. This led to a discovery of two unrelated bugs, which are hereby fixed in two commits.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 29, 2024
@@ -36,7 +36,7 @@ macro_rules! f {
}
struct#0:1@58..64#1# MyTraitMap2#0:2@31..42#0# {#0:1@72..73#1#
map#0:1@86..89#1#:#0:1@89..90#1# #0:1@89..90#1#::#0:1@91..92#1#std#0:1@93..96#1#::#0:1@96..97#1#collections#0:1@98..109#1#::#0:1@109..110#1#HashSet#0:1@111..118#1#<#0:1@118..119#1#(#0:1@119..120#1#)#0:1@120..121#1#>#0:1@121..122#1#,#0:1@122..123#1#
map#0:1@86..89#1#:#0:1@89..90#1# #0:1@89..90#1#::#0:1@91..93#1#std#0:1@93..96#1#::#0:1@96..98#1#collections#0:1@98..109#1#::#0:1@109..111#1#HashSet#0:1@111..118#1#<#0:1@118..119#1#(#0:1@119..120#1#)#0:1@120..121#1#>#0:1@121..122#1#,#0:1@122..123#1#
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll save the reviewer the check (and the headache): this merges the span of the two colons in ::, appearing three times. Previously the span pointed to the first colon only, now it points to both.

Comment on lines +1057 to +1073
fn merge_spans(a: SpanData<Ctx>, b: SpanData<Ctx>) -> SpanData<Ctx> {
// We don't do what rustc does exactly, rustc does something clever when the spans have different syntax contexts
// but this runs afoul of our separation between `span` and `hir-expand`.
SpanData {
range: if a.ctx == b.ctx {
TextRange::new(
std::cmp::min(a.range.start(), b.range.start()),
std::cmp::max(a.range.end(), b.range.end()),
)
} else {
// Combining ranges make no sense when they come from different syntax contexts.
a.range
},
anchor: a.anchor,
ctx: a.ctx,
}
}
Copy link
Member

Choose a reason for hiding this comment

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

fyi we already have this for the proc-macro server (with a bit more complications to handle the fixup stuff)

fn join(&mut self, first: Self::Span, second: Self::Span) -> Option<Self::Span> {
// We can't modify the span range for fixup spans, those are meaningful to fixup, so just
// prefer the non-fixup span.
if first.anchor.ast_id == FIXUP_ERASED_FILE_AST_ID_MARKER {
return Some(second);
}
if second.anchor.ast_id == FIXUP_ERASED_FILE_AST_ID_MARKER {
return Some(first);
}
// FIXME: Once we can talk back to the client, implement a "long join" request for anchors
// that differ in [AstId]s as joining those spans requires resolving the AstIds.
if first.anchor != second.anchor {
return None;
}
// Differing context, we can't merge these so prefer the one that's root
if first.ctx != second.ctx {
if first.ctx.is_root() {
return Some(second);
} else if second.ctx.is_root() {
return Some(first);
}
}
Some(Span {
range: first.range.cover(second.range),
anchor: second.anchor,
ctx: second.ctx,
})
}

Not too relevant (as you pointed out, the separation gives us trouble), just wanted to point it out

…esolve them textually, instead reuse the results of HIR lowering

This fixes a bug where labels inside macros were not resolved, but more importantly this prepares us to a future where we have hygiene, and textual equivalence isn't enough to resolve identifiers.
@ChayimFriedman2
Copy link
Contributor Author

@Veykril Addressed comments.

@Veykril
Copy link
Member

Veykril commented Sep 30, 2024

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Sep 30, 2024

📌 Commit cd7cbdd has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 30, 2024

⌛ Testing commit cd7cbdd with merge ac8509a...

@bors
Copy link
Contributor

bors commented Sep 30, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing ac8509a to master...

@bors bors merged commit ac8509a into rust-lang:master Sep 30, 2024
11 checks passed
@ChayimFriedman2 ChayimFriedman2 deleted the label-macro branch October 6, 2024 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants