Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate function bodies from their signatures in HIR #37918

Merged
merged 37 commits into from
Nov 29, 2016

Conversation

flodiebold
Copy link
Member

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

@rust-highfive
Copy link
Collaborator

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.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. =)

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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Plus then we can avoid the NestedVisitMode tuple)

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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

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()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't need to implement FnLike though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

#[rustc_dirty(label="TypeckItemBody", cfg="rpass2")]
#[rustc_clean(label="TypeckItemBody", cfg="rpass2")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice =)

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

#[rustc_clean(label="Hir", cfg="rpass2")]
#[rustc_clean(label="HirBody", cfg="rpass2")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@flodiebold
Copy link
Member Author

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor

bors commented Nov 22, 2016

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

@flodiebold
Copy link
Member Author

@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
Copy link
Contributor

@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
Copy link
Member Author

@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
Copy link
Member Author

@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
Copy link
Member

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

@bors
Copy link
Contributor

bors commented Nov 28, 2016

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

@eddyb
Copy link
Member

eddyb commented Nov 28, 2016

@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
Copy link
Member Author

@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
Copy link
Member

eddyb commented Nov 28, 2016

@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
Copy link
Member Author

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

@nikomatsakis
Copy link
Contributor

@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
Copy link
Contributor

done.

@nikomatsakis
Copy link
Contributor

Pushed one more commit updating the comments a bit.

@nikomatsakis
Copy link
Contributor

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

@flodiebold
Copy link
Member Author

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

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Nov 28, 2016

📌 Commit f620072 has been approved by nikomatsakis

@eddyb
Copy link
Member

eddyb commented Nov 28, 2016

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

@bors
Copy link
Contributor

bors commented Nov 29, 2016

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

@flodiebold
Copy link
Member Author

@eddyb @nikomatsakis Fixed and rebased :)

@michaelwoerister
Copy link
Member

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Nov 29, 2016

📌 Commit 593b273 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Nov 29, 2016

⌛ Testing commit 593b273 with merge f50dbd5...

bors added a commit that referenced this pull request Nov 29, 2016
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
Copy link
Contributor

@flodiebold great, thanks!

@bors bors merged commit 593b273 into rust-lang:master Nov 29, 2016
bors added a commit that referenced this pull request Dec 7, 2016
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
bors added a commit that referenced this pull request Dec 28, 2016
[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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants