groundwork for rustc_clean/dirty improvements #44983

vitiral · 2017-10-02T17:38:15Z

This is a WIP PR that needs mentoring from @michaelwoerister.

There are several TODOs but no outstanding questions (except for the main one -- is this the right approach?)

This is the plumbing for supporing groups in rustc_clean(labels="..."), as well as supporting an except="..." which will remove the excepted labels in the "clean" check and then assert that they are dirty (this is still TODO).

See the code TODO's and example comments for a rough design.

I'd like to know if this is the design you would like to do, and then I can go about actually filling out the groups and implementing the remaining logic.

rust-highfive · 2017-10-02T17:38:28Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

vitiral · 2017-10-02T19:07:46Z

the failures are because TODO is depricated. It should compile and pass tests as-is.

vitiral · 2017-10-02T20:32:23Z

the goal of this is to make tests in #44924 more succinct, as well as make it easy to add additional assertions later. If we wanted to assert a different DefNode, we can just add it to the groups that it makes sense for and fix the tests (manually validating that the fixes make sense, obviously).

It will also reduce boilerplate. This:

#[rustc_clean(label="Hir", cfg="cfail3")]
#[rustc_clean(label="HirBody", cfg="cfail3")]
#[rustc_clean(label="TypeOfItem", cfg="cfail3")]
#[rustc_clean(label="GenericsOfItem", cfg="cfail3")]
#[rustc_clean(label="PredicatesOfItem", cfg="cfail3")]

Will become this:

#[rustc_clean(label="StructGroup", cfg="cfail3")]

This:


#[rustc_dirty(label="Hir", cfg="cfail2")]
#[rustc_dirty(label="HirBody", cfg="cfail2")]
#[rustc_dirty(label="TypeOfItem", cfg="cfail2")]
#[rustc_clean(label="GenericsOfItem", cfg="cfail2")]
#[rustc_clean(label="PredicatesOfItem", cfg="cfail2")]

Will become this:

#[rustc_clean(label="StructGroup", except="HirGroup,TypeOfItem", cfg="cfail2")]

Or equivalently this:

#[rustc_dirty(label="StructGroup", except="GenericsOfItem,PredicatesOfItem", cfg="cfail2")]

michaelwoerister · 2017-10-03T07:50:43Z

the failures are because TODO is depricated. It should compile and pass tests as-is.

You can use FIXME instead of TODO.

michaelwoerister · 2017-10-03T08:17:30Z

Thanks, @vitiral! I think this is a good initiative. I've been a bit skeptical initially because I like how an explicit list makes it easy to see what exactly is tested. However, I think that is outweighed by reducing the amount of redundancy and being able to test new kinds of DepNodes easily.

Let's iterate on the specific design some more:

I'm not a big fan of the "Group" suffix. I'm also not a big fan of the existing label="..." syntax :) A different syntax would be nice in both cases.
In theory, we could let the compiler look at what the attribute is attached to and let it infer from that which set of DepNodes is the relevant one. That wouldn't be hard to implement but I'm not sure if that much magic would make tests hard to read.

If we would infer the group, the above examples could look like:

#[rustc_clean(except="Hir, HirBody, TypeOfItem", cfg="cfail3")]
#[rustc_clean(cfg="cfail3")]
struct Abc { ... }

That's pretty nice, actually :) What do you think, @vitiral?

cc @nikomatsakis

vitiral · 2017-10-03T15:38:24Z

I think that sounds great! So are you thinking of removing Groups alltogether then?

I think far harder to read (or at least understand) is what might be missing in any assertion. This covers all possible things that might change for any DepNode, so it makes it very clear. Let me see how possible this is...

vitiral · 2017-10-03T17:25:54Z

Got some help from @nikomatsakis

We want to match on the Node enum, which can be gotten from hir.get. From there I will construct the expected fields that could change. The initial implementation will just use the ones suggested in #44924

vitiral · 2017-10-03T17:29:50Z

Also, I think the initial implementation will still support label=. If label is given, the old behavior will be used.

I'll open a new issue before this gets merged to remove that behavior. I don't want to change thousands of lines of code to get this feature.

vitiral · 2017-10-03T18:06:47Z

I opened #45009 to track this feature through to completion. I'd like to convert this PR to just support multiple DefNode's in label= and add the plumbing for building the "groups" (which will be inferred automatically instead of specified via label= or except=)

vitiral · 2017-10-03T18:10:52Z

So @michaelwoerister this PR should be good to go

michaelwoerister

Very good, thank you, @vitiral!

If you address the two nits this is ready to merge.

michaelwoerister · 2017-10-04T08:28:17Z

src/librustc_incremental/persist/dirty_clean.rs

