Skip to content

Conversation

@npmccallum
Copy link
Contributor

  1. add a derive macro for deriving enumerations that are tagged as INTEGER.
  2. use the new macro for pkcs10::Version.
  3. add x509::Version.

Comment on lines 251 to 218
#[proc_macro_derive(Integer)]
#[proc_macro_error]
pub fn derive_integer(input: TokenStream) -> TokenStream {
let input = parse_macro_input!(input as DeriveInput);
DeriveEnumerated::new(input, true).to_tokens().into()
}
Copy link
Member

@tarcieri tarcieri Feb 11, 2022

Choose a reason for hiding this comment

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

This seems like you're somewhat needlessly defining another proc macro which is ultimately just a 1-argument wrapper for Enumerated.

Integer is really not very descriptive name and kind of ambiguous here: what it's doing is mapping integers to enums, just like Enumerated.

I'd suggest just making the Enumerated macro more flexible instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add I said before, I already tried that. But Rust doesn't remove the type-level attribute, so it is passed through to the final AST and generates a compile error.

Copy link
Member

Choose a reason for hiding this comment

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

That must've been a bug in your code, as I've written many proc macros that do that without issue, including this one (see TypeAttrs)

Comment on lines +22 to +24
/// Whether or not to tag the enum as an integer
integer: bool,

Copy link
Member

Choose a reason for hiding this comment

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

This seems like an inextensible way to override the tag of an Enumerated, so why not just add a #[asn1(tag = "...")] argument instead that lets you override the ENUMERATED tag however you want? Then it could potentially work for more than just INTEGER.

@npmccallum
Copy link
Contributor Author

@tarcieri I have implemented it with the attributes. However, the error I mentioned before has returned:

error: cannot find attribute `asn1` in this scope
 --> pkcs10/src/version.rs:9:3
  |
9 | #[asn1(type = "INTEGER")]
  |   ^^^^

This is because the derive macro outputs the asn1 attribute in the final AST. But Rust doesn't know about this attribute.

@tarcieri
Copy link
Member

@npmccallum the problem is in the definition of the Enumerated proc macro:

https://github.com/RustCrypto/formats/blob/master/der/derive/src/lib.rs#L213-L218

#[proc_macro_derive(Enumerated)]
#[proc_macro_error]
pub fn derive_enumerated(input: TokenStream) -> TokenStream {
    let input = parse_macro_input!(input as DeriveInput);
    DeriveEnumerated::new(input).to_tokens().into()
}

You'll need to notate that it accepts an asn1 attribute. Change:

#[proc_macro_derive(Enumerated)]

to

#[proc_macro_derive(Enumerated, attributes(asn1))]

@npmccallum
Copy link
Contributor Author

@tarcieri THANK YOU! You have no idea how much I was pulling my hair out on that one.

@npmccallum
Copy link
Contributor Author

@tarcieri This is ready for final review now.

Comment on lines +46 to +60
if let Ok(Meta::List(MetaList { nested, .. })) = attr.parse_meta() {
for meta in nested {
if let NestedMeta::Meta(Meta::NameValue(nv)) = meta {
if nv.path.is_ident("type") {
if let Lit::Str(lit) = nv.lit {
match lit.value().as_str() {
"ENUMERATED" => integer = false,
"INTEGER" => integer = true,
s => abort!(lit, "`type = \"{}\"` is unsupported", s),
}
}
}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

This is... less than ideal, but I have some ideas about how to refactor this along with attribute parsing in general which I can do in a followup PR.

@tarcieri tarcieri merged commit 9c25616 into RustCrypto:master Feb 11, 2022
@npmccallum npmccallum deleted the integer branch March 10, 2022 20:46
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