trace: Add shorthand syntax for local fields

[hawkw]

Motivation

A common pattern in tokio-trace is to use the value of a local
variable as a field on a span or event. Currently, this requires code
like:

info!(foo = foo);

which is not particularly ergonomic given how commonly this occurs.
Struct initializers support a shorthand syntax for fields where the name
of the field is the same as a local variable, and tokio-trace should
as well.

Solution

This branch adds support for syntax like

let foo = ...;
info!(foo);

and

let foo = Foo {
    bar: ...,
    ...
};
info!(foo.bar)

to the tokio-trace span and event macros. This syntax also works with
the Debug and Display field shorthand.

The span macros previously used a field name with no value to indicate
an uninitialized field. A new issue, #1138, has been opened for finding a
replacement syntax for uninitialized fields. Until then, the tokio-trace
macros will no longer provide a way to create fields without values,
although the -core API will continue to support this.

Closes #1062

Signed-off-by: Eliza Weisman eliza@buoyant.io

[hawkw]

@carllerche, I'd like to go ahead and merge this, but I figured I'd ping you first in case you want to take a closer look.

[carllerche]

i'm not in love w/ foo = _ as a syntax to define a field that is not being assigned to. This is using the assignment operator to say "not assigned".

[carllerche]

Could something like: info!(foo, bar = 3, unassigned(baz, bur)) work?

[hawkw]

@carllerche I agree that using the assignment operator to indicate unassigned fields is definitely a bit strange, and I'm open to a different syntax.

I have a couple of quibbles about unassigned(baz, bur) though:

• unassigned is kind of wordy for what I still expect to be at least a relatively common macro usage.
• I'd prefer to avoid a syntax that looks like a function call for something that isn't actually implemented as a function -- I'd rather have it look like a keyword, as in unassigned bar. On the other hand, in that case you'd have to repeat the unassigned for every unassigned field, which is even wordier.

So, I'm open to this syntax if a majority of folks agree it's better.

I'm still not really in love with any of the options we've got here, using the field name without an assignment was probably the best fit for this meaning. But the case of using local variables is even more common, and allowing shorthand for that definitely improves ergonomics. It's just a shame that there isn't really an obvious, natural syntax for unassigned fields.

I'm still open to suggestions.

[carllerche]

info!(foo, bar, let baz, let buz);

?

[hawkw]

@jonhoo, @davidbarsky, @LucioFranco, you've all weighed in on this in the past --- any thoughts on Carl's suggestions? What seems the most intuitive to you?

[jonhoo]

I still like field = _ better. let bar in a list strikes me as really odd.. We could use some kind of surrounder like (field) to indicate unnamed maybe, but that's also weird.

[hawkw]

Unless anyone else using tokio-trace (@davidbarsky? @LucioFranco? @Ralith?) has opinions on the syntax, I'm inclined to just move forward with field = _ and merge this PR as-is, since it seems like it was what most folks were in favor of in the discussion on #1062.

I'm also open to Carl's suggestion of let field, but personally I'm slightly in favor of field = _.

[carllerche]

I don't want to move forward with field = _. It seems very counter intuitive.

[carllerche]

(unless others think I am wrong and the syntax makes sense).

[carllerche]

why not foo? for an unspecified field?

[davidbarsky]

I prefer the field = _ syntax significantly more than foo?, in that it is symmetric to the existing let _ = call(); syntax. foo? feels like the field is being assigned and an error might be returned from this context.

[carllerche]

foo = _ is using the assignment operator to say "don't assign", this feels backwards to me and I don't know of any precedence where such syntax is used.

[seanmonstar]

let field seems better. Maybe field?, but I haven't thought that one through at all.

[hawkw]

why not foo? for an unspecified field?

There are a couple reasons I don't think this is a great option:

1. trace: Add shorthand for field::display and field::debug #1088 added foo = ?expr as shorthand for foo = field::debug(expr), and this branch builds on that by allowing that shorthand to be used alongside the local variable shorthand (i.e., ?foo as shorthand for foo = field::debug(foo)). It seems potentially confusing to have both ?foo and foo? with different meanings.
2. It also looks similar to the ? operator for propagating errors. An ? expression wouldn't currently be valid in this position in the macro, so it doesn't actually conflict, but it looks similar, which I think also has the potential to be confusing.

---

Another potential option I thought was worth bringing up again is @jonhoo's suggestion of field.. in #1062 (comment). We currently allow single . characters in field names, but only with an identifier on both sides (i.e., the field access syntax), so it would work with the existing macro matchers. I'm not sure if something like foo.bar.baz.. would be confusing to users or not, though.

[hawkw]

In order to move forwards with the local field shorthand, I'm considering punting on the replacement syntax for uninitialized fields for now, and adding that in a future PR once we can agree on an appropriate syntax. This means that the tokio-trace span macros would (temporarily) not support uninitialized fields on master.

@jonhoo, @davidbarsky, @LucioFranco, do any of you have any objections to that? The alternative is trying to reach a consensus on a replacement syntax.

[jonhoo]

Seems fine to me

[LucioFranco]

I personally do not need this right away, so im always +1 on punting (I like procrastinating anyways :D )

[carllerche]

@hawkw would adding support later require a change to -core?

[hawkw]

@carllerche no, tokio-trace-core will support uninitialized fields regardless of this change, this is strictly a change to the macros in tokio-trace. The support in the core APIs will still be there, it will just be inaccessible through the macros until they have a new syntax for using it.

[carllerche]

@hawkw +1 on punting.

[hawkw]

Okay, I've removed the field = _ syntax for now, and opened #1138 for agreeing on a new syntax for uninitialized fields.

[mbilker]

Was changing the debug and debug_span macros to use the INFO level instead of the DEBUG level intentional with this PR? If not, I can open a PR changing those macros back to the DEBUG level.

[hawkw]

Nope, that was definitely not intentional! A fix would be very appreciated.

[mbilker]

I put up a fix in PR #1170