Implement `Default` trait #480

flier · 2017-02-05T09:16:42Z

We need Default trait to handle so many auto generated fields when create new structure.

highfive · 2017-02-05T09:16:45Z

Warning

These commits modify unsafe code. Please review it carefully!

nox · 2017-02-05T13:22:46Z

This definitely needs a rebase.

emilio · 2017-02-05T13:50:42Z

I'm not too fond on this, what's the advantage vs. using std::mem::zeroed explicitly?

emilio · 2017-02-05T13:51:47Z

I guess you can do something like:

let whatever = StructName {
    field1: 1,
    .. Default::default()
};

But that seems like it can turn into a footgun to me.

nox · 2017-02-05T13:52:35Z

14:51 <nox> emilio: mem::zeroes is unsafe,
14:51 <nox> emilio: Default isn't,
14:51 <nox> emilio: and if that operation shouldn't be safe, why are all fields public?

flier · 2017-02-05T13:54:31Z

If we have Default, those auto generated fields could be private, or we have to set it with unsafe code. From my perspective, those fields are implementation details should be hidden from user.

emilio · 2017-02-05T14:14:49Z

14:56 <emilio> nox: the fields are public because otherwise it bindgen becomes a pain to interact with. Note we have configuration to annotate some of them so they're not public if wanted/needed, what other approach do you suggest?
14:56 <nox> emilio: Well, then the design is unsound anyway.
14:57 <nox> emilio: Forbidding Default for safety reasons, but keeping everything public, that is not consistent.
14:57 <nox> emilio: You could derive Default on everything without private fields, or something like that.
14:57 <emilio> nox: I'm not fond on it because of the complexity, not because of safety.
14:57 <nox> Ok
14:58 <nox> emilio: Because it's hard to know which fields implement Default?
14:58 <emilio> nox: pretty much, it's already a pain to track which of them can derive debug with template parameters and all the different configurations that the user can specify.

If we have Default, those auto generated fields could be private, or we have to set it with unsafe code. From my perspective, those fields are implementation details should be hidden from user.

That's a good point, but I'm not sure implementing Default is a good way to deal with this. Mainly, the semantics of Default are that the resulting value is useful/valid, and that's not guaranteed at all. As @nox said, there's nothing preventing you for doing that manually, that's fair, but I think it's slightly different. If this is implemented, this needs to be documented somewhere (that we implement Default for convenience to avoid having to specify padding fields, but that it's effectively std::mem::zeroed).

Anyway, will do a first review pass, I don't want my opinion to prevent adding this to bindgen if other people consider it's useful. @fitzgen, any opinion here?

emilio

A few nits here and there, otherwise looks ok. Please add a test for the config.

One question, what about if instead of tracking what types can and can't derive default, we simply add a convenience method to each struct/union?

I sincerely don't see the advantage of tracking all these fields if the result it's going to be a zeroed struct.

It makes sense when deriving copy because we don't want to implement it always (we don't do so when a type has a destructor, etc.). Similarly, it makes sense when deriving Debug because we can't write Debug manually, but I don't see the point in tracking whether Default is derivable if the result is just mem::zeroed().

Note that this will be UB if there's a rust-like enum inside the struct (it could have an invalid discriminant), as far as I know.

That's something you can't do with safe rust even when all fields are public, @nox.

emilio · 2017-02-05T14:19:56Z

src/codegen/mod.rs

