Can derive default analysis by photoszzt · Pull Request #861 · rust-lang/rust-bindgen

photoszzt · 2017-07-27T18:28:57Z

fitzgen

Looking good! I left a bunch of comments, but its mostly just nitpicking.

fitzgen · 2017-07-27T20:10:14Z

src/ir/layout.rs

    type Extra = ();

-    fn can_derive_default(&self, _: &BindgenContext, _: ()) -> bool {
+    fn can_trivially_derive_default(&self, _: &BindgenContext, _: ()) -> bool {


it seems like this is the only impl, and since it is not using the Extra associated type, nor the ctx parameter, we should remove them.

fitzgen · 2017-07-27T20:13:42Z

tests/expectations/tests/issue-573-layout-test-failures.rs

 }
-impl Default for Outer {
-    fn default() -> Self { unsafe { ::std::mem::zeroed() } }
-}


This expectation change should be fine.

fitzgen · 2017-07-27T20:20:29Z

src/ir/comp.rs

-                          -> bool {
-        // We can reach here recursively via template parameters of a member,
-        // for example.
-        if self.detect_derive_default_cycle.get() {


We can also remove the detect_derive_default_cycle member now.

fitzgen · 2017-07-27T20:29:46Z

tests/expectations/tests/issue-584-stylo-template-analysis-panic.rs

 }
-impl <c> Default for e<c> {
-    fn default() -> Self { unsafe { ::std::mem::zeroed() } }
-}


This expectation change seems to be from how if a type is hidden/blacklisted it will never end up inside this analysis's worklist because it isn't in the whitelisted items set, and therefore will never end up in the cannot_derive_default set. This means that our default is "can derive" for these types.

It isn't clear to me what the old default was; I think we would have looked inside the blacklisted item, which I'm not sure is what we want either. The type is blacklisted for some reason or another, and bindgen doesn't have the whole picture so how can it make an opinionated decision?

In light of that, I think the best thing to do would be to act conservatively, similar to how we do with blacklisted items in the template parameter analysis. With template params, we conservatively say that blacklisted items use all of their template parameters. In this case, conservative means "assume it cannot derive Default if it is blacklisted". As far as implementing this behavior goes, it means that we need to initially populate cannot_derive_default with all the items that are reachable from the whitelisted item set, but are not actually in the whitelisted item set (in other words, populate it with the blacklisted items).

@emilio wdyt? Does defaulting to "we should conservatively deny deriving default for blacklisted items" sound right to you?

That sounds fine off-hand... In general note that deriving default is not on by default, so it's less likely to break stuff. As long as we keep it working and consistent I think i'd be fine with it.

fitzgen · 2017-07-27T20:36:14Z

tests/expectations/tests/issue-645-cannot-find-type-T-in-this-scope.rs

 pub type HasRefPtr_TypedefOfT<T> = T;
-impl <T> Default for HasRefPtr<T> {
-    fn default() -> Self { unsafe { ::std::mem::zeroed() } }
-}


This is the same thing as above.

fitzgen · 2017-07-27T20:50:20Z

src/ir/analysis/derive_default.rs

+        match *ty.kind() {
+            // Handle the simple cases. These can derive debug without further
+            // information.
+


Nitpick: remove this newline.

fitzgen · 2017-07-27T20:51:47Z

src/ir/analysis/derive_default.rs

+
+            TypeKind::Void |
+            TypeKind::Named |
+            TypeKind::TemplateInstantiation(..) |


Yeah, as mentioned on IRC, I think a template instantiation should derive default if its definition and arguments derive default.

fitzgen · 2017-07-27T20:55:16Z

src/ir/analysis/derive_default.rs

+                if info.needs_explicit_vtable(self.ctx, item) {
+                    trace!("    comp that needs vtable cannot derive Default");
+                    return self.insert(id);
+                }


Is it ever the case that a type can need an explicit vtable but not be in the has_vtable set??

I think this case is redundant...

fitzgen · 2017-07-27T20:55:57Z

src/ir/analysis/derive_default.rs

+                    }
+                }
+
+                if self.ctx.lookup_item_id_has_vtable(&item.id()) {


Nitpick:

if item.has_vtable(self.ctx) { ... }

fitzgen · 2017-07-27T20:57:42Z

src/ir/item.rs

-            }
-            _ => false,
-        }
+        ctx.options().derive_default && ctx.lookup_item_id_can_derive_default(self.id())


So there should only be two CanDeriveDefault impls left: Item and ItemId and neither use the Extra type parameter, so we should remove it now.

bors-servo · 2017-07-27T22:24:39Z

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

fitzgen · 2017-07-31T18:45:19Z

@bors-servo r+

bors-servo · 2017-07-31T18:45:20Z

📋 Looks like this PR is still in progress, ignoring approval

fitzgen · 2017-07-31T19:25:18Z

@bors-servo r+

bors-servo · 2017-07-31T19:25:18Z

📌 Commit 0231a21 has been approved by fitzgen

bors-servo · 2017-07-31T19:25:21Z

⌛ Testing commit 0231a21 with merge d1d3500...

@fitzgen

Can derive default analysis r? @fitzgen

bors-servo · 2017-07-31T20:02:37Z

☀️ Test successful - status-travis
Approved by: fitzgen
Pushing d1d3500 to master...

fitzgen reviewed Jul 27, 2017

View reviewed changes

can derive default analysis

0231a21

fitzgen changed the title ~~WIP can derive default analysis~~ Can derive default analysis Jul 31, 2017

bors-servo pushed a commit that referenced this pull request Jul 31, 2017

Auto merge of #861 - photoszzt:derive_default, r=fitzgen

d1d3500

Can derive default analysis r? @fitzgen

bors-servo merged commit 0231a21 into rust-lang:master Jul 31, 2017

This was referenced Jul 31, 2017

Can derive copy analysis #866

Merged

Feature 832 custom rust target #859

Merged

photoszzt deleted the derive_default branch July 31, 2017 20:30

fitzgen mentioned this pull request Aug 1, 2017

Rewrite can_derive_default as a fix-point analysis #856

Closed

Conversation

photoszzt commented Jul 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fitzgen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors-servo commented Jul 27, 2017

Uh oh!

fitzgen commented Jul 31, 2017

Uh oh!

bors-servo commented Jul 31, 2017

Uh oh!

fitzgen commented Jul 31, 2017

Uh oh!

bors-servo commented Jul 31, 2017

Uh oh!

bors-servo commented Jul 31, 2017

Uh oh!

bors-servo commented Jul 31, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

photoszzt commented Jul 27, 2017 •

edited

Loading