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.

Sign up for GitHub

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

Jump to bottom

Allow custom derives and attributes #1089

Closed
RReverser opened this issue Oct 18, 2017 · 15 comments
Closed

Allow custom derives and attributes #1089

RReverser opened this issue Oct 18, 2017 · 15 comments
Labels
E-less-easy enhancement

Comments

@RReverser
Copy link

Several times I found myself in need to derive traits that are not autogenerated by bindgen, and, unfortunately, Rust doesn't allow to put a derive directive (or any attribute) at distance from the original type, even in the same file.

Would it make sense to provide ability to add custom derives or even any attributes to generated types via ParseCallbacks or some other mechanism?
@fitzgen
Copy link
Member

fitzgen commented Oct 23, 2017

This seems like a good feature to have.

I'd prefer not using ParseCallbacks if possible, because avoiding them allows easier testing, and I believe that custom derives is specific enough that it doesn't need the open ended programmatic access that ParseCallbacks provides.

As far as CLI syntax: --custome-derive '<trait>=<regex>'?

@RReverser
Copy link
Author

RReverser commented Oct 23, 2017 •
edited

@fitzgen Thing is, some derives still accept options via extra attributes (for example, https://github.com/kwohlfahrt/enum-tryfrom), so I'm not sure if it's worth locking up to just trait name. Maybe accept a raw "header" string to prepend to the type definition? Or at least attribute value, and then "derive" is just one of attribute types like --prepend-attr 'derive(Default)=<regex>'.

@fitzgen
Copy link
Member

fitzgen commented Oct 23, 2017

Good catch. I guess that it does make sense to have an escape hatch for more complicated cases in ParseCallbacks -- but I'd still prefer the simple versions to be supported on the CLI for easier testing (and in general it is best when things are usable in either scenario).

@RReverser
Copy link
Author

What do you think about the attribute approach then? (allowing any attributes to be appended)
Should be still CLI compatible while allowing these more complex cases.

@fitzgen
Copy link
Member

fitzgen commented Oct 23, 2017

Sorry I totally thought throught that stuff in my head and then failed to leave any comment about it :-p

The problem is that we would need to parse arbitrary attributes to know which = we are splitting on between the attr and the regex. This is because of attrs like #[serde(rename = "blah")]. I guess we could use https://dtolnay.github.io/syn/syn/fn.parse_outer_attr.html, but that signature doesn't seem to allow trailing bits or return where the parsing ended...

@RReverser
Copy link
Author

@fitzgen Maybe could use some other delimiter then, like "=>" or whatever?

@RReverser
Copy link
Author

RReverser commented Oct 23, 2017 •
edited

Or, actually, could as well put the regexp first and start at "#" then like regex#[derive(Default)] or regex=#[derive(Default)] or even just regex=derive(Default) avoiding '#' completely.

@fitzgen
Copy link
Member

fitzgen commented Oct 23, 2017

Yeah, I think that works

@fitzgen fitzgen changed the title Allow custom derives Allow custom derives and attributes Oct 24, 2017
@fitzgen fitzgen mentioned this issue Oct 24, 2017
Closed
@joelgallant
Copy link

This would be nice for serde derives

@meme
Copy link

meme commented Oct 4, 2019

Looking for this as well, willing to work on it -- has any work since been done?

@skywhale skywhale mentioned this issue Dec 5, 2019
Merged
@pheki pheki mentioned this issue Feb 24, 2020
Closed
@kpozin kpozin mentioned this issue Jun 12, 2020
Merged
bercarug added a commit to aws/aws-nitro-enclaves-cli that referenced this issue Jul 14, 2020
@bercarug
cli: Generate driver structs automatically using bindgen
f1e1f57 
Added a build script that automatically generates
rust bindings for the C structures used by the NE driver.
Currently, rust does not allow to implement a destructor
and to derive the `Copy` trait for a structure. By default,
bindgen derives `Copy` for the generated structures.
Although the behavior can be altered by using the
Builder's `no_copy()` method, this will also prevent
deriving `Clone` (which in turn makes the binding
unusable).

According to these sources (https://users.rust-lang.org/t/how-to-derive-custom-traits-for-struct-definitions-generated-by-bindgen/39710,
rust-lang/rust-bindgen#1089),
adding custom derives for the generated bindings is not
currently supported.

Update: as a workaround (for `ne_user_memory_region`),
the bindgen generated struct has been kept and the CLI's
MemoryRegion now contains a method for converting between
its own format and the driver struct format. This is used
for converting to the driver struct representation, when
issuing a `NE_SET_USER_MEMORY_REGION` ioctl.

The conversion between the two mentioned types is done
by implementing the `From` trait for UserMemoryRegion (
which also automatically provides an implementation for
`.into()`, for constructing a UserMemoryRegion instance
from a MemoryRegion one).

Signed-off-by: Gabriel Bercaru <bercarug@amazon.com>
acipu-aws pushed a commit to aws/aws-nitro-enclaves-cli that referenced this issue Oct 22, 2020
@bercarug @acipu-aws
cli: Generate driver structs automatically using bindgen
8ff8bda 
Added a build script that automatically generates
rust bindings for the C structures used by the NE driver.
Currently, rust does not allow to implement a destructor
and to derive the `Copy` trait for a structure. By default,
bindgen derives `Copy` for the generated structures.
Although the behavior can be altered by using the
Builder's `no_copy()` method, this will also prevent
deriving `Clone` (which in turn makes the binding
unusable).

According to these sources (https://users.rust-lang.org/t/how-to-derive-custom-traits-for-struct-definitions-generated-by-bindgen/39710,
rust-lang/rust-bindgen#1089),
adding custom derives for the generated bindings is not
currently supported.

Update: as a workaround (for `ne_user_memory_region`),
the bindgen generated struct has been kept and the CLI's
MemoryRegion now contains a method for converting between
its own format and the driver struct format. This is used
for converting to the driver struct representation, when
issuing a `NE_SET_USER_MEMORY_REGION` ioctl.

The conversion between the two mentioned types is done
by implementing the `From` trait for UserMemoryRegion (
which also automatically provides an implementation for
`.into()`, for constructing a UserMemoryRegion instance
from a MemoryRegion one).

Signed-off-by: Gabriel Bercaru <bercarug@amazon.com>
@emilio
Copy link
Contributor

emilio commented Apr 6, 2021

I don't think there has been much progress here, but I don't think this should be particularly hard to implement, happy to help.

@emilio emilio mentioned this issue Apr 6, 2021
Closed
@tbodt
Copy link
Contributor

tbodt commented Apr 6, 2021

I want my derive to apply to all structs, which would simplify the implementation for me. Perhaps I can implement that and leave it open to be extended to a regex?

This was referenced Apr 14, 2021
Closed
Closed
@ericseppanen ericseppanen mentioned this issue Jun 3, 2021
Merged
@trobanga
Copy link

trobanga commented Nov 7, 2021

It would be nice, if it would work with enums too. If I'm right then the current implementation of #2059 does only work with structs, or does it?

@ericseppanen
Copy link
Contributor

It would be nice, if it would work with enums too. If I'm right then the current implementation of #2059 does only work with structs, or does it?

I agree, it doesn't seem to work.

@ericseppanen ericseppanen mentioned this issue Nov 7, 2021
Closed
@yshui yshui mentioned this issue Nov 8, 2021
Open
@jmerdich jmerdich mentioned this issue Feb 3, 2022
Merged
@pvdrz
Copy link
Contributor

pvdrz commented Sep 20, 2022

I'm closing this issue as the enums support was fixed in #2117

@pvdrz pvdrz closed this as completed Sep 20, 2022
@Crzyrndm Crzyrndm mentioned this issue May 5, 2023
Open
@andreeaflorescu andreeaflorescu mentioned this issue Mar 6, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
E-less-easy enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
9 participants
@fitzgen @RReverser @emilio @joelgallant @tbodt @trobanga @meme @pvdrz @ericseppanen