Skip to content

Commit

Permalink
Panic on duplicated field names
Browse files Browse the repository at this point in the history
  • Loading branch information
nico-incubiq committed Nov 27, 2024
1 parent bddd173 commit acd38e1
Show file tree
Hide file tree
Showing 2 changed files with 59 additions and 19 deletions.
43 changes: 24 additions & 19 deletions tracing-attributes/tests/fields.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,14 @@ fn fn_struct_const_field_name() {}
#[instrument(fields({"foo"} = "bar"))]
fn fn_string_field_name() {}

const CLASHY_FIELD_NAME: &str = "s";

#[instrument(fields({CLASHY_FIELD_NAME} = "foo"))]
fn fn_clashy_const_field_name(s: &str) { let _ = s; }
// 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 {
Expand Down Expand Up @@ -235,21 +239,22 @@ fn string_field_name() {
});
}

#[test]
fn clashy_const_field_name() {
let span = expect::span().with_fields(
// 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).
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");
});
}
// 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()
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 {
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

0 comments on commit acd38e1

Please sign in to comment.