Skip to content

Conversation

@Duckaet
Copy link
Contributor

@Duckaet Duckaet commented Oct 29, 2025

implements follow-up #538

1.entrypoint expect() would cause panic, handled err better
2.PxStripper for Impl and Trait aswell
3.add ast nodes functions to re-emit with input
4.stripped almost at all entrypoints
5.also generated handle constants when validation fails... is it graceful?

add a fail test as suggested in issue.

@LukeMathWalker
Copy link
Owner

Thanks for picking this up!
I think we need to refine the approach a bit further: the user may have specified a custom id which will be used as the name of the generated handle type. If that's present, we should honor it.

@LukeMathWalker
Copy link
Owner

use pavex_macros::error_handler;

#[error_handler(id = "CUSTOM")]  
pub fn invalid_handler(_a: &str, _b: u64) -> pavex::Response {
    todo!()
}

fn test_handle_exists() {
    let _handle = CUSTOM;
}

fn main() {}

^ The above should not cause an error in test_handle_exists when compiled.

@Duckaet
Copy link
Contributor Author

Duckaet commented Nov 12, 2025

I'm not able to understand the better approach.. as custom id is already honored like this

in the error_handler.rs ->

Err(e) => {
let properties = Properties::new(metadata, 0);
let result = emit(impl_, item, properties);

and in emit function
let id = id.unwrap_or_else(|| default_id(impl_.as_ref(), &item));

and the test also does not cause an error in "fn test_handle_exists"

ig a better approach would be to re-emit striped functions? as now it only emits the error and handler
let combined_error = quote::quote! {
#error_tokens
#handle_def
};
return Err(combined_error.into());

should i implement this approach? @LukeMathWalker

Comment on lines 152 to 155
if let Ok(mut foreign_fn) = syn::parse2::<syn::ForeignItemFn>(input.clone()) {
crate::utils::PxStripper.visit_foreign_item_fn_mut(&mut foreign_fn);
return quote! { #error_tokens #foreign_fn }.into();
}
Copy link
Owner

Choose a reason for hiding this comment

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

No need to handle foreign functions, it's a usecase that we won't officially support for a while, probably ever.

Comment on lines 89 to 92
fn visit_foreign_item_fn_mut(&mut self, node: &mut ForeignItemFn) {
strip_pavex_attrs(&mut node.attrs);
visit_mut::visit_foreign_item_fn_mut(self, node);
}
Copy link
Owner

Choose a reason for hiding this comment

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

Same as above, let's remove it.

@LukeMathWalker
Copy link
Owner

You're right, I was thinking of a slightly different scenario—parsing the entire metadata fails, but we can successfully parse a subset of it out (i.e. the id).
No need to handle everything at this stage though. Left a couple of small comments, then we're good to go.

@LukeMathWalker
Copy link
Owner

/ok-to-test sha=3f7f2b9

@LukeMathWalker LukeMathWalker merged commit fe764db into LukeMathWalker:main Nov 30, 2025
10 of 11 checks passed
@LukeMathWalker
Copy link
Owner

Thanks!

@Duckaet
Copy link
Contributor Author

Duckaet commented Dec 6, 2025

Thanks!

I learnt alot, while reading the codebase... thanks to you ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants