fix(derive-encode): emit friendly compile errors instead of panicking#307
fix(derive-encode): emit friendly compile errors instead of panicking#307lohitkolluri wants to merge 1 commit into
Conversation
The proc-macro derives panicked for unsupported shapes (unnamed/unit struct, enum, union, unknown `#[prometheus(...)] attribute) and the bad-attribute case was inside a closure, which produced a confusing `error: proc-macro derive panicked` diagnostic that pointed users at the proc-macro internals rather than the `#[derive(...)] line they needed to fix. Replace all `panic!` calls with `compile_error!` so users see a direct diagnostic that points to the offending `#[derive(...)] line. For the unknown-attribute case, validate attributes in a pre-pass so the derive can return a `compile_error!` token stream before generating the impl. Add a trybuild `compile_fail` test that exercises each error path and pins the expected diagnostics. Signed-off-by: Lohit Kolluri <lohitkolluri@gmail.com>
lohitkolluri
force-pushed
the
fix/269-friendly-compile-errors
branch
from
756f7a2 to
9fcbe1b
Compare
krisztianfekete
left a comment
krisztianfekete
left a comment
There was a problem hiding this comment.
Hey, thanks for the PR, left a few comments!
| let msg = format!( | ||
| "Provided attribute '{other}', but only 'flatten' is supported" | ||
| ); | ||
| return quote! { compile_error!(#msg); }.into(); |
There was a problem hiding this comment.
We could, instead of hand crafting the compile_error!, you could use syn::Error::new_spanned(&other, msg).to_compile_error(). It generates the same kind of error, but the little ^^^^ arrow points right at the unknown in [prometheus(unknown)] instead of #[derive(...)]
We could do these for the other sites too, syn::Error::new_spanned(&ast, "...") for the enum/union/unnamed-struct cases points at the offending type.
| @@ -27,13 +54,7 @@ pub fn derive_encode_label_set(input: TokenStream) -> TokenStream { | |||
| .iter() | |||
| .find(|a| a.path().is_ident("prometheus")) | |||
| .map(|a| a.parse_args::<syn::Ident>().unwrap().to_string()); | |||
There was a problem hiding this comment.
This .unwrap() is only safe because the pre-pass already filtered out unparseable attrs. This is easy to break later, and it means we parse every #[prometheus(...)] twice. Having the closure return a Result and .collect()-ing it would drop the pre-pass, the double parse, and the unwrap all at once.
2 participants
Summary
Replace
panic!calls in theEncodeLabelSetandEncodeLabelValueproc-macro derives withcompile_error!so unsupported shapes surface a direct diagnostic that points to the#[derive(...)]line the user wrote, instead of the currenterror: proc-macro derive panickedmessage that points into the proc-macro internals.Full changes
panic!sites inderive-encode/src/lib.rsnow emitcompile_error!tokens.#[prometheus(...)]attributes (which previously panicked inside a closure) are validated in a pre-pass so the derive can return acompile_error!token stream before generating the impl.compile_failtest inderive-encode/tests/build/friendly-compilation-error-msg.rscovers unnamed struct, unit struct, enum, union, and unknown-attribute paths. The expected.stderris committed alongside it.Testing
cargo test -p prometheus-client-derive-encode --test libpasses deterministically (with and withoutTRYBUILD=overwrite).cargo clippy -p prometheus-client-derive-encode --all-targets -- -D warningsclean.cargo fmt --check -p prometheus-client-derive-encodeclean.Notes
For valid inputs the derive behavior is unchanged. The new diagnostics are user-facing; no downstream tooling currently parses these derive errors, so this is not a breaking change.
Issues
Closes #269