Make `?` and `%` formatters in `kv!` more flexible #170

panicbit · 2018-06-19T00:03:37Z

This allows using ? and % formatters when creating new loggers,
e.g. logger.new(o!("foo" => ?bar)).

Note that this is probably a breaking change
as ? and % don't borrow implicitly anymore.

This change is

dpc · 2018-06-19T01:36:57Z

Would &FmtDisplay<'a>(pub &'a T) be a way for it not to be a breaking change?

panicbit · 2018-06-19T01:38:58Z

I had the same idea, but the issue is the same as with format_args (having a borrow in the kv map).

panicbit · 2018-06-19T01:41:43Z

The non-breaking alternative would be allocating a String, which is less ideal since it's not necessary for log! invocations.

dpc · 2018-06-19T02:13:53Z

What's wrong with https://github.com/slog-rs/slog/tree/kv_fmt ?

dpc · 2018-06-19T02:14:34Z

8d3d34b I mean

dpc · 2018-06-19T02:15:20Z

I still don't understand what is this improving etc. so please add some tests that used to fail, and now are passing. :)

panicbit · 2018-06-19T09:23:02Z

I've added a test now.

Btw, this is the massive and confusing error you'd get if you try to use ? or % when an owned kv map is required (without this patch):

error[E0277]: `*mut core::ops::Fn() + 'static` cannot be shared between threads safely
   --> src/lib.rs:363:9
    |
363 |         $crate::OwnedKV(kv!($($args)*))
    |         ^^^^^^^^^^^^^^^ `*mut core::ops::Fn() + 'static` cannot be shared between threads safely
    | 
   ::: src/tests.rs:255:27
    |
255 |     Logger::root(Discard, o!("foo" => ?n));
    |                           --------------- in this macro invocation
    |
    = help: within `[core::fmt::ArgumentV1<'_>]`, the trait `core::marker::Sync` is not implemented for `*mut core::ops::Fn() + 'static`
    = note: required because it appears within the type `core::marker::PhantomData<*mut core::ops::Fn() + 'static>`
    = note: required because it appears within the type `core::fmt::Void`
    = note: required because it appears within the type `&core::fmt::Void`
    = note: required because it appears within the type `core::fmt::ArgumentV1<'_>`
    = note: required because it appears within the type `[core::fmt::ArgumentV1<'_>]`
    = note: required because of the requirements on the impl of `core::marker::Send` for `&[core::fmt::ArgumentV1<'_>]`
    = note: required because it appears within the type `core::fmt::Arguments<'_>`
    = note: required because it appears within the type `SingleKV<core::fmt::Arguments<'_>>`
    = note: required because it appears within the type `(SingleKV<core::fmt::Arguments<'_>>, ())`
    = note: required because of the requirements on the impl of `SendSyncRefUnwindSafeKV` for `(SingleKV<core::fmt::Arguments<'_>>, ())`
note: required by `OwnedKV`
   --> src/lib.rs:3081:1
    |
