Separate function bodies from their signatures in HIR #37918

flodiebold · 2016-11-21T18:17:13Z

Also give them their own dep map node.

I'm still unhappy with the handling of inlined items (1452edc1), but maybe you have a suggestion how to improve it.

rust-highfive · 2016-11-21T18:17:27Z

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.

nikomatsakis

OK, this is looking really good! I had a few comments as to possible other ways that we could do things. I'm not sure which, if any, we should do before landing. I might have to experiment locally to form a real opinion in any case. Curious to get your opinion. These are the major themes I remember:

use a separate method for determining whether to visit bodies or not?
should we use DepNode::HirBody, or else give def-ids to fn bodies and use DepNode::Hir?
how to fix the collect edges? -- pushed some commits into the branch that fix it

I have some thoughts on the last one, but I think it'd be easier for me to just experiment locally and send you a diff if what I am saying makes sense. =)

nikomatsakis · 2016-11-21T19:12:59Z

src/librustc/hir/intravisit.rs

-impl<'v> Visitor<'v> for IdRangeComputingVisitor {
+impl<'a, 'ast> Visitor<'ast> for IdRangeComputingVisitor<'a, 'ast> {
+    fn nested_visit_map(&mut self) -> Option<(&Map<'ast>, NestedVisitMode)> {
+        Some((&self.map, NestedVisitMode::OnlyBodies))


Ah, of course... in our latest comments on the issue, I had forgotten the central problem where you want to visit fn bodies as the default, but of course we don't have the map by default... my inclination is to make the nested_body_map be a separate method, and make it have no default implementation, so that we always specify it for every visitor. I feel like it is potentially much more surprising to skip over bodies.

(Plus then we can avoid the NestedVisitMode tuple)

nikomatsakis · 2016-11-21T19:31:58Z

src/librustc/hir/map/blocks.rs

        self.handle(|i: ItemFnParts<'a>|  i.body,
                    |_, _, _: &'a ast::MethodSig, _, body: ast::ExprId, _, _|  body,
                    |c: ClosureParts<'a>| c.body)
    }

    pub fn decl(self) -> &'a FnDecl {
+        if let map::NodeInlinedItem(&InlinedItem { kind: InlinedItemKind::Fn(ref decl), .. }) = self.node {
+            return &decl;
+        }
        self.handle(|i: ItemFnParts<'a>|  &*i.decl,


Seems like the most natural way to extend this code would be extend handle to cover this case, probably by adding a fourth closure callback (inlined-item).

nikomatsakis · 2016-11-21T19:35:54Z

src/librustc_passes/consts.rs

-                                      fn_like.body(),
-                                      fn_like.span(),
-                                      fn_like.id());
+            let qualif = match self.tcx.const_qualif_map.borrow_mut().entry(fn_like.body().node_id()) {


What is happening here? Is this populated for inlined items or something? At minimum, some comments seem good.

Ah, yeah. Because the FnLike interface is only half implemented for inlined items, the fn_like call would fail for them, but the const_qualif_map is already populated for them, so I added the check beforehand. I'll need to clean that up somehow...

nikomatsakis · 2016-11-21T19:39:32Z

src/librustc/middle/cstore.rs

-    Item(DefId /* def-id in source crate */, P<hir::Item>),
-    TraitItem(DefId /* impl id */, P<hir::TraitItem>),
-    ImplItem(DefId /* impl id */, P<hir::ImplItem>)
+pub struct InlinedItem {


It's hard for me to assess if this is the best way -- vs say trying to encode the fn bodies separately. Apart from some nits (which I tried to cite), it does seem a bit cleaner than what was here before, though. Perhaps @eddyb has an opinion. But overall I think I like it?

Seems okay - shouldn't need the type of a constant though, in fact, you should be able to only record a Vec<DefId> for the arguments of a const fn, which doesn't even need to be in a separate enum anymore, just replace the kind field with const_fn_args: Vec<DefId> and that should be that.

That would make implementing FnLike even harder, though, I think.

You shouldn't need to implement FnLike though.

lookup_const_fn_by_id returns an FnLikeNode currently. But I guess it could return something else, e.g. a new enum?

Yeah, that would make more sense, FnLikeNode is more about pretty-printing.

nikomatsakis · 2016-11-21T19:41:17Z

src/librustc/dep_graph/dep_node.rs

@@ -42,6 +42,9 @@ pub enum DepNode<D: Clone + Debug> {
    // Represents the HIR node with the given node-id
    Hir(D),

+    // Represents the body of a function or method
+    HirBody(D),


This comment should specify that D is the def-id of the fn/method, as opposed to the id of the body itself.

As an alternative, we could get rid of this variant and give expression bodies their own def-id. Not yet sure which approach I prefer, have to read more.

nikomatsakis · 2016-11-21T20:13:01Z

src/test/incremental/hello_world.rs

@@ -31,7 +31,7 @@ mod x {
 mod y {
    use x;

-    #[rustc_dirty(label="TypeckItemBody", cfg="rpass2")]
+    #[rustc_clean(label="TypeckItemBody", cfg="rpass2")]


nikomatsakis · 2016-11-21T20:13:30Z

src/test/incremental/source_loc_macros.rs

 fn col_same() {
    let _ = column!();
 }

-#[rustc_clean(label="Hir", cfg="rpass2")]
+#[rustc_clean(label="HirBody", cfg="rpass2")]


Seems like it'd be good to have both Hir and HirBody here?

nikomatsakis · 2016-11-21T20:14:55Z

src/librustc/middle/cstore.rs

@@ -144,6 +144,7 @@ impl<'a> InlinedItemRef<'a> {
    pub fn from_trait_item(def_id: DefId, item: &'a hir::TraitItem, _map: &hir_map::Map) -> InlinedItemRef<'a> {
        let (body, kind) = match item.node {
            hir::ConstTraitItem(ref ty, Some(ref body)) => (&**body, InlinedItemKindRef::Const(ty)),
+            hir::ConstTraitItem(_, None) => bug!("InlinedItemRef::from_trait_item called for const without body"),


Is this a pre-existing bug, or something you introduced?

The code which calls from_trait_item takes care to only call it when there's a body, but I accidentally changed that once, and it wasn't immediately clear to me what I'd done wrong, so I just added this special message in case it happens again.

nikomatsakis · 2016-11-21T20:17:52Z

src/librustc_typeck/collect.rs

-    fn visit_item(&mut self, item: &hir::Item) {
+impl<'a, 'tcx> Visitor<'tcx> for CollectItemTypesVisitor<'a, 'tcx> {
+    fn nested_visit_map(&mut self) -> Option<(&hir::map::Map<'tcx>, NestedVisitMode)> {
+        Some((&self.ccx.tcx.map, NestedVisitMode::OnlyBodies))


Hmm, this is suboptimal from the POV of the dep-graph tasks. We don't want the task which writes to the signature of a fn also reading from the fn body. Not sure what's the best way to tweak that just now; it's certainly possible.

nikomatsakis · 2016-11-21T20:19:10Z

src/test/incremental/hello_world.rs

-    #[rustc_clean(label="TypeckItemBody", cfg="rpass2")]
-    pub fn y() {
-        x::x();
+    // FIXME: This should be clean


I think the problem is the issue of collect reading from the fn body that I pointed out earlier.

flodiebold · 2016-11-21T22:00:37Z

Giving def-ids to bodies didn't really occur to me (I was thinking only definitions can have def-ids). That would probably make a few things cleaner, but I don't really have the perspective to know what else it may complicate. Also, how would the testing attributes work? Can you give attributes to expressions?

nikomatsakis · 2016-11-21T22:17:49Z

@flodiebold so I pushed two commits into your branch -- take a look. The first one fixes the TypeckCollect dependency problem in a reasonable clean way. The other one is a WIP -- basically a bunch of the "svh" tests are now much cleaner (in particular, a lot of once-dirty metadata and signatures are now clean) and I didn't get time to make them all pass.

nikomatsakis · 2016-11-21T22:18:27Z

Can you give attributes to expressions?

You can give attributes to statements, not expressions. So that is a possible complication; I guess we could look for an attribute on the first statement of a block or something.

nikomatsakis · 2016-11-21T22:19:31Z

@flodiebold all in all, the main thing I'd prefer to settle now is the visitor trait -- i.e., whether to have another method. The DepNode::HirBody thing is relatively easy to refactor later, I think.

bors · 2016-11-22T03:16:51Z

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

flodiebold · 2016-11-22T12:06:16Z

@nikomatsakis It does seem to me like having two methods and having no default for nested_visit_map would be cleaner. I'll do that.

I'll fix the remaining tests; I think they may have come in during my last rebase, after which I forgot to check them again :)

nikomatsakis · 2016-11-22T14:10:19Z

@flodiebold

It does seem to me like having two methods and having no default for nested_visit_map would be cleaner. I'll do that.

Great =)

If that change is made and tests are working, I think this PR is ready to go. We can revisit the HirBody thing afterwards. I tend to think it'd be mildly cleaner to just have one variant (Hir) but I think the diff will be non-trivial-enough we might as well handle it in a follow-up (we have to create def-ids for fn bodies, at minimum? I feel like when I thought it through I decided there would be some (minor, but still) complications to overcome.)

flodiebold · 2016-11-24T21:34:53Z

@eddyb I refactored the inlined item handling again. I didn't get rid of the FnLikeNode usage for local const fns though, just for the inlined items.

@nikomatsakis Ok, I think I've addressed everything.

flodiebold · 2016-11-24T21:38:09Z

@michaelwoerister By the way, the unary_and_binary_exprs test was actually not working correctly because cfail2 was misspelled cfails2 in the rustc_clean annotations. Maybe that could be prevented somehow?

michaelwoerister · 2016-11-25T18:06:19Z

@flodiebold Good catch. Yes, maybe we can let the rest runner do some checks to prevent this.

bors · 2016-11-28T13:04:34Z

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

eddyb · 2016-11-28T16:56:05Z

@flodiebold The commit "Merge master into separate-bodies" is worrying me - did you git pull without --rebase? In other news, I'm blocked on this PR, more or less, so let me know if I can help.

flodiebold · 2016-11-28T17:02:34Z

@eddyb I did merge because I wasn't sure whether a merge or a rebase is the preferred way to resolve conflicts. But I've actually just done a rebase onto the current master, so I could push that too.

Apart from that, I think the PR is ready and just needs to be looked at again.

eddyb · 2016-11-28T17:13:15Z

@flodiebold Rebase + force push is the way, only @bors does merges (into the auto branch).
I've also used merge locally to temporarily create a branch made from multiple PRs to test something.

flodiebold · 2016-11-28T17:15:57Z

Ok, that's what I preferred anyway. Rebased version coming up.

nikomatsakis · 2016-11-28T19:39:08Z

@flodiebold I was experimenting with an alternative version of the visitor refactoring. I've just rebased it atop your rebase, once it builds I'll push it to your branch and you can tell me what you think. It's actually sort of closer to your original approach.

nikomatsakis · 2016-11-28T19:44:43Z

done.

nikomatsakis · 2016-11-28T19:52:38Z

Pushed one more commit updating the comments a bit.

nikomatsakis · 2016-11-28T19:54:14Z

@flodiebold (if you are happy with my latest commit, then r=me on the PR)

flodiebold · 2016-11-28T20:02:02Z

Ah, that does seem nicer. Makes it easier to search for visitors which currently don't return a map, too. 👍

nikomatsakis · 2016-11-28T20:02:14Z

@bors r+

bors · 2016-11-28T20:02:15Z

📌 Commit f620072 has been approved by nikomatsakis

eddyb · 2016-11-28T21:18:09Z

@bors r- failed Travis (librustdoc changes needed after rebase)

bors · 2016-11-29T01:55:33Z

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

They don't implement FnLikeNode anymore, instead are handled differently further up in the call tree. Also, keep less information (just def ids for the args).

... and make the latter mandatory to implement.

flodiebold · 2016-11-29T14:05:40Z

@eddyb @nikomatsakis Fixed and rebased :)

michaelwoerister · 2016-11-29T14:20:15Z

@bors r=nikomatsakis

bors · 2016-11-29T14:20:17Z

📌 Commit 593b273 has been approved by nikomatsakis

bors · 2016-11-29T14:50:38Z

⌛ Testing commit 593b273 with merge f50dbd5...

@nikomatsakis

Separate function bodies from their signatures in HIR Also give them their own dep map node. I'm still unhappy with the handling of inlined items (1452edc1), but maybe you have a suggestion how to improve it. Fixes #35078. r? @nikomatsakis

nikomatsakis · 2016-11-29T14:56:47Z

@flodiebold great, thanks!

bors · 2016-11-29T18:53:45Z

@Manishearth

annotate stricter lifetimes on LateLintPass methods to allow them to forward to a Visitor this unblocks clippy (rustup blocked after #37918) clippy has lots of lints that internally call an `intravisit::Visitor`, but the current lifetimes on `LateLintPass` methods conflicted with the required lifetimes (there was no connection between the HIR elements and the `TyCtxt`) r? @Manishearth

[10/n] Split constants and functions' arguments into disjoint bodies. _This is part of a series ([prev](#38053) | [next]()) of patches designed to rework rustc into an out-of-order on-demand pipeline model for both better feature support (e.g. [MIR-based](https://github.com/solson/miri) early constant evaluation) and incremental execution of compiler passes (e.g. type-checking), with beneficial consequences to IDE support as well. If any motivation is unclear, please ask for additional PR description clarifications or code comments._ <hr> Finishes the signature-body split started in #37918, namely: * `trait` items are separated just like `impl` items were, for uniformity, closing #37712 * `static`s, `const`s (including associated ones), `enum` discriminants and array lengths get bodies * even the count in "repeat expressions", i.e. `n` in `[x; n]`, which fixes #24414 * arguments' patterns are moved to the bodies, with the types staying in `FnDecl` * `&self` now desugars to `self: &Self` instead of `self: &_` (similarly for other `self` forms) * `astconv`'s and metadata's (for rustdoc) informative uses are explicitly ignored for the purposes of the dep graph. this could be fixed in the future by hashing the exact information being extracted about the arguments as opposed to generating a dependency on *the whole body*

rust-highfive assigned nikomatsakis Nov 21, 2016

nikomatsakis requested changes Nov 21, 2016

View reviewed changes

flodiebold force-pushed the separate-bodies branch from cd9e807 to 5162288 Compare November 28, 2016 17:16

nikomatsakis approved these changes Nov 28, 2016

View reviewed changes

Add exprs map to crate, collect item blocks there

069a244

nikomatsakis and others added 10 commits November 29, 2016 13:04

WIP: update tests to pass -- not complete

dd1491c

Refactor inlined items some more

d0ae2c8

They don't implement FnLikeNode anymore, instead are handled differently further up in the call tree. Also, keep less information (just def ids for the args).

Fix remaining SVH tests

d5a501d

Address remaining review comments

725cffb

Split nested_visit_mode function off from nested_visit_map

f0ce5bb

... and make the latter mandatory to implement.

Fix SVH tests some more

b10bbde

Fix doc test collection

bf298ae

Fix rebase breakage

8575184

revamp Visitor with a single method for controlling nested visits

104125d

update comments

9457497

flodiebold force-pushed the separate-bodies branch from f620072 to bc9bae9 Compare November 29, 2016 12:07

librustdoc: Fix compilation after visitor change

593b273

flodiebold force-pushed the separate-bodies branch from bc9bae9 to 593b273 Compare November 29, 2016 12:21

bors merged commit 593b273 into rust-lang:master Nov 29, 2016

This was referenced Nov 29, 2016

in region, treat current (and future) item-likes alike #37920

Merged

Warn unused type aliases, reimplemented #38051

Merged

This was referenced Dec 4, 2016

WIP: rustup rust-lang/rust-clippy#1373

Merged

annotate stricter lifetimes on LateLintPass methods to allow them to forward to a Visitor #38191

Merged

eddyb mentioned this pull request Dec 18, 2016

[10/n] Split constants and functions' arguments into disjoint bodies. #38449

Merged

Separate function bodies from their signatures in HIR #37918

Separate function bodies from their signatures in HIR #37918

Conversation

flodiebold commented Nov 21, 2016

rust-highfive commented Nov 21, 2016

nikomatsakis left a comment • edited Loading

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

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

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

flodiebold commented Nov 21, 2016

nikomatsakis commented Nov 21, 2016

nikomatsakis commented Nov 21, 2016

nikomatsakis commented Nov 21, 2016

bors commented Nov 22, 2016

flodiebold commented Nov 22, 2016

nikomatsakis commented Nov 22, 2016

flodiebold commented Nov 24, 2016

flodiebold commented Nov 24, 2016

michaelwoerister commented Nov 25, 2016

bors commented Nov 28, 2016

eddyb commented Nov 28, 2016

flodiebold commented Nov 28, 2016

eddyb commented Nov 28, 2016

flodiebold commented Nov 28, 2016

nikomatsakis commented Nov 28, 2016

nikomatsakis commented Nov 28, 2016

nikomatsakis commented Nov 28, 2016

nikomatsakis commented Nov 28, 2016

flodiebold commented Nov 28, 2016

nikomatsakis commented Nov 28, 2016

bors commented Nov 28, 2016

eddyb commented Nov 28, 2016

bors commented Nov 29, 2016

flodiebold commented Nov 29, 2016

michaelwoerister commented Nov 29, 2016

bors commented Nov 29, 2016

bors commented Nov 29, 2016

nikomatsakis commented Nov 29, 2016

bors commented Nov 29, 2016

nikomatsakis left a comment •

edited

Loading