Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
lilyball opened this issue Jul 27, 2021 · 4 comments
Closed

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

lilyball opened this issue Jul 27, 2021 · 4 comments

Comments

@lilyball
Copy link

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
Copy link
Collaborator

dpc commented Jul 27, 2021

How was this undiscovered for so long? :D

slog/src/lib.rs

Line 422 in f9e30f0

(@ $args_ready:expr; $k:expr => #$v:expr) => {

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

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

@dpc
Copy link
Collaborator

dpc commented Jul 27, 2021

Oh, and slog_kv

slog/src/lib.rs

Line 475 in f9e30f0

(@ $args_ready:expr; $k:expr => $v:expr) => {
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. 🤷

@lilyball
Copy link
Author

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
Copy link
Collaborator

dpc commented Jul 27, 2021

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

@dpc dpc closed this as completed in e8c0c9f Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants