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

Implement #[zeroize(bound = "T: MyTrait")] #663

Merged
merged 1 commit into from Nov 11, 2021

Conversation

daxpedda
Copy link
Contributor

@tarcieri
Copy link
Member

@daxpedda needs a rebase

@daxpedda
Copy link
Contributor Author

Done!

Comment on lines +173 to +203
panic!(
concat!(
"The #[zeroize(bound)] attribute is not allowed on {} fields. ",
"Use it on the containing {} instead.",
),
item_kind, item_kind,
)
}
(Some(_variant), None) => panic!(concat!(
"The #[zeroize(bound)] attribute is not allowed on enum variants. ",
"Use it on the containing enum instead.",
)),
(None, None) => {
if let Meta::NameValue(meta_name_value) = meta {
if let Lit::Str(lit) = &meta_name_value.lit {
if lit.value().is_empty() {
self.bound = Some(Bounds(Punctuated::new()));
} else {
self.bound = Some(lit.parse().unwrap_or_else(|e| {
panic!("error parsing bounds: {:?} ({})", lit, e)
}));
}

return;
}
}

panic!(concat!(
"The #[zeroize(bound)] attribute expects a name-value syntax with a string literal value.",
"E.g. #[zeroize(bound = \"T: MyTrait\")]."
))
Copy link
Member

Choose a reason for hiding this comment

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

This much error handling really makes me wish we were using proc-macro-error, although I haven't figured out how to make it play nicely with synstructure.

Copy link
Contributor Author

@daxpedda daxpedda Nov 11, 2021

Choose a reason for hiding this comment

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

I've tended to not use a lot of libraries when working with proc-macros exactly because of interoperability. I was close to suggesting to drop synstrucutre completely in favor of just using plain syn to skip parsing the user input and let rustc just say what's wrong with the where bounds instead of trying to convert them to WherePredicates.

I can look into it if you deem it necessary.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I recently dropped synstructure from another proc macro crate, although zeroize_derive is better leveraging it.

proc-macro-error though I think is definitely worth it, since it allows emitting errors with diagnostic information that includes the original source spans, which makes error messages generated from proc macros substantially better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it without proc-macro-error by just using to_compile_error until now. But our current strategy using panic isn't ideal, true enough.

Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming you meant elsewhere outside the scope of this PR?

With proc-macro-error I've managed to make sure that practically every error case is tied to the input code that caused it.

Copy link
Contributor Author

@daxpedda daxpedda Nov 11, 2021

Choose a reason for hiding this comment

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

I'm assuming you meant elsewhere outside the scope of this PR?

Sorry, lack of context indeed 😄.

With proc-macro-error I've managed to make sure that practically every error case is tied to the input code that caused it.

That sounds great! I will take a look at it then.

@tarcieri
Copy link
Member

Will go ahead and merge this, although if you'd like to look into removing synstructure that'd be great.

@tarcieri tarcieri merged commit 63c8e60 into RustCrypto:master Nov 11, 2021
@daxpedda
Copy link
Contributor Author

Will do!

This was referenced Jan 14, 2022
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.

None yet

2 participants