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

Support constant expressions as instrument field names #3158

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 26 additions & 3 deletions tracing-attributes/src/attr.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use std::collections::HashSet;
use syn::{punctuated::Punctuated, Expr, Ident, LitInt, LitStr, Path, Token};
use syn::{punctuated::Punctuated, Block, Expr, Ident, LitInt, LitStr, Path, Token};

use proc_macro2::TokenStream;
use quote::{quote, quote_spanned, ToTokens};
Expand Down Expand Up @@ -294,7 +294,7 @@ pub(crate) struct Fields(pub(crate) Punctuated<Field, Token![,]>);

#[derive(Clone, Debug)]
pub(crate) struct Field {
pub(crate) name: Punctuated<Ident, Token![.]>,
pub(crate) name: FieldName,
pub(crate) value: Option<Expr>,
pub(crate) kind: FieldKind,
}
Expand All @@ -306,6 +306,21 @@ pub(crate) enum FieldKind {
Value,
}

#[derive(Clone, Debug)]
pub(crate) enum FieldName {
Expr(Block),
Punctuated(Punctuated<Ident, Token![.]>),
}

impl ToTokens for FieldName {
fn to_tokens(&self, tokens: &mut TokenStream) {
match self {
FieldName::Expr(expr) => expr.to_tokens(tokens),
FieldName::Punctuated(punctuated) => punctuated.to_tokens(tokens),
}
}
}

