Support constant expressions as instrument field names

[nico-incubiq]

Motivation

The #[instrument] proc macro only allows setting field names as identifiers, which is not on par with other macros such as span! or event! that allow constant expressions. Using constants for example can ensure consistency across a project for the field names used.

This addresses #2969, which was already mentioned in this comment here on #2617 (note that "classic macros" mentioned there already work, there are tests attesting of it).
I also updated the doc block to mention field values can be any arbitrary expressions, which was implemented by #672 but never updated.

Solution

I updated the instrument proc macro to allow constant expressions (const, const fn, literal str, ...) as field names, using the same syntax as #2617, enclosed in curly braces.

const CONST_FIELD_NAME: &str = "foo.bar";

#[instrument(fields({CONST_FIELD_NAME} = "quux"))]
fn fn_const_field_name() {}

Notes

• The identifier (Punctuated) field names have a convenient feature allowing to supersede non-skipped function arguments (a field named s would be used instead of a function argument s in the span). As far as I'm aware this is not possible to implement for constant expressions, as the compiler doesn't allow to evaluate constants at build time (MIRI might be a solution but that seems complicated, and https://rustc-dev-guide.rust-lang.org/const-eval seems experimental). It's a minor inconsistency; in this scenario, the code would panic immediately at startup, and the workaround is as simple as skipping the function argument.

[hds]

Let's add a test for the panic here, to ensure that this check doesn't get accidentally removed at some point in the future. It may be necessary to move the test to a "ui" test to validate the compiler error message.

[nico-incubiq]

I no longer think adding a panic or deduplication logic for constant expressions is feasible at compile time. Every solution I tested incurs a runtime cost, as constant expressions need to be evaluated first, and then every value needs to be compared against each other.
Given that the impact of not doing it is "just" duplicated fields reported, and is limited to people newly opting into braced constant expressions, I don't expect this to cause much problem.

I updated the docs to explain this.

[nico-incubiq]

Expanding on the previous message with more details:
The panic solution worked at compile time because it was fully integrated inside a constant context, which I learned recently the compiler is happy to execute. But I understand this is less than ideal from a user perspective, as it introduces a breaking change.

On the other hand, a new FieldSet::new_checked constructor would be very hard to surface, as it would force to duplicate a lot of the code, including the span! macro, at which point the code becomes unmaintainable.

I would prefer to implement deduplication, but this remained out of reach after many attempts.

• The existing deduplication code in expand.rs occurs during macro expansion, while constant evaluation by the compiler only happens at a later stage.
• Another point at which this could be implemented is around FieldSet creation, but given that ValueSet is created in a totally unrelated part of the code and the two need to stay in sync, it seems impossible to keep them aligned as duplicated fields are removed.
• I have stayed away from runtime deduplication, as this is an undesirable cost to pay for something that should only be checked statically.

It'll be possible to do a follow-up PR to add deduplication when someone hopefully figures it out.

[NickLarsenNZ]

@nico-incubiq , are you still planning to work on this?

[nico-incubiq]

@nico-incubiq , are you still planning to work on this?

I'd like to pick this back up yes.
Although, I'm not sure the suggested approach of creating a new constructor to avoid introducing a panic achieves the original goal as it wouldn't be possible to use constants in existing attributes. I would like attributes to be on par with macros as introduced by #2617.
I've been looking for a way to evaluate the constant at compile time to avoid panicking, but I'm not sure how.

Also I've used Block for the FieldName because it is already parsed enclosed in braces, but that's much too broad, ideally I'd rather parse { + Expr + }. Done.

[nico-incubiq]

This is ready for review again when someone has time 🙏🏼

[hds]

Small docs nit. Otherwise, this looks good now!

[nico-incubiq]

Thanks @hds, I merged your change and rebased off the latest main, some checks haven't ran i suppose you need to trigger them on your side?

[hds]

@nico-incubiq run. Just a rustfmt failure (small one).

[nico-incubiq]

Fixed

[hds]

Thanks for all your work on this PR!