3081| / pub struct OwnedKV<T>(
3082| |     #[doc(hidden)]
3083| |     /// The exact details of that it are not considered public
3084| |     /// and stable API. `slog_o` or `o` macro should be used
...   |
3088| | where
3089| |     T: SendSyncRefUnwindSafeKV + ?Sized;
    | |________________________________________^

error[E0277]: the trait bound `core::ops::Fn() + 'static: std::panic::RefUnwindSafe` is not satisfied in `(SingleKV<core::fmt::Arguments<'_>>, ())`
   --> src/lib.rs:363:9
    |
363 |         $crate::OwnedKV(kv!($($args)*))
    |         ^^^^^^^^^^^^^^^ the type core::ops::Fn() + 'static may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
    | 
   ::: src/tests.rs:255:27
    |
255 |     Logger::root(Discard, o!("foo" => ?n));
    |                           --------------- in this macro invocation
    |
    = help: within `(SingleKV<core::fmt::Arguments<'_>>, ())`, the trait `std::panic::RefUnwindSafe` is not implemented for `core::ops::Fn() + 'static`
    = note: required because it appears within the type `*mut core::ops::Fn() + 'static`
    = note: required because it appears within the type `core::marker::PhantomData<*mut core::ops::Fn() + 'static>`
    = note: required because it appears within the type `core::fmt::Void`
    = note: required because it appears within the type `&core::fmt::Void`
    = note: required because it appears within the type `core::fmt::ArgumentV1<'_>`
    = note: required because it appears within the type `[core::fmt::ArgumentV1<'_>]`
    = note: required because it appears within the type `&[core::fmt::ArgumentV1<'_>]`
    = note: required because it appears within the type `core::fmt::Arguments<'_>`
    = note: required because it appears within the type `SingleKV<core::fmt::Arguments<'_>>`
    = note: required because it appears within the type `(SingleKV<core::fmt::Arguments<'_>>, ())`
    = note: required because of the requirements on the impl of `SendSyncRefUnwindSafeKV` for `(SingleKV<core::fmt::Arguments<'_>>, ())`
note: required by `OwnedKV`
   --> src/lib.rs:3081:1
    |
3081| / pub struct OwnedKV<T>(
3082| |     #[doc(hidden)]
3083| |     /// The exact details of that it are not considered public
3084| |     /// and stable API. `slog_o` or `o` macro should be used
...   |
3088| | where
3089| |     T: SendSyncRefUnwindSafeKV + ?Sized;
    | |________________________________________^

error[E0277]: `*mut core::ops::Fn() + 'static` cannot be shared between threads safely
   --> src/lib.rs:363:9
    |
363 |         $crate::OwnedKV(kv!($($args)*))
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `*mut core::ops::Fn() + 'static` cannot be shared between threads safely
    | 
   ::: src/tests.rs:255:27
    |
255 |     Logger::root(Discard, o!("foo" => ?n));
    |                           --------------- in this macro invocation
    |
    = help: within `[core::fmt::ArgumentV1<'_>]`, the trait `core::marker::Sync` is not implemented for `*mut core::ops::Fn() + 'static`
    = note: required because it appears within the type `core::marker::PhantomData<*mut core::ops::Fn() + 'static>`
    = note: required because it appears within the type `core::fmt::Void`
    = note: required because it appears within the type `&core::fmt::Void`
    = note: required because it appears within the type `core::fmt::ArgumentV1<'_>`
    = note: required because it appears within the type `[core::fmt::ArgumentV1<'_>]`
    = note: required because of the requirements on the impl of `core::marker::Send` for `&[core::fmt::ArgumentV1<'_>]`
    = note: required because it appears within the type `core::fmt::Arguments<'_>`
    = note: required because it appears within the type `SingleKV<core::fmt::Arguments<'_>>`
    = note: required because it appears within the type `(SingleKV<core::fmt::Arguments<'_>>, ())`
    = note: required because of the requirements on the impl of `SendSyncRefUnwindSafeKV` for `(SingleKV<core::fmt::Arguments<'_>>, ())`
note: required by `OwnedKV`
   --> src/lib.rs:3081:1
    |
3081| / pub struct OwnedKV<T>(
3082| |     #[doc(hidden)]
3083| |     /// The exact details of that it are not considered public
3084| |     /// and stable API. `slog_o` or `o` macro should be used
...   |
3088| | where
3089| |     T: SendSyncRefUnwindSafeKV + ?Sized;
    | |________________________________________^

error[E0277]: the trait bound `core::ops::Fn() + 'static: std::panic::RefUnwindSafe` is not satisfied in `(SingleKV<core::fmt::Arguments<'_>>, ())`
   --> src/lib.rs:363:9
    |
363 |         $crate::OwnedKV(kv!($($args)*))
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the type core::ops::Fn() + 'static may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
    | 
   ::: src/tests.rs:255:27
    |
255 |     Logger::root(Discard, o!("foo" => ?n));
    |                           --------------- in this macro invocation
    |
    = help: within `(SingleKV<core::fmt::Arguments<'_>>, ())`, the trait `std::panic::RefUnwindSafe` is not implemented for `core::ops::Fn() + 'static`
    = note: required because it appears within the type `*mut core::ops::Fn() + 'static`
    = note: required because it appears within the type `core::marker::PhantomData<*mut core::ops::Fn() + 'static>`
    = note: required because it appears within the type `core::fmt::Void`
    = note: required because it appears within the type `&core::fmt::Void`
    = note: required because it appears within the type `core::fmt::ArgumentV1<'_>`
    = note: required because it appears within the type `[core::fmt::ArgumentV1<'_>]`
    = note: required because it appears within the type `&[core::fmt::ArgumentV1<'_>]`
    = note: required because it appears within the type `core::fmt::Arguments<'_>`
    = note: required because it appears within the type `SingleKV<core::fmt::Arguments<'_>>`
    = note: required because it appears within the type `(SingleKV<core::fmt::Arguments<'_>>, ())`
    = note: required because of the requirements on the impl of `SendSyncRefUnwindSafeKV` for `(SingleKV<core::fmt::Arguments<'_>>, ())`
note: required by `OwnedKV`
   --> src/lib.rs:3081:1
    |
3081| / pub struct OwnedKV<T>(
3082| |     #[doc(hidden)]
3083| |     /// The exact details of that it are not considered public
3084| |     /// and stable API. `slog_o` or `o` macro should be used
...   |
3088| | where
3089| |     T: SendSyncRefUnwindSafeKV + ?Sized;
    | |________________________________________^

error[E0277]: `*mut core::ops::Fn() + 'static` cannot be shared between threads safely
   --> src/tests.rs:255:5
    |
255 |     Logger::root(Discard, o!("foo" => ?n));
    |     ^^^^^^^^^^^^ `*mut core::ops::Fn() + 'static` cannot be shared between threads safely
    |
    = help: within `[core::fmt::ArgumentV1<'_>]`, the trait `core::marker::Sync` is not implemented for `*mut core::ops::Fn() + 'static`
    = note: required because it appears within the type `core::marker::PhantomData<*mut core::ops::Fn() + 'static>`
    = note: required because it appears within the type `core::fmt::Void`
    = note: required because it appears within the type `&core::fmt::Void`
    = note: required because it appears within the type `core::fmt::ArgumentV1<'_>`
    = note: required because it appears within the type `[core::fmt::ArgumentV1<'_>]`
    = note: required because of the requirements on the impl of `core::marker::Send` for `&[core::fmt::ArgumentV1<'_>]`
    = note: required because it appears within the type `core::fmt::Arguments<'_>`
    = note: required because it appears within the type `SingleKV<core::fmt::Arguments<'_>>`
    = note: required because it appears within the type `(SingleKV<core::fmt::Arguments<'_>>, ())`
    = note: required because of the requirements on the impl of `SendSyncRefUnwindSafeKV` for `(SingleKV<core::fmt::Arguments<'_>>, ())`
note: required by `<Logger<D>>::root`
   --> src/lib.rs:1103:5
    |
1103| /     pub fn root<T>(drain: D, values: OwnedKV<T>) -> Logger
1104| |     where
1105| |         D: 'static + SendSyncRefUnwindSafeDrain<Err = Never, Ok = ()>,
1106| |         T: SendSyncRefUnwindSafeKV + 'static,
...   |
1112| |         }
1113| |     }
    | |_____^

error[E0277]: the trait bound `core::ops::Fn() + 'static: std::panic::RefUnwindSafe` is not satisfied in `(SingleKV<core::fmt::Arguments<'_>>, ())`
   --> src/tests.rs:255:5
    |
255 |     Logger::root(Discard, o!("foo" => ?n));
    |     ^^^^^^^^^^^^ the type core::ops::Fn() + 'static may contain interior mutability and a reference may not be safely transferrable across a catch_unwind boundary
    |
    = help: within `(SingleKV<core::fmt::Arguments<'_>>, ())`, the trait `std::panic::RefUnwindSafe` is not implemented for `core::ops::Fn() + 'static`
    = note: required because it appears within the type `*mut core::ops::Fn() + 'static`
    = note: required because it appears within the type `core::marker::PhantomData<*mut core::ops::Fn() + 'static>`
    = note: required because it appears within the type `core::fmt::Void`
    = note: required because it appears within the type `&core::fmt::Void`
    = note: required because it appears within the type `core::fmt::ArgumentV1<'_>`
    = note: required because it appears within the type `[core::fmt::ArgumentV1<'_>]`
    = note: required because it appears within the type `&[core::fmt::ArgumentV1<'_>]`
    = note: required because it appears within the type `core::fmt::Arguments<'_>`
    = note: required because it appears within the type `SingleKV<core::fmt::Arguments<'_>>`
    = note: required because it appears within the type `(SingleKV<core::fmt::Arguments<'_>>, ())`
    = note: required because of the requirements on the impl of `SendSyncRefUnwindSafeKV` for `(SingleKV<core::fmt::Arguments<'_>>, ())`
note: required by `<Logger<D>>::root`
   --> src/lib.rs:1103:5
    |
1103| /     pub fn root<T>(drain: D, values: OwnedKV<T>) -> Logger
1104| |     where
1105| |         D: 'static + SendSyncRefUnwindSafeDrain<Err = Never, Ok = ()>,
1106| |         T: SendSyncRefUnwindSafeKV + 'static,
...   |
1112| |         }
1113| |     }
    | |_____^

