rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.28k stars 12.71k forks source link

Diagnostic does not fire on marker trait that requires static #131683

Open jamesmunns opened 3 weeks ago

jamesmunns commented 3 weeks ago

Code

#[diagnostic::on_unimplemented(
    message = "Arguments to embassy tasks must be 'static",
    label = "Not 'static",
    note = "See https://embassy.dev/... for more details on sharing",
)]
trait SecretStatic {
}

// // Option A
impl<T: 'static> SecretStatic for T {

}
// //

// Option B
// impl SecretStatic for i8
// {
// }

// impl SecretStatic for u16
// {
// }

// impl SecretStatic for u32
// {
// }
//

fn expanded_func<T, U, V>(t: T, u: U, v: V)
where
    T: SecretStatic,
    U: SecretStatic,
    V: SecretStatic,
{
    todo!()
}

struct Example {
    x: u32,
}

fn main() {
    expanded_func(1i8, 2u16, 3u32);
    let x = Example { x: 123 };
    expanded_func::<&Example, u16, u32>(&x, 2u16, 3u32);
}

Current output

error[E0597]: `x` does not live long enough
  --> src/main.rs:45:41
   |
44 |     let x = Example { x: 123 };
   |         - binding `x` declared here
45 |     expanded_func::<&Example, u16, u32>(&x, 2u16, 3u32);
   |     ------------------------------------^^-------------
   |     |                                   |
   |     |                                   borrowed value does not live long enough
   |     argument requires that `x` is borrowed for `'static`
46 | }
   | - `x` dropped here while still borrowed

For more information about this error, try `rustc --explain E0597`.

Desired output

error[E0277]: Arguments to embassy tasks must be 'static
  --> src/main.rs:45:21
   |
45 |     expanded_func::<&Example, u16, u32>(&x, 2u16, 3u32);
   |                     ^^^^^^^^ Not 'static
   |
   = help: the trait `SecretStatic` is not implemented for `&Example`
   = note: See https://embassy.dev/... for more details on sharing
   = help: the following other types implement trait `SecretStatic`:
             i8
             u16
             u32
note: required by a bound in `expanded_func`
  --> src/main.rs:31:8
   |
29 | fn expanded_func<T, U, V>(t: T, u: U, v: V)
   |    ------------- required by a bound in this function
30 | where
31 |     T: SecretStatic,
   |        ^^^^^^^^^^^^ required by this bound in `expanded_func`

Rationale and extra context

We want to use a marker trait in macro expanded code for the embassy executor so that all arguments to tasks are 'static. This is a limitation of the executor, and is a common stumbling block for users.

We wanted to use diagnostics for this to reduce the amount of work required in a proc macro to get good spans and errors, but noticed that diagnostics did not fire at all. They DO fire when we manually add impls for the type instead of using a blanket impl (this is not practical to support for arbitrary user types

Other cases

No response

Rust Version

Tested with 1.81.0 stable and 1.82.0-beta.6, same output

Anything else?

No response

compiler-errors commented 3 weeks ago

diagnostic::on_unimplemented does not current work for borrow checker errors, for the record. Just trait errors. Borrowck happens after trait selection, where we currently lack the necessary information to currently issue on-unimplemented errors.

jamesmunns commented 3 weeks ago

@Dirbaio suggested this might just be a limitation of the trait solver:

when checking &'a Example: SecretStatic, the trait solver goes "okay there's an applicable impl, impl<T: 'static> SecretStatic for T, i'm going to use that one", and emits a lifetime constraint 'a: 'static that gets checked and fails later

Dirbaio commented 3 weeks ago

Would it make sense to track where the lifetime constraint "comes from" so that borrowck can emit the diagnostic if it came from that impl? (Or maybe it should be new kind of custom diagnostic `#[on_lifetime_error], because I imagine this can apply to more situations than trait impls)

compiler-errors commented 3 weeks ago

Would it make sense to track where the lifetime constraint "comes from" so that borrowck can emit the diagnostic if it came from that impl?

I fear this would be quite difficult to do given how we track lifetime constraints, particularly due to NLL.

Or maybe it should be new kind of custom diagnostic `#[on_lifetime_error], because I imagine this can apply to more situations than trait impls

Yeah, I think so. This isn't really an "unimplemented trait" per se, because lifetimes don't affect trait selection (mostly lol).