Implement cli option for custom derive #2328

hcldan · 2022-11-01T18:57:41Z

hcldan · 2022-11-01T19:20:27Z

not sure why rustfmt runner failed.

bindgen-cli/options.rs

pvdrz · 2022-11-01T20:50:56Z

bindgen-cli/options.rs

+                .value_name("trait")
+                .multiple_occurrences(true)
+                .number_of_values(1),
+            Arg::new("with-derive-custom-struct")


maybe I'm missing something but why aren't there equivalents for enum and union?

Because I didn't need those, and they caused huge issues with the Derive I was trying to use.
The support is there in the base project, I was implementing this because I need this on the cli.

bindgen-cli/options.rs

pvdrz · 2022-11-02T18:47:37Z

@hcldan could you add some tests?

bors-servo · 2022-11-02T18:49:17Z

☔ The latest upstream changes (presumably a673a6b) made this pull request unmergeable. Please resolve the merge conflicts.

pvdrz · 2022-11-21T16:45:52Z

Now that #2355 and #2330 have been merged it should be possible to:

Use --with-derive-custom and --with-derive-custom-struct at the same time without issue.
Add a field to DepInfo for handling structures instead of adding another method to ParseCallbacks.

It seems that taking a regex argument is something important here but that can be done in a different PR.

@hcldan let me know what do you think.

hcldan · 2022-11-23T15:04:47Z

I can look into doing this.
Sorry for the delay in responding, I was at a customer conference and then I got sick :)

hcldan · 2022-12-01T22:43:18Z

Tried to follow what was going on but I needed some internals.

LMK if this is looking like it's the right direction and I'll add some tests.

pvdrz

This looks great! I suggested changes about some small details but this is looking great :)

pvdrz · 2022-12-05T16:15:31Z

bindgen-cli/options.rs