@@ -689,6 +689,7 @@ impl<'a> CodeGenerator for Vtable<'a> {
            .item()
            .pub_()
            .with_attr(attributes::repr("C"))
+            .with_attr(attributes::derives(&["Default"]))


Please do this only if ctx.options().derive_default.

emilio · 2017-02-05T14:21:52Z

src/codegen/mod.rs

+        if item.can_derive_default(ctx, ()) {
+            derives.push("Default");
+        } else {
+            needs_default_impl = true;


This needs to be needs_default_impl = ctx.options().derive_default. Please add a test for this (see tests/headers/no-derive-debug.h for a similar example).

emilio · 2017-02-05T14:22:56Z

src/ir/comp.rs

@@ -891,6 +904,53 @@ impl CanDeriveDebug for CompInfo {
        can_derive_debug
    }
 }


nit: newline missing.

emilio · 2017-02-05T14:23:28Z

src/ir/comp.rs

+                          ctx: &BindgenContext,
+                          layout: Option<Layout>)
+                          -> bool {
+        {


nit: Why this block wrapping the full function? We should be able to just remove these braces and deindent it, right?

emilio · 2017-02-05T14:25:33Z

tests/expectations/tests/381-decltype-alias.rs

@@ -10,3 +10,6 @@ pub struct std_allocator_traits<_Alloc> {
    pub _address: u8,
    pub _phantom_0: ::std::marker::PhantomData<_Alloc>,
 }
+impl <_Alloc> Default for std_allocator_traits<_Alloc> {


Probably we shouldn't derive default for incomplete/unsized types, but i understand it's needed because of derive bounds with template parameters, right?

Yes, it's hard to trace which type could be used in where, such as base_members, template_args, fields or ref_template. So, the most straight way is to implement Default as much as possible.

flier · 2017-02-05T14:36:46Z

We have introduced a lot of auto generated fields to reconstruct C/C++/ObjC ABI, from a end user viewpoint, those public magic fields are more difficulty understanding than Default trait, even the Default trait is not a suitable semantics, it may better than _bitfield_1 or __bindgen_padding_0.

To use property could be a alternative, but Rust lack of Abstract and Concrete Fields in Traits like Scala did. It also hard to initialize or maintain a very complex data structure with properties.

On the other hand, I don't think we must trace all fields who implement Default, we just implement Default trait with derivie attribute as much as possible, use unsafe { mem::zeroed() } will be fine, who care how we implement it? It is just a syntactic sugar to make C/C++ happy, that's enough. :)

Anyway, I think we could support the Default trait and disable it by default, let's user choose to enable it or not.

emilio · 2017-02-05T14:47:20Z

We have introduced a lot of auto generated fields to reconstruct C/C++/ObjC ABI, from a end user viewpoint, those public magic fields are more difficulty understanding than Default trait, even the Default trait is not a suitable semantics, it may better than _bitfield_1 or __bindgen_padding_0.

Sure, I agree, over all if the fields (like the padding fields) depend on a per-platform basis. The padding fields will probably go away eventually once Rust supports custom alignment, and I think the others aren't that problematic IMO.

On the other hand, I don't think we must trace all fields who implement Default, we just implement Default trait with derivie attribute as much as possible, use unsafe { mem::zeroed() } will be fine, who care how we implement it? It is just a syntactic sugar to make C/C++ happy, that's enough. :)

Sure, my point is that I think we shouldn't bother trying to derive when possible, because that's most of the code this PR adds, and it's completely unneeded. You could argue for the readability of the bindings, but I don't think we should care a lot about it when it involves complexity in the code.

Anyway, I think we could support the Default trait and disable it by default, let's user choose to enable it or not.

Making it opt-in looks good to me, I think it's better.

flier · 2017-02-05T15:26:37Z

We have introduced a lot of auto generated fields to reconstruct C/C++/ObjC ABI, from a end user viewpoint, those public magic fields are more difficulty understanding than Default trait, even the Default trait is not a suitable semantics, it may better than _bitfield_1 or __bindgen_padding_0.

Sure, I agree, over all if the fields (like the padding fields) depend on a per-platform basis. The padding fields will probably go away eventually once Rust supports custom alignment, and I think the others aren't that problematic IMO.

Yes, the best solution is upstream Rust implement all of those dirty works in the language like Golang did, but I think it is a Utopia, we have to live to that one day with those dirty magic fields. Hide its implemented detail, simulate same memory layout and public the behavior will be better.

On the other hand, I don't think we must trace all fields who implement Default, we just implement Default trait with derivie attribute as much as possible, use unsafe { mem::zeroed() } will be fine, who care how we implement it? It is just a syntactic sugar to make C/C++ happy, that's enough. :)

