Skip to content

Commit ac51d96

Browse files
author
Martin Taillefer
committed
fix(ohno): address some correctness and usability findings
Correctness: - Fix compounding name bug in generate_unique_field_name (ohno_core_1_2 instead of ohno_core_2 on repeated collision) - Emit clear compile error when #[ohno::error] is applied to enums - Add #[track_caller] to Option<T> IntoAppError implementations Usability: - Make enrichments()/enrichment_messages() iterate in chronological order (oldest-first), consistent with format_error display order
1 parent 7030790 commit ac51d96

11 files changed

Lines changed: 78 additions & 35 deletions

File tree

crates/automation/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ pub fn list_packages(workspace_root: impl AsRef<Path>) -> Result<Vec<PackageMeta
5252

5353
if !output.status.success() {
5454
let stderr = String::from_utf8_lossy(&output.stderr);
55-
ohno::bail!("cargo metadata failed: {stderr}");
55+
ohno::bail!("cargo metadata failed: {}", stderr);
5656
}
5757

5858
let metadata: CargoMetadata = serde_json::from_slice(&output.stdout).into_app_err("failed to parse cargo metadata output")?;

crates/ohno/src/app/into_app_err.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,10 +70,12 @@ where
7070
}
7171

7272
impl<T> IntoAppError<T> for Option<T> {
73+
#[track_caller]
7374
fn into_app_err(self, msg: impl Display) -> Result<T, AppError> {
7475
self.ok_or_else(|| AppError::new(msg.to_string()))
7576
}
7677

78+
#[track_caller]
7779
fn into_app_err_with<F, D>(self, msg_fn: F) -> Result<T, AppError>
7880
where
7981
F: FnOnce() -> D,

crates/ohno/src/core.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -103,14 +103,14 @@ impl OhnoCore {
103103
self.data.backtrace.as_backtrace()
104104
}
105105

106-
/// Returns an iterator over the enrichment information in reverse order (most recent first).
106+
/// Returns an iterator over the enrichment information in chronological order (oldest first).
107107
pub fn enrichments(&self) -> impl Iterator<Item = &EnrichmentEntry> {
108-
self.data.enrichment.iter().rev()
108+
self.data.enrichment.iter()
109109
}
110110

111-
/// Returns an iterator over just the enrichment messages in reverse order (most recent first).
111+
/// Returns an iterator over just the enrichment messages in chronological order (oldest first).
112112
pub fn enrichment_messages(&self) -> impl Iterator<Item = &str> {
113-
self.data.enrichment.iter().rev().map(|ctx| ctx.message.as_ref())
113+
self.data.enrichment.iter().map(|ctx| ctx.message.as_ref())
114114
}
115115

116116
/// Formats the main error message without backtrace and error enrichment.
@@ -305,7 +305,7 @@ mod tests {
305305
error.add_enrichment(EnrichmentEntry::new("ctx1", "test.rs", 1));
306306
error.add_enrichment(EnrichmentEntry::new("ctx2", "test.rs", 2));
307307
let messages: Vec<_> = error.enrichment_messages().collect();
308-
assert_eq!(messages, vec!["ctx2", "ctx1"]);
308+
assert_eq!(messages, vec!["ctx1", "ctx2"]);
309309
}
310310

311311
#[test]

crates/ohno/tests/core.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -75,10 +75,10 @@ fn test_detailed_enrich() {
7575
let enrichments: Vec<_> = error.enrichments().collect();
7676
assert_eq!(enrichments.len(), 3);
7777

78-
// Most recent first
79-
assert_eq!(enrichments[0].message, "third message");
78+
// Oldest first (chronological), matching display order
79+
assert_eq!(enrichments[0].message, "first message");
8080
assert_eq!(enrichments[1].message, "second message");
81-
assert_eq!(enrichments[2].message, "first message");
81+
assert_eq!(enrichments[2].message, "third message");
8282
}
8383

8484
#[test]
@@ -125,7 +125,7 @@ fn test_trace_messages_iterator() {
125125
let error = OhnoCore::from("base").enrich("first").enrich("second");
126126

127127
let messages: Vec<_> = error.enrichment_messages().collect();
128-
assert_eq!(messages, vec!["second", "first"]);
128+
assert_eq!(messages, vec!["first", "second"]);
129129

130130
let display = error.to_string();
131131
let lines: Vec<_> = display.lines().collect();

crates/ohno/tests/enrich_err.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -330,7 +330,7 @@ fn empty_context_iter() {
330330
}
331331

332332
#[test]
333-
fn context_iter_reverse_order() {
333+
fn context_iter_chronological_order() {
334334
let mut core = OhnoCore::default();
335335

336336
let messages = ["msg 1", "msg 2", "msg 3", "msg 4", "msg 5"];
@@ -351,11 +351,11 @@ fn context_iter_reverse_order() {
351351
assert_eq!(
352352
messages,
353353
vec![
354-
("msg 5", "test.rs", 50),
355-
("msg 4", "test.rs", 40),
356-
("msg 3", "test.rs", 30),
357-
("msg 2", "test.rs", 20),
358354
("msg 1", "test.rs", 10),
355+
("msg 2", "test.rs", 20),
356+
("msg 3", "test.rs", 30),
357+
("msg 4", "test.rs", 40),
358+
("msg 5", "test.rs", 50),
359359
]
360360
);
361361
}

crates/ohno_macros/src/error_type_attr.rs

Lines changed: 33 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,10 @@ use crate::utils::generate_unique_field_name;
4646
pub fn error(_args: TokenStream, input: TokenStream) -> TokenStream {
4747
let mut input = parse_macro_input!(input as DeriveInput);
4848

49+
if let Err(err) = add_ohno_core_field(&mut input) {
50+
return TokenStream::from(err.to_compile_error());
51+
}
4952
add_fiasko_error_derive(&mut input);
50-
add_ohno_core_field(&mut input);
5153

5254
TokenStream::from(quote! { #input })
5355
}
@@ -61,7 +63,7 @@ fn add_fiasko_error_derive(input: &mut DeriveInput) {
6163
);
6264
}
6365

64-
fn add_ohno_core_field(input: &mut DeriveInput) {
66+
fn add_ohno_core_field(input: &mut DeriveInput) -> syn::Result<()> {
6567
if let Data::Struct(data_struct) = &mut input.data {
6668
match &mut data_struct.fields {
6769
Fields::Unit => {
@@ -95,8 +97,12 @@ fn add_ohno_core_field(input: &mut DeriveInput) {
9597
});
9698
}
9799
}
100+
Ok(())
98101
} else {
99-
// Not a struct, can't transform
102+
Err(syn::Error::new_spanned(
103+
&input.ident,
104+
"#[ohno::error] can only be applied to structs",
105+
))
100106
}
101107
}
102108

@@ -134,7 +140,7 @@ mod tests {
134140
}
135141
};
136142

137-
crate::error_type_attr::add_ohno_core_field(&mut input);
143+
crate::error_type_attr::add_ohno_core_field(&mut input).unwrap();
138144

139145
let expected: proc_macro2::TokenStream = parse_quote! {
140146
struct TestError {
@@ -149,19 +155,36 @@ mod tests {
149155

150156
#[test]
151157
fn test_add_ohno_core_field_with_enum() {
152-
// Test that the function doesn't crash when given an enum
153-
// This covers line 99 (the else branch for non-struct types)
158+
// Test that the function returns an error when given an enum
154159
let mut input: DeriveInput = parse_quote! {
155160
enum TestError {
156161
Variant1,
157162
Variant2,
158163
}
159164
};
160165

161-
let original = input.to_token_stream().to_string();
162-
crate::error_type_attr::add_ohno_core_field(&mut input);
166+
let result = crate::error_type_attr::add_ohno_core_field(&mut input);
167+
assert!(result.is_err());
168+
let err = result.unwrap_err();
169+
assert!(err.to_string().contains("#[ohno::error] can only be applied to structs"));
170+
}
171+
172+
#[test]
173+
fn test_add_ohno_core_field_enum_produces_compile_error() {
174+
// Verifies the to_compile_error() path used by the error proc macro (line 50)
175+
let mut input: DeriveInput = parse_quote! {
176+
enum NotAStruct {
177+
A,
178+
}
179+
};
180+
181+
let err = crate::error_type_attr::add_ohno_core_field(&mut input).unwrap_err();
182+
let compile_error = err.to_compile_error().to_string();
163183

164-
// The enum should remain unchanged since we can't transform it
165-
assert_eq!(input.to_token_stream().to_string(), original);
184+
assert!(compile_error.contains("compile_error"));
185+
assert!(
186+
compile_error.contains("#[ohno::error] can only be applied to structs"),
187+
"compile error should contain the expected message, got: {compile_error}"
188+
);
166189
}
167190
}

crates/ohno_macros/src/utils.rs

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,7 @@ pub fn generate_unique_field_name(existing_fields: &[&syn::Ident]) -> syn::Ident
4848
let mut counter = 0;
4949
while existing_fields.iter().any(|ident| ident == &AsRef::<str>::as_ref(&candidate)) {
5050
counter += 1;
51-
candidate = format!("{candidate}_{counter}");
51+
candidate = format!("ohno_core_{counter}");
5252
}
5353

5454
syn::Ident::new(&candidate, proc_macro2::Span::call_site())
@@ -110,4 +110,17 @@ mod tests {
110110
// Verify that the returned name is NOT in the existing fields
111111
assert!(!fields.iter().any(|ident| name == ident.to_string()));
112112
}
113+
114+
#[test]
115+
fn test_generate_unique_field_name_multiple_collisions() {
116+
let ident1 = syn::Ident::new("ohno_core", proc_macro2::Span::call_site());
117+
let ident2 = syn::Ident::new("ohno_core_1", proc_macro2::Span::call_site());
118+
let ident3 = syn::Ident::new("ohno_core_2", proc_macro2::Span::call_site());
119+
let fields = vec![&ident1, &ident2, &ident3];
120+
121+
let name = generate_unique_field_name(&fields);
122+
123+
// Should return "ohno_core_3", not a compounding name like "ohno_core_1_2_3"
124+
assert_eq!(name.to_string(), "ohno_core_3");
125+
}
113126
}

crates/seatbelt/Cargo.toml

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -73,27 +73,27 @@ tracing-subscriber = { workspace = true, features = ["fmt", "std"] }
7373

7474
[[example]]
7575
name = "timeout"
76-
required-features = ["timeout"]
76+
required-features = ["timeout", "metrics"]
7777

7878
[[example]]
7979
name = "timeout_advanced"
80-
required-features = ["timeout"]
80+
required-features = ["timeout", "metrics"]
8181

8282
[[example]]
8383
name = "retry"
8484
required-features = ["retry"]
8585

8686
[[example]]
8787
name = "retry_advanced"
88-
required-features = ["retry"]
88+
required-features = ["retry", "metrics"]
8989

9090
[[example]]
9191
name = "retry_outage"
92-
required-features = ["retry"]
92+
required-features = ["retry", "metrics"]
9393

9494
[[example]]
9595
name = "resilience_pipeline"
96-
required-features = ["retry", "timeout"]
96+
required-features = ["retry", "timeout", "metrics"]
9797

9898
[[example]]
9999
name = "tower"

crates/seatbelt/src/context.rs

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,12 @@ impl<In, Out> ResilienceContext<In, Out> {
7373

7474
#[cfg_attr(
7575
not(any(feature = "metrics", feature = "logs", test)),
76-
expect(unused_variables, reason = "unused when logs nor metrics are used")
76+
expect(
77+
unused_variables,
78+
clippy::unused_self,
79+
clippy::needless_pass_by_value,
80+
reason = "unused when logs nor metrics are used"
81+
)
7782
)]
7883
#[cfg(any(feature = "retry", feature = "breaker", feature = "timeout", test))]
7984
pub(crate) fn create_telemetry(&self, strategy_name: Cow<'static, str>) -> crate::utils::TelemetryHelper {

crates/seatbelt/src/retry/service.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,7 @@ impl<In, Out> RetryShared<In, Out> {
217217

218218
#[cfg_attr(
219219
not(any(feature = "logs", test)),
220-
expect(unused_variables, reason = "unused when logs feature not used")
220+
expect(unused_variables, clippy::unused_self, reason = "unused when logs feature not used")
221221
)]
222222
fn emit_telemetry(&self, attempt: Attempt, retry_delay: Duration) {
223223
#[cfg(any(feature = "logs", test))]

0 commit comments

Comments
 (0)