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

Add support for custom attributes #2866

Merged
merged 1 commit into from
Sep 3, 2024
Merged

Conversation

mkroening
Copy link
Contributor

This adds support for custom attributes:

  • ParseCallbacks::add_attributes(&self, info: &AttributeInfo<'_>) -> Vec<TokenStream>
  • --with-attribute-custom <CUSTOM>
  • --with-attribute-custom-struct <CUSTOM>
  • --with-attribute-custom-enum <CUSTOM>
  • --with-attribute-custom-union <CUSTOM>

The implementation is very similar to the custom derives functionality. One thing I am not sure about is returning Vec<TokenStream> instead of Vec<String>, but that seemed natural to me.

Fixes #2520

Copy link
Contributor

@pvdrz pvdrz left a comment

Choose a reason for hiding this comment

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

Hi @mkroening sorry for the delay with the review.

I only have two requests for you because otherwise this looks great:

  • It is better to use String instead of TokenStream as the latter leaks proc_macro2 into our public API, meaning that we would have to do releases from time to time just to keep up to date with proc_macro2.
  • There should be a test for the CLI args and also some documentation explaining the syntax of the attributes as right now.

Edit: I also have an additional question for you that is non-blocking and can be addressed later (even by someone else). Would it be possible to re-implement the custom derives functionality using the custom attributes machinery instead?

@mkroening
Copy link
Contributor Author

Thanks for the review!

  • It is better to use String instead of TokenStream as the latter leaks proc_macro2 into our public API, meaning that we would have to do releases from time to time just to keep up to date with proc_macro2.

  • There should be a test for the CLI args

and also some documentation explaining the syntax of the attributes as right now.

Where would I put that? I did not find anything similar for --with-derive-custom.

Would it be possible to re-implement the custom derives functionality using the custom attributes machinery instead?

That should be possible! Would you like me to tackle this in this PR or in a separate one?

I have also implemented support for annotations, but error reporting is difficult there. It currently panics, if the attribute is not parsable as TokenStream.

@pvdrz
Copy link
Contributor

pvdrz commented Aug 29, 2024

and also some documentation explaining the syntax of the attributes as right now.

Where would I put that? I did not find anything similar for --with-derive-custom.

This would go in bindgen-cli as part of the docstring for the respective fields so clap can generate the --help message with that documentation. This is how it looks for the --with-derive-custom.* flags:

$ bindgen --help
Generates Rust bindings from C/C++ headers.

Usage: bindgen <FLAGS> <OPTIONS> <HEADER> -- <CLANG_ARGS>...

Arguments:
  [HEADER]         C or C++ header file
  [CLANG_ARGS]...  Arguments to be passed straight through to clang

Options:
      ...
      --with-derive-custom <CUSTOM>
          Derive custom traits on any kind of type. The CUSTOM value must be of the shape REGEX=DERIVE where DERIVE is a coma-separated list of derive macros
      --with-derive-custom-struct <CUSTOM>
          Derive custom traits on a `struct`. The CUSTOM value must be of the shape REGEX=DERIVE where DERIVE is a coma-separated list of derive macros
      --with-derive-custom-enum <CUSTOM>
          Derive custom traits on an `enum. The CUSTOM value must be of the shape REGEX=DERIVE where DERIVE is a coma-separated list of derive macros
      --with-derive-custom-union <CUSTOM>
          Derive custom traits on a `union`. The CUSTOM value must be of the shape REGEX=DERIVE where DERIVE is a coma-separated list of derive macros

Would it be possible to re-implement the custom derives functionality using the custom attributes machinery instead?

That should be possible! Would you like me to tackle this in this PR or in a separate one?

I'd do it on a separate one so it is easier to review and track.

Thanks!

@mkroening
Copy link
Contributor Author

This would go in bindgen-cli as part of the docstring for the respective fields so clap can generate the --help message with that documentation. This is how it looks for the --with-derive-custom.* flags:

Ah, that's what you mean. I wondered, since I had already done that when opening this PR. Perhaps you overlooked it?

I have also fixed the formatting for CI.

@pvdrz
Copy link
Contributor

pvdrz commented Aug 30, 2024

Oh my bad, it seems my head just skipped that section of code 🤣. It seems there are some compilation errors due to the change from TokenStream to String, could you fix those so we can roll this up?. Thanks!

@mkroening mkroening force-pushed the attributes branch 4 times, most recently from fd1409c to 8881246 Compare August 31, 2024 11:16
Signed-off-by: Martin Kröning <[email protected]>
@mkroening
Copy link
Contributor Author

The CI is green now. :)

@pvdrz pvdrz added this pull request to the merge queue Sep 3, 2024
Merged via the queue into rust-lang:main with commit 9a8e5ca Sep 3, 2024
33 checks passed
@mkroening mkroening deleted the attributes branch September 3, 2024 14:45
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.

Support conditional derives / custom attributes
2 participants