dpc · 2018-06-19T16:08:16Z

I see. Thanks! Will look into it in 10 hours, I hope.

Make ? and % formatters in kv! more flexible

4580121

Add test ensuring % and ? take ownership

eccc839

dpc merged commit f5f7499 into slog-rs:master Jun 21, 2018

netvl mentioned this pull request Dec 27, 2019

? or % can't be used with o!(), looks like regression #248

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `?` and `%` formatters in `kv!` more flexible #170

Make `?` and `%` formatters in `kv!` more flexible #170

panicbit commented Jun 19, 2018 •

edited

Loading

dpc commented Jun 19, 2018

panicbit commented Jun 19, 2018

panicbit commented Jun 19, 2018 •

edited

Loading

dpc commented Jun 19, 2018

dpc commented Jun 19, 2018

dpc commented Jun 19, 2018

panicbit commented Jun 19, 2018 •

edited

Loading

dpc commented Jun 19, 2018

Make ? and % formatters in kv! more flexible #170

Make ? and % formatters in kv! more flexible #170

Conversation

panicbit commented Jun 19, 2018 • edited Loading

dpc commented Jun 19, 2018

panicbit commented Jun 19, 2018

panicbit commented Jun 19, 2018 • edited Loading

dpc commented Jun 19, 2018

dpc commented Jun 19, 2018

dpc commented Jun 19, 2018

panicbit commented Jun 19, 2018 • edited Loading

dpc commented Jun 19, 2018

Make `?` and `%` formatters in `kv!` more flexible #170

Make `?` and `%` formatters in `kv!` more flexible #170

panicbit commented Jun 19, 2018 •

edited

Loading

panicbit commented Jun 19, 2018 •

edited

Loading

panicbit commented Jun 19, 2018 •

edited

Loading