Hovering any ident a function body of an `#[async_trait]` function shows `Box`

[Veykril]

The cause for that is the re-spanning of the macro here https://github.com/dtolnay/async-trait/blob/master/src/expand.rs, but its using the brace spans (which cover the entire body), it feels like we shouldn't even be considering them at all?

It might be that this is not actionable but its something we should investigate whether we can make work better, as the macro has all the rights to do this for better diagnostics.

[ChayimFriedman2]

This is indeed weird, are you sure that's the cause?

[Veykril]

I can't think of anything else, and we do show Box::pin as well so it must be that

[ChayimFriedman2]

Yes, it's this (verified by modifying the macro).

[ChayimFriedman2]

Investigating this a bit, that happens because the group's span contains the entire block, and when we map down spans, we consider all spans that contain this span:

rust-analyzer/crates/span/src/map.rs

Lines 85 to 87 in 7b65a45

	if !s.range.contains_range(span.range) {
	return None;
	}

[Veykril]

Right, but I don't think we want to change that as otherwise people joining spans can generally cause us to miss some token mappings :/

[ChayimFriedman2]

I'm not actually sure. I feel at least hover should focus on the precise span; joint spans are unlikely to be an exact match. I think we would benefit from seeing for what span joining is used in the wild (also, it's unstable).

[Veykril]

Ah it's still unstable? Yea then maybe its fine.

[ChayimFriedman2]

Well, eventually it will become stable. Maybe.

[ChayimFriedman2]

I implemented this, and there is one minor improvement in a semantic highlighting test, but there is also one regression:

rust-analyzer/crates/ide/src/goto_definition.rs

Lines 2187 to 2205 in b2a58b8

	#[test]
	fn goto_include_has_eager_input() {
	check(
	r#"
	//- /main.rs
	#[rustc_builtin_macro]
	macro_rules! include_str {}
	#[rustc_builtin_macro]
	macro_rules! concat {}
	fn main() {
	let str = include_str!(concat!("foo", ".tx$0t"));
	}
	//- /foo.txt
	// empty
	//^file
	"#,
	);
	}

Here we have include!(concat!("foo", ".txt")). The concat!() produces a span that includes both its operands, so this fails the new check.

If I had to explain the conceptual difference between this (wanted case) and the unwanted async-trait case, I'd say the difference is that here the span of the string is actually multiple spans, while in the block case it was just encompassing multiple tokens. That does explain the difference but it's not something we can code: coding the "made of multiple spans" span, besides complicating the code, will also increase span size by a lot which will impact memory usage by a lot, I don't think we can allow ourselves to do that.

So I'm inclined to push this change anyway as the async-trait (and other) cases seem more important than this minor case; @Veykril what do you think?

[lnicola]

Would that break e.g. include!(concat!(env!("OUT_DIR"), "/bindings.rs"));? I'm not sure that's a minor case.

[Veykril]

Yea I agree that seems not great to regress with. Note that the async trait issue is not the worst for hover, but go to def becomes annoying in that it brings you to Box::pin as well. I wonder if there is something else we can do specifically when something gets respawned with delimiter pair spans 🤔