+                if out.contains(label) {
+                    self.tcx.sess.span_fatal(
+                        item.span,
+                        &format!("dep-node label `{}` is repeated (possibly by a group)", label));


This doesn't really handle groups yet, does it?

michaelwoerister · 2017-10-04T08:31:56Z

src/librustc_incremental/persist/dirty_clean.rs

+        let mut out = Vec::with_capacity(labels.len());
+        let def_path_hash = self.tcx.def_path_hash(def_id);
+        for label in labels.iter() {
+            match DepNode::from_label_string(label, def_path_hash.clone()) {


The .clone() should be unnecessary as DefPathHash is Copy.

vitiral · 2017-10-04T13:20:55Z

@michaelwoerister just pushed an ammended commit with your changes.

michaelwoerister · 2017-10-04T14:56:54Z

@bors r+

Thanks, @vitiral!

bors · 2017-10-04T14:56:56Z

📌 Commit ad9b1ed has been approved by michaelwoerister

vitiral · 2017-10-04T15:07:19Z

@michaelwoerister forgot to mention: we won't be supporting "groups" at all -- just auto-detection. That's what you wanted right?

(technically we will create "hidden groups" to use when assigning which assertions to do when auto-detecting, but there won't be any user-facing groups)

michaelwoerister · 2017-10-04T16:11:11Z

Yes, that's what I expected.

vitiral · 2017-10-07T14:14:17Z

@michaelwoerister I've almost got part2 ready, but it depends on these changes (I didn't think it would take this long to merge). How should I proceed? Should I just push a PR that doesn't properly diff against master? I could also just edit these commits (and you could pause bors), as it's not like anyone depends on them.

bors · 2017-10-08T03:02:41Z

⌛ Testing commit ad9b1ed with merge 928a295...

@michaelwoerister

groundwork for rustc_clean/dirty improvements This is a WIP PR that needs mentoring from @michaelwoerister. There are several TODOs but no outstanding questions (except for the main one -- **is this the right approach?**) This is the plumbing for supporing groups in `rustc_clean(labels="...")`, as well as supporting an `except="..."` which will remove the excepted labels in the "clean" check and then assert that they are dirty (this is still TODO). See the code TODO's and example comments for a rough design. I'd like to know if this is the design you would like to do, and then I can go about actually filling out the groups and implementing the remaining logic.

bors · 2017-10-08T05:55:09Z

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing 928a295 to master...

rust-highfive assigned nikomatsakis Oct 2, 2017

michaelwoerister assigned michaelwoerister and unassigned nikomatsakis Oct 2, 2017

vitiral mentioned this pull request Oct 3, 2017

tracking issue for rustc_dirty/clean enhancements #45009

Closed

3 tasks

vitiral force-pushed the dirty_clean_groups branch from bab96fa to b5bae56 Compare October 3, 2017 18:09

vitiral force-pushed the dirty_clean_groups branch from b5bae56 to e7ff6f6 Compare October 3, 2017 18:12

michaelwoerister reviewed Oct 4, 2017

View reviewed changes

vitiral changed the title ~~Dirty clean groups~~ groundwork for rustc_clean/dirty improvements Oct 4, 2017

vitiral force-pushed the dirty_clean_groups branch from e7ff6f6 to ad9b1ed Compare October 4, 2017 13:20

groundwork for rust-lang#45009: rustc_dirty/clean enhancements

ad9b1ed

aidanhs added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 5, 2017

vitiral mentioned this pull request Oct 7, 2017

Incremental compilation auto assert for rustc_clean/dirty vitiral/rust#1

Closed

bors merged commit ad9b1ed into rust-lang:master Oct 8, 2017

vitiral deleted the dirty_clean_groups branch October 8, 2017 15:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

groundwork for rustc_clean/dirty improvements #44983

groundwork for rustc_clean/dirty improvements #44983

vitiral commented Oct 2, 2017 •

edited

Loading

rust-highfive commented Oct 2, 2017

vitiral commented Oct 2, 2017

vitiral commented Oct 2, 2017 •

edited

Loading

michaelwoerister commented Oct 3, 2017

michaelwoerister commented Oct 3, 2017

vitiral commented Oct 3, 2017

vitiral commented Oct 3, 2017 •

edited

Loading

vitiral commented Oct 3, 2017

vitiral commented Oct 3, 2017

vitiral commented Oct 3, 2017

michaelwoerister left a comment

michaelwoerister Oct 4, 2017

michaelwoerister Oct 4, 2017

vitiral commented Oct 4, 2017

michaelwoerister commented Oct 4, 2017

bors commented Oct 4, 2017

vitiral commented Oct 4, 2017 •

edited

Loading

michaelwoerister commented Oct 4, 2017

vitiral commented Oct 7, 2017

bors commented Oct 8, 2017

bors commented Oct 8, 2017

groundwork for rustc_clean/dirty improvements #44983

groundwork for rustc_clean/dirty improvements #44983

Conversation

vitiral commented Oct 2, 2017 • edited Loading

rust-highfive commented Oct 2, 2017

vitiral commented Oct 2, 2017

vitiral commented Oct 2, 2017 • edited Loading

michaelwoerister commented Oct 3, 2017

michaelwoerister commented Oct 3, 2017

vitiral commented Oct 3, 2017

vitiral commented Oct 3, 2017 • edited Loading

vitiral commented Oct 3, 2017

vitiral commented Oct 3, 2017

vitiral commented Oct 3, 2017

michaelwoerister left a comment

Choose a reason for hiding this comment

michaelwoerister Oct 4, 2017

Choose a reason for hiding this comment

michaelwoerister Oct 4, 2017

Choose a reason for hiding this comment

vitiral commented Oct 4, 2017

michaelwoerister commented Oct 4, 2017

bors commented Oct 4, 2017

vitiral commented Oct 4, 2017 • edited Loading

michaelwoerister commented Oct 4, 2017

vitiral commented Oct 7, 2017

bors commented Oct 8, 2017

bors commented Oct 8, 2017

vitiral commented Oct 2, 2017 •

edited

Loading

vitiral commented Oct 2, 2017 •

edited

Loading

vitiral commented Oct 3, 2017 •

edited

Loading

vitiral commented Oct 4, 2017 •

edited

Loading