Add fake return to improve span on type error in tracing::instrument

[compiler-errors]

Motivation

Return type errors on instrumented async functions are a bit vague, since the type error originates within the macro itself due to the indirection of additional async {} blocks generated in the proc-macro (and due to the way that inference propagates around in Rust).

This leads to a pretty difficult to understand error. For example:

#[instrument]
async fn foo() -> String {
  ""
}

results in...

error[E0308]: mismatched types
 --> src/main.rs:1:1
  |
1 | #[tracing::instrument]
  | ^^^^^^^^^^^^^^^^^^^^^^- help: try using a conversion method: `.to_string()`
  | |
  | expected struct `String`, found `&str`

Solution

Installs a fake return statement as the first thing that happens in the auto-generated block of an instrumented async function.

This causes the coercion machinery within rustc to infer the right return type (matching the the outer function) eagerly, as opposed to after the async {} block has been type-checked.

This will cause us to both be able to point out the return type span correctly, and properly suggest fixes on the expressions that cause the type mismatch.

After this change, the example code above compiles to:

error[E0308]: mismatched types
  --> src/main.rs:3:5
   |
3  |     ""
   |     ^^- help: try using a conversion method: `.to_string()`
   |     |
   |     expected struct `String`, found `&str`
   |
note: return type inferred to be `String` here
  --> src/main.rs:2:20
   |
2  | async fn foo() -> String {
   |                   ^^^^^^

[compiler-errors]

Note:

Is it possible to provide a UI test (like https://github.com/dtolnay/trybuild) for tracing macros? Otherwise, any recommendations on how to provide a test for this change, since all affected code is not expected to compile (and thus probably doesn't work with a traditional unit test)?

[hawkw]

Note:

Is it possible to provide a UI test (like https://github.com/dtolnay/trybuild) for tracing macros? Otherwise, any recommendations on how to provide a test for this change, since all affected code is not expected to compile (and thus probably doesn't work with a traditional unit test)?

it's fine to introduce a dev dependency on trybuild for testing this.

[compiler-errors]

it's fine to introduce a dev dependency on trybuild for testing this.

@davidbarsky just told me to do the same -- will do!

[compiler-errors]

Okay, think this is ready for review. Sorry for the force-pushes, wanted to make sure the history was clean.

[hawkw]

(as a side note, you don't need to force push to branches, as all pull requests will be merged by squashing. see https://github.com/tokio-rs/tracing/blob/master/CONTRIBUTING.md#commit-squashing for details.)

[compiler-errors]

Good to know. Guess I really should've read the contributing guide first, my bad.

[compiler-errors]

(sorry, that force push is because I am dumb and don't know how to use git)

[hawkw]

no worries, it's nto a big deal!

[davidbarsky]

mercurial + the diff tool encourages the equivalent of force pushes, but github punishes you for it, so no worries.

[compiler-errors]

also used to force-pushing when working on rustc because bors doesn't auto-squash 😸

[hawkw]

CI failure is a new clippy warning, unrelated to this change: https://github.com/tokio-rs/tracing/runs/7842367961?check_suite_focus=true I'll fix that on main.

[SanchithHegde]

Would it be too much to ask for an #[allow(clippy::unreachable)] to be added on line 67? (I can open a PR if necessary, or if anyone can directly make that change on master branch, that should be fine too.)

tracing/tracing-attributes/src/expand.rs

Lines 66 to 73 in 196e83e

	let fake_return_edge = quote_spanned! {return_span=>
	#[allow(unreachable_code, clippy::diverging_sub_expression, clippy::let_unit_value)]
	if false {
	let __tracing_attr_fake_return: #return_type =
	unreachable!("this is just for type inference, and is unreachable code");
	return __tracing_attr_fake_return;
	}
	};

We use #![warn(clippy::unreachable)] across our codebase and we now receive a load of clippy warnings about this unreachable! call after the tracing upgrade.

[compiler-errors]

I think opening a PR for this would be fine. Alternatively, one could do the same thing as #[async_trait] and use an alternative desugaring to provide the same return type hint: https://github.com/dtolnay/async-trait/blob/master/src/expand.rs#L374

Though please be careful that it handles return-position impl Trait correctly, since async_trait doesn't need to handle that, but this definitely does.