impl Parse for Fields {
fn parse(input: ParseStream<'_>) -> syn::Result<Self> {
let _ = input.parse::<kw::fields>();
Expand All @@ -332,7 +347,15 @@ impl Parse for Field {
input.parse::<Token![?]>()?;
kind = FieldKind::Debug;
};
let name = Punctuated::parse_separated_nonempty_with(input, Ident::parse_any)?;
// Parse name as either an expr between braces or a dotted identifier.
let name = if input.peek(syn::token::Brace) {
FieldName::Expr(input.parse::<Block>()?)
} else {
FieldName::Punctuated(Punctuated::parse_separated_nonempty_with(
input,
Ident::parse_any,
)?)
};
let value = if input.peek(Token![=]) {
input.parse::<Token![=]>()?;
if input.peek(Token![%]) {
Expand Down
13 changes: 11 additions & 2 deletions tracing-attributes/src/expand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ use syn::{
Path, ReturnType, Signature, Stmt, Token, Type, TypePath,
};

use crate::attr::FieldName;
use crate::{
attr::{Field, Fields, FormatMode, InstrumentArgs, Level},
MaybeItemFn, MaybeItemFnRef,
Expand Down Expand Up @@ -190,8 +191,16 @@ fn gen_block<B: ToTokens>(
// and allow them to be formatted by the custom field.
if let Some(ref fields) = args.fields {
fields.0.iter().all(|Field { ref name, .. }| {
let first = name.first();
first != name.last() || !first.iter().any(|name| name == &param)
match name {
// TODO(#3158): Implement this variant when the compiler supports const
// evaluation (https://rustc-dev-guide.rust-lang.org/const-eval).
FieldName::Expr(_) => true,
FieldName::Punctuated(punctuated) => {
let first = punctuated.first();
first != punctuated.last()
|| !first.iter().any(|name| name == &param)
}
}
})
} else {
true
Expand Down
24 changes: 17 additions & 7 deletions tracing-attributes/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,14 +107,16 @@ mod expand;
/// - multiple argument names can be passed to `skip`.
/// - arguments passed to `skip` do _not_ need to implement `fmt::Debug`.
///
/// Additional fields (key-value pairs with arbitrary data) can be passed to
/// Additional fields (key-value pairs with arbitrary data) can be passed
/// to the generated span through the `fields` argument on the
/// `#[instrument]` macro. Strings, integers or boolean literals are accepted values
/// `#[instrument]` macro. Arbitrary expressions are accepted as value
/// for each field. The name of the field must be a single valid Rust
/// identifier, nested (dotted) field names are not supported.
/// identifier, or a constant expression that evaluates to one, enclosed in curly
/// braces. Note that nested (dotted) field names are supported.
///
/// Note that overlap between the names of fields and (non-skipped) arguments
/// will result in a compile error.
/// Note that defining a field with the same name as a (non-skipped)
/// argument will implicitly skip the argument, or even panic if using
/// constants as field names.
///
/// # Examples
/// Instrumenting a function:
Expand Down Expand Up @@ -214,8 +216,16 @@ mod expand;
///
/// ```
/// # use tracing_attributes::instrument;
/// #[instrument(fields(foo="bar", id=1, show=true))]
/// fn my_function(arg: usize) {
/// #[derive(Debug)]
/// struct Argument;
/// impl Argument {
/// fn bar() -> &'static str {
/// "bar"
/// }
/// }
/// const FOOBAR: &'static str = "foo.bar";
/// #[instrument(fields(foo="bar", id=1, show=true, {FOOBAR}=%arg.bar()))]
/// fn my_function(arg: Argument) {
/// // ...
/// }
/// ```
Expand Down
97 changes: 97 additions & 0 deletions tracing-attributes/tests/fields.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,38 @@ fn fn_string(s: String) {
#[instrument(fields(keywords.impl.type.fn = _arg), skip(_arg))]
fn fn_keyword_ident_in_field(_arg: &str) {}

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

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

const fn get_const_fn_field_name() -> &'static str {
"foo.bar"
}

#[instrument(fields({get_const_fn_field_name()} = "baz"))]
fn fn_const_fn_field_name() {}

struct FieldNames {}
impl FieldNames {
const FOO_BAR: &'static str = "foo.bar";
}

#[instrument(fields({FieldNames::FOO_BAR} = "baz"))]
fn fn_struct_const_field_name() {}

#[instrument(fields({"foo"} = "bar"))]
fn fn_string_field_name() {}

// TODO(#3158): To be consistent with event! and span! macros, the argument's value should be
// dropped, but this is only possible to implement when the compiler supports const evaluation
// (https://rustc-dev-guide.rust-lang.org/const-eval). For now the duplicated field would
// trigger a panic.
// const CLASHY_FIELD_NAME: &str = "s";
//
// #[instrument(fields({CLASHY_FIELD_NAME} = "foo"))]
// fn fn_clashy_const_field_name(s: &str) { let _ = s; }

#[derive(Debug)]
struct HasField {
my_field: &'static str,
Expand Down Expand Up @@ -159,6 +191,71 @@ fn keyword_ident_in_field_name() {
run_test(span, || fn_keyword_ident_in_field("test"));
}

#[test]
fn expr_const_field_name() {
let span = expect::span().with_fields(
expect::field("foo.bar")
.with_value(&"baz")
.only(),
);
run_test(span, || {
fn_const_field_name();
});
}

#[test]
fn expr_const_fn_field_name() {
let span = expect::span().with_fields(
expect::field("foo.bar")
.with_value(&"baz")
.only(),
);
run_test(span, || {
fn_const_fn_field_name();
});
}

#[test]
fn struct_const_field_name() {
let span = expect::span().with_fields(
expect::field("foo.bar")
.with_value(&"baz")
.only(),
);
run_test(span, || {
fn_struct_const_field_name();
});
}

#[test]
fn string_field_name() {
let span = expect::span().with_fields(
expect::field("foo")
.with_value(&"bar")
.only(),
);
run_test(span, || {
fn_string_field_name();
});
}

// TODO(#3158): To be consistent with event! and span! macros, the argument's value should be
// dropped, but this is only possible to implement when the compiler supports const evaluation
// (https://rustc-dev-guide.rust-lang.org/const-eval). For now the duplicated field would
// trigger a panic.
// #[test]
// fn clashy_const_field_name() {
// let span = expect::span().with_fields(
// expect::field("s")
// .with_value(&"foo")
// .and(expect::field("s").with_value(&"hello world"))
// .only(),
// );
// run_test(span, || {
// fn_clashy_const_field_name("hello world");
// });
// }

fn run_test<F: FnOnce() -> T, T>(span: NewSpan, fun: F) {
let (collector, handle) = collector::mock()
.new_span(span)
Expand Down
35 changes: 35 additions & 0 deletions tracing-core/src/field.rs
Original file line number Diff line number Diff line change
Expand Up @@ -730,11 +730,46 @@ impl Clone for Field {
}
}

// Compares two strings for equality in a constant context.
const fn str_eq(left: &str, right: &str) -> bool {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all of this is because the code is executed from a const fn context. i couldn't find off-the-shelf functions that do the same, and I also couldn't find a better place to add this check than the constructor of the FieldSet.

if left.len() != right.len() {
return false;
}

let left_bytes = left.as_bytes();
let right_bytes = right.as_bytes();
let mut i = left_bytes.len();
while i > 0 {
if left_bytes[i - 1] != right_bytes[i - 1] {
return false;
}
i = i - 1;
}

true
}

// Asserts field names are distinct.
const fn assert_distinct(names: &[&str]) {
let mut i = names.len();
while i > 0 {
let mut j = i - 1;
while j > 0 {
if str_eq(names[i - 1], names[j - 1]) {
panic!("duplicated field name");
}
j = j - 1;
}
i = i - 1;
}
}

// ===== impl FieldSet =====

impl FieldSet {
/// Constructs a new `FieldSet` with the given array of field names and callsite.
pub const fn new(names: &'static [&'static str], callsite: callsite::Identifier) -> Self {
assert_distinct(names);
Self { names, callsite }
}

Expand Down