+            fn add_derives(&self, info: &bindgen::callbacks::DeriveInfo<'_>) -> Vec<String> {
+                match self.kind.to_lowercase().as_str() {
+                    "struct" => match info.kind {
+                        Some(bindgen::ir::comp::CompKind::Struct) => {}, // continue


Hmm this brings up again a discussion we had with @emilio before. Should this <kind> be the kind of c types or rust types? For example some c unions are emitted as rust structs and CompKind doesn't include enums` as those aren't compound types per-se.

I'm more inclined to this based on the rust kinds instead as we're deriving rust traits on them but I'm open to hear other arguments.

That's a good question... I think I agree with you... which kind of kind (😄 ) am I using here?

I was not expecting to not find enums in the enum for CompKind (this is starting to get meta...)
So I gave up on the branch of code that handles everything but structs and unions.

If this is referencing C types, how do I get the rust types to check?

Deciding whether a comptype becomes a rust struct or union happens here:

rust-bindgen/bindgen/codegen/mod.rs

Lines 2122 to 2132 in 9c2167c

let mut tokens = if is_union && struct_layout.is_rust_union() {

quote! {

#( #attributes )*

pub union #canonical_ident

}

} else {

quote! {

#( #attributes )*

pub struct #canonical_ident

}

};

pvdrz · 2022-12-05T16:17:31Z

bindgen-cli/options.rs

+                .long("with-derive-custom")
+                .help(
+                    "Derive custom traits on any kind of type. \
+                     The derive value must be of the shape <regex>=<kind>=<derive> where <kind> can be one of: \


I'd be more inclined to keep this as you had it before --with-derive-custom-struct iirc and pass <regex>=<derive> as arguments.

hmm... ok but I might wait for resolution on the previous comment first

pvdrz · 2022-12-05T16:22:40Z

bindgen/callbacks.rs

 #[non_exhaustive]
 pub struct DeriveInfo<'a> {
    /// The name of the type.
    pub name: &'a str,
+    /// The kind of the type
+    pub kind: Option<CompKind>,


I think this shoudn't be wrapped in an Option but the user should be able to ignore the kind if they want to from the callback implementation directly:

fn add_derives(&self, info: &bindgen::callbacks::DeriveInfo<'_>) -> Vec<String> { let mut derives = vec![]; match &info.kind { CompKind::Struct => /* add derives exclusive for `struct`s */, CompKind::Union => /* add derives exclusive for `union`s */, } /* add derives for any kind of type */ derives }

Well I wasn't regexing on CompKind values, and there's no Any member of that enum, so I figured that if you wanted them all, we could use None, and if you wanted none then you shouldn't be using the option 😄

This was to allow for the other branch where I don't filter by CompKind because it is not available. I did not want to stop people who wanted to derive things on other rust types that support it.

I find the None confusing because if someone does something like:

fn add_derives(&self, info: &bindgen::callbacks::DeriveInfo<'_>) -> Vec<String> { if info.kind.is_none() { vec!["serde::Serialize"] } else { vec![] } }

Then Serialize would only be derived for rust enums instead of being for "every" kind of type definition.

pvdrz · 2022-12-08T19:43:24Z

bindgen/codegen/mod.rs

@@ -2105,6 +2105,7 @@ impl CodeGenerator for CompInfo {
        let custom_derives = ctx.options().all_callbacks(|cb| {
            cb.add_derives(&crate::callbacks::DeriveInfo {
                name: &canonical_name,
+                kind: Some(self.kind()),


Continuation of #2328 (comment)

So what I'd do is defining a new enum called TypeKind or ItemKind that can be used instead of CompKind inside of DeriveInfo:

enum WhateverKind { Struct, Enum, Union, }

and then use the condition is_union && struct_layout.is_rust_union() to decide if the current type will be represented as a rust union or as a struct and use WhateverKind::Union or WhateverKind::Struct accordingly instead of self.kind()

pvdrz · 2022-12-08T19:44:16Z

bindgen/codegen/mod.rs

-                cb.add_derives(&crate::callbacks::DeriveInfo { name: &name })
+                cb.add_derives(&crate::callbacks::DeriveInfo { 
+                    name: &name,
+                    kind: None,


Continuation of https://github.com/rust-lang/rust-bindgen/pull/2328/files#r1043753788

And here I'd use kind: WhateverKind::Enum as this is the CodeGenerator implementation for Enum.

In that way you don't need the Option wrapper either.

pvdrz · 2022-12-08T19:54:47Z

bindgen/callbacks.rs

 #[non_exhaustive]
 pub struct DeriveInfo<'a> {
    /// The name of the type.
    pub name: &'a str,
+    /// The kind of the type
+    pub kind: Option<CompKind>,


I find the None confusing because if someone does something like:

fn add_derives(&self, info: &bindgen::callbacks::DeriveInfo<'_>) -> Vec<String> { if info.kind.is_none() { vec!["serde::Serialize"] } else { vec![] } }

Then Serialize would only be derived for rust enums instead of being for "every" kind of type definition.

hcldan · 2022-12-08T20:54:59Z

@pvdrz Thank you for the feedback, I think these are good suggestions, but I don't have the time to address them :(
Work is piling up

pvdrz · 2022-12-08T21:35:18Z

I can do the extra changes if that's OK with you. This is really close to be done :)

hcldan · 2022-12-12T19:58:45Z

that would be great!

hcldan · 2023-01-03T18:15:48Z

looks like it doesn't want to derive the traits on unions. I ran into this at work, few (if any) derives have worked on any unions I've seen.

pvdrz · 2023-01-05T17:42:54Z

Yeah that part is a bit tricky but the actual main issue is that we cannot recover the CLI flags because we're using a callback. We're still discussing what's the best way forward.

bors-servo · 2023-01-12T20:40:40Z

☔ The latest upstream changes (presumably ed2d06e) made this pull request unmergeable. Please resolve the merge conflicts.

hcldan · 2023-01-26T19:05:18Z

@pvdrz any chance for a new release?

pvdrz · 2023-01-26T19:50:25Z

our release policy is whenever someone asks 🤣

I'll get a new release ready soon.

hcldan · 2023-01-27T15:12:03Z

@pvdrz Cheers!

hcldan · 2023-02-06T19:27:41Z

@pvdrz friendly reminder if it's not too much trouble, I could really use a new release to move to the functionality in this PR.

pvdrz · 2023-02-06T19:28:50Z

👋 hey I haven't forgot. I'm trying to get a new feature merged alongside this one before releasing a new version. If this feature does not land on friday I'll do a release anyway

pvdrz · 2023-02-07T15:42:12Z

@hcldan the new version has just been published

hcldan · 2023-02-07T15:57:55Z

@pvdrz ❤️ Thank you!

hcldan force-pushed the master branch from 4342828 to 8e3f9a8 Compare November 1, 2022 19:06

pvdrz requested changes Nov 2, 2022

View reviewed changes

pvdrz mentioned this pull request Nov 2, 2022

Add the --override-abi option #2329

Merged

pvdrz mentioned this pull request Nov 18, 2022

Introduce DeriveInfo #2355

Merged

hcldan force-pushed the master branch from 8e3f9a8 to 9c2167c Compare December 1, 2022 22:42

hcldan requested a review from pvdrz December 2, 2022 13:36

pvdrz reviewed Dec 5, 2022

View reviewed changes

pvdrz reviewed Dec 8, 2022

View reviewed changes

hcldan and others added 4 commits January 20, 2023 13:56

custom derives after DeriveInfo

2021799

Introduce TypeKind instead of CompKind

c0cdd8c

Add tests

225b118

Emit CLI flags for callbacks

c60ea04

pvdrz force-pushed the master branch from 1fb6fa2 to c60ea04 Compare January 20, 2023 19:10

pvdrz changed the base branch from master to main January 20, 2023 19:37

pvdrz added 3 commits January 20, 2023 14:41

update changelog

8474927

run rustfmt

3a70dba

fix tests

8c9e5eb

fix features

cbfa03d

pvdrz merged commit bca47cd into rust-lang:main Jan 20, 2023

hcldan mentioned this pull request Mar 3, 2023

Expanded RegEx Support? #2432

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement cli option for custom derive #2328

Implement cli option for custom derive #2328

hcldan commented Nov 1, 2022

hcldan commented Nov 1, 2022

pvdrz Nov 1, 2022

hcldan Nov 2, 2022

pvdrz commented Nov 2, 2022

bors-servo commented Nov 2, 2022

pvdrz commented Nov 21, 2022

hcldan commented Nov 23, 2022 •

edited

Loading

hcldan commented Dec 1, 2022

pvdrz left a comment

pvdrz Dec 5, 2022

hcldan Dec 5, 2022

pvdrz Dec 8, 2022

pvdrz Dec 5, 2022

hcldan Dec 5, 2022

pvdrz Dec 5, 2022 •

edited

Loading

hcldan Dec 5, 2022 •

edited

Loading

pvdrz Dec 8, 2022

pvdrz Dec 8, 2022

pvdrz Dec 8, 2022

pvdrz Dec 8, 2022

hcldan commented Dec 8, 2022

pvdrz commented Dec 8, 2022

hcldan commented Dec 12, 2022

hcldan commented Jan 3, 2023

pvdrz commented Jan 5, 2023

bors-servo commented Jan 12, 2023

hcldan commented Jan 26, 2023

pvdrz commented Jan 26, 2023

hcldan commented Jan 27, 2023

hcldan commented Feb 6, 2023

pvdrz commented Feb 6, 2023

pvdrz commented Feb 7, 2023

hcldan commented Feb 7, 2023

	let mut tokens = if is_union && struct_layout.is_rust_union() {
	quote! {
	#( #attributes )*
	pub union #canonical_ident
	}
	} else {
	quote! {
	#( #attributes )*
	pub struct #canonical_ident
	}
	};

Implement cli option for custom derive #2328

Implement cli option for custom derive #2328

Conversation

hcldan commented Nov 1, 2022

hcldan commented Nov 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvdrz commented Nov 2, 2022

bors-servo commented Nov 2, 2022

pvdrz commented Nov 21, 2022

hcldan commented Nov 23, 2022 • edited Loading

hcldan commented Dec 1, 2022

pvdrz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvdrz Dec 5, 2022 • edited Loading

Choose a reason for hiding this comment

hcldan Dec 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hcldan commented Dec 8, 2022

pvdrz commented Dec 8, 2022

hcldan commented Dec 12, 2022

hcldan commented Jan 3, 2023

pvdrz commented Jan 5, 2023

bors-servo commented Jan 12, 2023

hcldan commented Jan 26, 2023

pvdrz commented Jan 26, 2023

hcldan commented Jan 27, 2023

hcldan commented Feb 6, 2023

pvdrz commented Feb 6, 2023

pvdrz commented Feb 7, 2023

hcldan commented Feb 7, 2023

hcldan commented Nov 23, 2022 •

edited

Loading

pvdrz Dec 5, 2022 •

edited

Loading

hcldan Dec 5, 2022 •

edited

Loading