Sure, my point is that I think we shouldn't bother trying to derive when possible, because that's most of the code this PR adds, and it's completely unneeded. You could argue for the readability of the bindings, but I don't think we should care a lot about it when it involves complexity in the code.

I think the most different between those two strategies is how we reconstruct the C/C++/ObjC ABI.

If we only use those plain data, such as build-in primitive types or array, I agree the unsafe { mem::zeroed() } could handle them all, because their Default doesn't need a real meaning, same memory layout could make C/C++ happy.

But, if we want to use those std::xxx or generated helper to simulate the C/C++/ObjC behaviors, #[derive(Default)] could give them a chance to implement Default behavior more than unsafe { mem::zeroed() }, we may need a whitelist to allow them later, or forbid to use any of them unless it's zeroed Default could works.

Till now, we only use some marker::PhantomData, VTable or other zero size helper, it safe to make them all mem::zeroed(). But I'm not sure what's your plan in future, that's why I implement Default with as much as possible strategy.

Anyway, I think we could support the Default trait and disable it by default, let's user choose to enable it or not.

Making it opt-in looks good to me, I think it's better.

emilio · 2017-02-05T20:27:52Z

Till now, we only use some marker::PhantomData, VTable or other zero size helper, it safe to make them all mem::zeroed(). But I'm not sure what's your plan in future, that's why I implement Default with as much as possible strategy.

I'd argue for keeping the complexity away for as long as we don't need it. A comment pointing that if more complex Default behavior is desired we could try to derive Default as needed would be helpful.

fitzgen · 2017-02-05T22:10:00Z

I don't have too many strong opinions here, but here are two thoughts:

Maintaining bindgen is fairly difficult, so it is important to minimize complexity by choosing to support only one of (a) default impls, and (b) the pub fields with mem::zeroed initialization.

It seems to me like in the long term the default impls is a more correct approach, but we do need to make sure it is robust (I haven't looked at any patches here).

emilio · 2017-02-06T18:26:41Z

Ok, let's do that then, @flier feel free to address comments and squash commits, and we'll merge this afterwards.

flier · 2017-02-07T01:34:35Z

Ok, let's do that then, @flier feel free to address comments and squash commits, and we'll merge this afterwards.

Glad to see it 👍

KiChjang · 2017-02-07T01:46:59Z

@flier This still hasn't been rebased against current master. The merge commits in your branch has to be dropped.

KiChjang · 2017-02-07T02:25:36Z

@flier Perhaps this guide can help you: https://github.com/servo/servo/wiki/Beginner's-guide-to-rebasing-and-squashing

flier · 2017-02-07T02:39:33Z

@flier Perhaps this guide can help you: https://github.com/servo/servo/wiki/Beginner's-guide-to-rebasing-and-squashing

Thanks @KiChjang, seems ok :)

emilio · 2017-02-07T12:25:02Z

src/lib.rs

@@ -581,6 +591,7 @@ impl Default for BindgenOptions {
            emit_ast: false,
            emit_ir: false,
            derive_debug: true,
+            derive_default: true,


Let's make it opt-in instead as discussed? Hmm, though that means that the option should be derive-default. I'm happy turning it on for testing so it gets more coverage (see the call to no_unstable_rust in test/test.rs).

Perhaps we should start doing something like --derive-default={yes,no}? Not sure how easy would that be.

What do you think?

I have added a new option with-derive-default and disable it by default, when we use it on tests, just enable the hidden no-derive-default option which has higher priority.

let prepend = ["bindgen", "--with-derive-default", header_str, "--raw-line", "", "--raw-line", "#![allow(non_snake_case)]", "--raw-line", ""]; let args = prepend.into_iter() .map(ToString::to_string) .chain(flags.into_iter()); builder_from_flags(args) .map(|(builder, _)| Some(builder.no_unstable_rust()))

emilio · 2017-02-07T15:58:04Z

@bors-servo r+

bors-servo · 2017-02-07T15:58:05Z

📌 Commit 5446818 has been approved by emilio

bors-servo · 2017-02-07T15:58:08Z

⌛ Testing commit 5446818 with merge 9bd602c...

Implement `Default` trait We need `Default` trait to handle so many auto generated fields when create new structure.

bors-servo · 2017-02-07T17:21:34Z

💔 Test failed - status-travis

emilio · 2017-02-07T19:50:18Z

You need to rebase to include the newest test changes, should be straight forward :)

