Add --no-default flag #1654

fletcherw · 2019-10-23T17:22:35Z

There's still one test failure I need to fix, namely, for the input file:

class Foo;
template <typename T>
struct RefPtr {
  T* m_inner;
};

struct Bar {
  RefPtr<Foo> m_member;
};

The problem here is that although Foo can neither derive nor Impl Default, RefPtr has a templated Default impl which should be at least sufficient to Impl Default for Bar, if not derive it.

This fails though because when running constrain_join() on RefPtr<Foo>, it considers the edge to Foo, sees that Foo is CanDerive::No, and so constrains RefPtr<Foo> to being CanDerive::No (when it really should be at least CanDerive::Manually if not CanDerive::Yes). See trace.txt for a trace of the derive stage where this happens.

Any ideas on how I could fix this?

highfive · 2019-10-23T17:22:40Z

Warning

These commits modify unsafe code. Please review it carefully!

fletcherw · 2019-10-23T17:28:41Z

tests/expectations/tests/layout_array.rs

@@ -284,6 +284,13 @@ impl Default for rte_mempool_ops_table {
        unsafe { ::std::mem::zeroed() }
    }
 }
+impl ::std::cmp::PartialEq for rte_mempool_ops_table {


I was unsure if this test output should be changed or not, but I think because all of the fields of the struct except the padding have PartialEq implementations, this should be valid (as long as the [rte_mempool_ops; 16] has PartialEq if rte_mempool_ops has PartialEq).

Yeah, this looks ok, I think.

fletcherw · 2019-10-23T17:45:11Z

Here's the failed output for that test: https://travis-ci.com/rust-lang/rust-bindgen/jobs/248826027#L1000

fletcherw · 2019-10-23T19:05:42Z

src/ir/analysis/derive.rs

@@ -143,7 +143,10 @@ impl<'ctx> CannotDerive<'ctx> {
                "    cannot derive {} for blacklisted type",
                self.derive_trait
            );
-            return CanDerive::No;
+            match self.derive_trait {


This seems like a bit of a hack, but it does represent the actual behavior. Before this patch, a CanDerive::No for Default implies that needs_default_impl is still true in codegen/mod.rs as long as the derive_default option is enabled and the type is not a forward declaration. So changing it Manually for blacklisted types doesn't change the behavior as far as I can tell.

I don't think the previous behavior made particular sense... I'd be fine with making the behavior change here so all derive traits are consistent.

emilio

Some general comments below, I need to page a bunch of this code back in :)

One thing I see is that this has changes that are specific to stuff with vtables and unions and such... Are those required to keep existing tests passing? Otherwise it'd require new tests.

emilio · 2019-10-26T11:12:27Z

src/ir/analysis/derive.rs

@@ -143,7 +143,10 @@ impl<'ctx> CannotDerive<'ctx> {
                "    cannot derive {} for blacklisted type",
                self.derive_trait
            );
-            return CanDerive::No;
+            match self.derive_trait {


I don't think the previous behavior made particular sense... I'd be fine with making the behavior change here so all derive traits are consistent.

emilio · 2019-10-26T11:14:46Z

tests/headers/no-default-opaque.hpp

@@ -0,0 +1,7 @@
+// bindgen-flags: --no-default "NoDefault" --opaque-type "NoDefault"
+
+// `--with-derive-default` is added by the test runner implicitly.


Does it complain if you add the flag anyways? If not I'd just do that.

Unfortunately the duplicate flag causes an error since the "with-derive-default" flag was not defined with multiple=true, so I can't add it again.

src/ir/analysis/derive.rs

emilio · 2019-10-26T11:21:12Z

src/ir/analysis/derive.rs

@@ -549,8 +611,8 @@ impl DeriveTrait {
    fn can_derive_pointer(&self) -> CanDerive {
        match self {
            DeriveTrait::Default => {
-                trace!("    pointer cannot derive Default");
-                CanDerive::No
+                trace!("    pointer cannot derive Default, but it may be implemented");


Hmm, how can it be implemented for a pointer?

Literally speaking we can't implement anything on pointers, but

struct Test { field: *mut i32, } impl Default for Test { fn default() -> Self { unsafe { ::std::mem::zeroed() } } }

does seem to compile for composite types that contain pointers.

emilio · 2019-10-26T11:23:15Z

tests/expectations/tests/layout_array.rs

@@ -284,6 +284,13 @@ impl Default for rte_mempool_ops_table {
        unsafe { ::std::mem::zeroed() }
    }
 }
+impl ::std::cmp::PartialEq for rte_mempool_ops_table {


Yeah, this looks ok, I think.

fletcherw · 2019-10-28T20:13:50Z

The changes within derive are all to keep existing tests passing, I don't think much has changed to necessitate new tests (for the first commit) but I'll double check.

fletcherw · 2019-11-11T18:28:15Z

Hey @emilio would you mind taking a look at the updated patch when you have a moment? Thank you!

emilio · 2019-11-14T16:39:20Z

Hi, sorry :)

This looks fine, though it seems there are a couple tests failing on CI? Let me know if I can help figure those out or what not.

fletcherw · 2019-11-18T17:48:30Z

Great! It's just the one test failing, but I think it's a slightly deeper problem with how we treat the dependency edge from RefPtr<Foo> to Foo there in the CanDerive analysis, so it might need some more significant changes.

Could you take a look at that and let me know what you think? There are more details and some traces in the first comment.

fletcherw · 2019-12-02T20:17:44Z

@emilio Please see my previous comment when you can!

bors-servo · 2019-12-11T00:48:56Z

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

emilio · 2019-12-11T02:09:25Z

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

Sorry for this, I'm happy to rebase this myself. This is still on my list, it's just harder than #1691 :)

fletcherw · 2019-12-11T18:48:29Z

No worries! Thanks for the status update.

For analyze::<CannotDerive>(), differentiate between traits which we can derive, traits we cannot derive but can Impl, and traits which we can neither derive nor Impl. These states are, respectively, CanDerive::Yes, CanDerive::Manually, and CanDerive::No. Convert BindgenContext's cannot_derive_default from a HashSet<ItemId> to a HashMap<ItemId, CanDerive>. Change codegen to only generate a Default Impl for ItemIds which are CanDerive::Manually (which we lookup in the cannot_derive_default set). By doing this, we can differentiate between ItemIds which don't support deriving Default, but still need an Impl, and ItemIds for which we do not want a derived or Impl'ed Default (namely types which are blacklisted using the forthcoming "--no-default" flag).

Add a command line flag "--no-default [regex]" which ensures that any types matching "[regex]" will neither derive nor Impl Default. Add the corresponding builder command "no_default(regex)" which has the same behavior for invocations of bindgen via Builder.

fletcherw · 2020-01-16T18:24:16Z

Rebased. There's an additional test failing (header_objc_template_h), but that test is also failing in master. Are you still planning on looking at the failing test @emilio or should I dig into it more?

bors-servo · 2020-06-15T15:40:38Z

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

pvdrz · 2022-08-31T21:53:39Z

I'm closing this PR as #1930 already implements this functionality

highfive added the S-awaiting-review label Oct 23, 2019

fletcherw commented Oct 23, 2019

View reviewed changes

emilio reviewed Oct 26, 2019

View reviewed changes

fletcherw force-pushed the no-default-flag branch from 46949e2 to e1b3828 Compare October 28, 2019 20:47

highfive added the S-needs-rebase label Dec 11, 2019

fletcherw added 2 commits January 16, 2020 11:19

fletcherw force-pushed the no-default-flag branch from e1b3828 to 1e00d63 Compare January 16, 2020 18:22

pvdrz closed this Aug 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --no-default flag #1654

Add --no-default flag #1654

fletcherw commented Oct 23, 2019 •

edited

highfive commented Oct 23, 2019

fletcherw Oct 23, 2019

emilio Oct 26, 2019

fletcherw commented Oct 23, 2019

fletcherw Oct 23, 2019

emilio Oct 26, 2019

emilio left a comment

emilio Oct 26, 2019

emilio Oct 26, 2019

fletcherw Oct 28, 2019

emilio Oct 26, 2019

fletcherw Oct 28, 2019

emilio Oct 26, 2019

fletcherw commented Oct 28, 2019

fletcherw commented Nov 11, 2019

emilio commented Nov 14, 2019

fletcherw commented Nov 18, 2019 •

edited

fletcherw commented Dec 2, 2019

bors-servo commented Dec 11, 2019

emilio commented Dec 11, 2019

fletcherw commented Dec 11, 2019

fletcherw commented Jan 16, 2020

bors-servo commented Jun 15, 2020

pvdrz commented Aug 31, 2022

		@@ -0,0 +1,7 @@
		// bindgen-flags: --no-default "NoDefault" --opaque-type "NoDefault"

		// `--with-derive-default` is added by the test runner implicitly.

Add --no-default flag #1654

Add --no-default flag #1654

Conversation

fletcherw commented Oct 23, 2019 • edited

highfive commented Oct 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fletcherw commented Oct 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

fletcherw commented Oct 28, 2019

fletcherw commented Nov 11, 2019

emilio commented Nov 14, 2019

fletcherw commented Nov 18, 2019 • edited

fletcherw commented Dec 2, 2019

bors-servo commented Dec 11, 2019

emilio commented Dec 11, 2019

fletcherw commented Dec 11, 2019

fletcherw commented Jan 16, 2020

bors-servo commented Jun 15, 2020

pvdrz commented Aug 31, 2022

fletcherw commented Oct 23, 2019 •

edited

fletcherw commented Nov 18, 2019 •

edited