slog-rs / slog

Structured, contextual, extensible, composable logging for Rust
https://slog.rs/
Apache License 2.0
1.58k stars 95 forks source link

Macro key-value `"err" => #e` syntax only works as the last argument #289

Closed lilyball closed 3 years ago

lilyball commented 3 years ago

The slog macros support "err" => #e as shorthand for "err" => slog::ErrorValue(e). But for some reason adding any argument after this one triggers a parse error (when building, or a recursion limit error in rust-analyzer):

This works:

warn!(log, "msg"; "error" => #e);

As does this:

warn!(log, "msg"; "key" => "value", "error" => #e);

But this triggers the error:

warn!(log, "msg"; "error" => #e, "key" => "value");
error: expected one of `!` or `[`, found `e`
   --> src/main.rs:52:35
    |
52  |     warn!(log, "msg"; "error" => #e, "key" => "value");
    |                                   ^ expected one of `!` or `[`
    | 
   ::: /Users/lily/.cargo/registry/src/github.com-1ecc6299db9ec823/slog-2.7.0/src/lib.rs:425:37
    |
425 |     (@ $args_ready:expr; $k:expr => $v:expr) => {
    |                                     ------- while parsing argument for this `expr` macro fragment

error: aborting due to previous error
dpc commented 3 years ago

How was this undiscovered for so long? :D

https://github.com/slog-rs/slog/blob/f9e30f0dd9bad610227d9374001fcc13d5b7c8c8/src/lib.rs#L422

this case needs to duplicated with , $($args:tt)* part, just like other forms.

Care do crate a PR with a test + fix? :)

dpc commented 3 years ago

Oh, and slog_kv https://github.com/slog-rs/slog/blob/f9e30f0dd9bad610227d9374001fcc13d5b7c8c8/src/lib.rs#L475 is supposed to be an exact copy of kv (just with a different name in case of a collision), but I can already see they are differences.

These macros used to be much simpler, and don't really scale well as contributors add new cases. :shrug:

lilyball commented 3 years ago

Probably because the log macros don’t bother to document this feature, so most people are unlikely to be using it. I only found it by looking at ErrorValue, seeing the reference to this feature, and looked at the macro implementation to confirm.

I am not able to write any PRs at this time.

dpc commented 3 years ago

I'll try to look into it later today then.