emilio · 2017-02-08T10:01:43Z

@bors-servo r+

Can't wait for this to bitrot all the other PRs ;)

Thanks @flier!

bors-servo · 2017-02-08T10:01:44Z

📌 Commit 25b68ba has been approved by emilio

bors-servo · 2017-02-08T10:55:42Z

⌛ Testing commit 25b68ba with merge 86ce9dd...

Implement `Default` trait We need `Default` trait to handle so many auto generated fields when create new structure.

bors-servo · 2017-02-08T11:50:34Z

☀️ Test successful - status-travis
Approved by: emilio
Pushing 86ce9dd to master...

highfive added the S-awaiting-review label Feb 5, 2017

emilio reviewed Feb 5, 2017

View reviewed changes

flier force-pushed the impl-default branch from 6197b08 to 0314406 Compare February 7, 2017 02:38

emilio reviewed Feb 7, 2017

View reviewed changes

flier force-pushed the impl-default branch from 51a12a2 to 5446818 Compare February 7, 2017 14:28

highfive assigned emilio Feb 7, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 7, 2017

bors-servo pushed a commit that referenced this pull request Feb 7, 2017

Auto merge of #480 - flier:impl-default, r=emilio

9bd602c

Implement `Default` trait We need `Default` trait to handle so many auto generated fields when create new structure.

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 7, 2017

implement Default trait

25b68ba

flier force-pushed the impl-default branch from 5446818 to 25b68ba Compare February 8, 2017 02:55

highfive added S-awaiting-review and removed S-tests-failed labels Feb 8, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 8, 2017

bors-servo pushed a commit that referenced this pull request Feb 8, 2017

Auto merge of #480 - flier:impl-default, r=emilio

86ce9dd

Implement `Default` trait We need `Default` trait to handle so many auto generated fields when create new structure.

bors-servo merged commit 25b68ba into rust-lang:master Feb 8, 2017

highfive removed the S-awaiting-merge label Feb 8, 2017

This was referenced Feb 8, 2017

Template instantiation #491

Merged

ir: Don't parse non-semantic-children cursor as inner structs. #486

Merged

codegen: Fix remaining cases of missing core prefix. #489

Merged

Implement Default trait #480

Implement Default trait #480

Conversation

flier commented Feb 5, 2017

highfive commented Feb 5, 2017

nox commented Feb 5, 2017

emilio commented Feb 5, 2017

emilio commented Feb 5, 2017

nox commented Feb 5, 2017

flier commented Feb 5, 2017

emilio commented Feb 5, 2017 • edited Loading

emilio 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

Choose a reason for hiding this comment

flier commented Feb 5, 2017

emilio commented Feb 5, 2017

flier commented Feb 5, 2017

emilio commented Feb 5, 2017

fitzgen commented Feb 5, 2017

emilio commented Feb 6, 2017

flier commented Feb 7, 2017 • edited Loading

KiChjang commented Feb 7, 2017

KiChjang commented Feb 7, 2017

flier commented Feb 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emilio commented Feb 7, 2017

bors-servo commented Feb 7, 2017

bors-servo commented Feb 7, 2017

bors-servo commented Feb 7, 2017

emilio commented Feb 7, 2017

emilio commented Feb 8, 2017

bors-servo commented Feb 8, 2017

bors-servo commented Feb 8, 2017

bors-servo commented Feb 8, 2017

Implement `Default` trait #480

Implement `Default` trait #480

emilio commented Feb 5, 2017 •

edited

Loading

flier commented Feb 7, 2017 •

edited

Loading