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

eddyb · 2016-12-18T04:15:27Z

This is part of a series (prev | 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 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.

Finishes the signature-body split started in #37918, namely:

trait items are separated just like impl items were, for uniformity, closing Separate trait items from trait #37712
statics, consts (including associated ones), enum discriminants and array lengths get bodies
- even the count in "repeat expressions", i.e. n in [x; n], which fixes Improve "no type for local variable" error when in check_const_with_ty #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 · 2016-12-18T04:15:33Z

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

eddyb · 2016-12-18T04:16:08Z

r? @nikomatsakis

michaelwoerister · 2016-12-19T19:58:16Z

astconv's and metadata's (for rustdoc) informative uses are explicitly ignored for the purposes of the dep graph.

That sounds a bit troubling.

eddyb · 2016-12-19T20:09:41Z

@michaelwoerister AFAIK astconv's use, which is only in case of error, couldn't be observed. But that one is fixable if need be, by representing the elision information in terms of the BodyId, not the data extracted from it.

The rustdoc use is more problematic. I'm not sure there is a scenario in which it could go wrong, but long-term we should really move away from the current cross-crate docs model which is becoming unsustainable. I'm leaning towards the save-analysis solution, more so for rustdoc than IDEs even.

michaelwoerister · 2016-12-19T20:12:21Z

Are you sure that compilation really aborts in case find_implied_output_region() returns Err? If it does, it should not make a difference anyway because we don't save the dep-graph in that case.

eddyb · 2016-12-19T20:14:53Z

@michaelwoerister The information (argument names) is only used in case of (elision) error, later on (and printed verbatim, not used in any decision).

michaelwoerister · 2016-12-19T20:23:48Z

I still think this is problematic. What you say might be true for now, but lots of people are modifying this code without global understanding. Can this be refactored to just access that information in case an actual error occurs? (Alternatively, for cases like this, we could have some opaque ErrorReportingInfo type that implements Display and makes it explicit this must only be used for error reporting).

Regarding metadata: Isn't this kind of a premature optimization? Metadata is full of overconservative edges anyway. I think it would better to review this together with the rest when the time comes. Or does it break existing tests?

eddyb · 2016-12-19T20:31:13Z

@michaelwoerister Alright I'll try to see what I can do to astconv. As for the metadata thing, it breaks all the tests that modify the body, making the body separation sort of pointless metadata-wise.
I would prefer to somehow hash the actual data being encoded (the argument names), as it has a much much smaller footprint and it would cause less of a hassle overall (@nikomatsakis said he wanted this).

michaelwoerister · 2016-12-19T20:44:11Z

Alright I'll try to see what I can do to astconv.

Cool :) Thanks!

I would prefer to somehow hash the actual data being encoded (the argument names), as it has a much much smaller footprint and it would cause less of a hassle overall.

I would prefer that too. I suspect we'll move away from monolithic Entrys in metadata and put independent things into separate tables. Or have individual DepNodes for Entry fields. In this case, where the data is just a list of plain strings, it would be easy to just hash that. However, it would require that we start refactoring metadata dep-tracking already, which is a bit much for this PR.

As long as this is just used in rustdoc, we should be fine. How about just adding an assertion to CrateMetadata::get_fn_arg_names() that incremental compilation is turned off?

eddyb · 2016-12-19T20:51:21Z

How about just adding an assertion to CrateMetadata::get_fn_arg_names() that incremental compilation is turned off?

Oh that is brilliant, thanks for the idea! I assume it would replace the usual read(Metadata(def_id))?

michaelwoerister · 2016-12-19T20:56:53Z

Oh that is brilliant, thanks for the idea! I assume it would replace the usual read(Metadata(def_id))?

Sounds reasonable.

nikomatsakis

Waiting on the commit to come. Looks good; we chatted about some of my comments on IRC. The thing about omitting the repeat expressions etc from borrowck is my biggest concern, though I'm not sure the best way to test it right now. Still seems like we should do it right.

nikomatsakis · 2016-12-19T21:27:26Z

src/librustc/hir/map/def_collector.rs


 use syntax::ast::*;
 use syntax::ext::hygiene::Mark;
 use syntax::visit;
 use syntax::symbol::{Symbol, keywords};

-/// Creates def ids for nodes in the HIR.
+/// Creates def ids for nodes in the AST.


Did you intentionally change this from HIR to AST?

Yeah, since it never touches the HIR anymore.

nikomatsakis · 2016-12-19T23:00:00Z

src/librustc_borrowck/borrowck/mod.rs

@@ -68,7 +68,7 @@ impl<'a, 'tcx> Visitor<'tcx> for BorrowckCtxt<'a, 'tcx> {
    }

    fn visit_fn(&mut self, fk: FnKind<'tcx>, fd: &'tcx hir::FnDecl,
-                b: hir::ExprId, s: Span, id: ast::NodeId) {
+                b: hir::BodyId, s: Span, id: ast::NodeId) {


so I can't find a way to make an error out of this (yet), but it seems like we are failing to borrowck embedded bodies -- e.g., in an expression like [22; X], we don't borrowck the expression X, right? The main reason I can't make an error here is that the set of constant expressions we accept in that context is pretty limited, but it seems like it wouldn't necessarily stay that way...

I did run across this, which seems like a bug, but a pre-existing one (this code compiles on nightly today...but it shouldn't...):

#![feature(const_fn)] struct Foo(usize); const fn get(x: Foo) -> usize { x.0 } const X: Foo = Foo(22); static Y: usize = get(*&X); fn main() { }

Why shouldn't it? That's equivalent to ({*&Foo(22)}).0 which is perfectly fine AFAIK.
The problem would be if X where static (and only until we get miri).

It should not because Foo is not Copy, and we are moving it from a &Foo -- e.g., this does not compile: https://is.gd/zU2BVo

Filed #38520

nikomatsakis · 2016-12-19T23:03:25Z

src/librustc_metadata/encoder.rs

@@ -839,7 +839,9 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
                _ => None,
            },
            mir: match item.node {
-                hir::ItemStatic(..) |
+                hir::ItemStatic(..) if self.tcx.sess.opts.debugging_opts.always_encode_mir => {


was this just a pre-existing bug?

Indeed it was, there were a few things in the PR introducing this that are less than ideal. They skipped review because they weren't in the original PR state but rather in "fixes for tests".

nikomatsakis · 2016-12-19T23:12:34Z

src/test/compile-fail/associated-const-type-parameter-arrays.rs

@@ -26,7 +26,9 @@ impl Foo for Def {

 pub fn test<A: Foo, B: Foo>() {
    let _array: [u32; <A as Foo>::Y];
-    //~^ ERROR the trait bound `A: Foo` is not satisfied
+    //~^ ERROR cannot use an outer type parameter in this context [E0402]


Is there a backwards compatibility concern here? Probably not, since this sort of doesn't work now -- but I feel like it should work someday, so it seems odd to be considering A as an "outer" type parameter.

Yeah, I didn't really make anything worse, just better encoded the existing semantics.
We'll be able to clean this up in the future, when everything is body-based.

nikomatsakis · 2016-12-19T23:13:26Z

src/test/incremental/hashes/consts.rs

@@ -66,8 +66,10 @@ const CONST_CHANGE_TYPE_2: Option<u64> = None;
 const CONST_CHANGE_VALUE_1: i16 = 1;

 #[cfg(not(cfail1))]
-#[rustc_dirty(label="Hir", cfg="cfail2")]
+#[rustc_clean(label="Hir", cfg="cfail2")]


nikomatsakis · 2016-12-19T23:28:49Z

src/test/incremental/hashes/trait_defs.rs

@@ -303,7 +303,7 @@ trait TraitChangeModeSelfOwnToMut: Sized {
 #[cfg(not(cfail1))]
 #[rustc_clean(label="Hir", cfg="cfail2")]
 #[rustc_clean(label="Hir", cfg="cfail3")]
-#[rustc_metadata_dirty(cfg="cfail2")]
+#[rustc_metadata_clean(cfg="cfail2")]


Can we make an ICH test targeting exactly this change? Seems easily do-able. In particular, if you have something like fn foo(x: u32) { } and you change it to fn foo(y: u32) { }, the hash of the trait should not change (just the body). Right?

That said, if you don't provide a default, based on my read of the code above, then the fn names would still be included in the hash... which doesn't seem exactly right. We could I guess just ignore them manually in the hasher code.

That said, if you don't provide a default, based on my read of the code above, then the fn names would still be included in the hash... which doesn't seem exactly right.

To me this seems like the correct way to handle this. If you remove a method decl from a trait, don't you have to re-run many computations depending on that trait?

I think @nikomatsakis meant the fn argument names.

Oh, that makes sense then.

bors · 2016-12-21T02:51:08Z

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

nikomatsakis · 2016-12-21T20:42:56Z

This all looks good to me I think except that I'd still like to run borrowck expressions like the N in [T; N] etc. It annoys me that I cannot find a way to write a test for this though, because constant expressions are too restrictive. Still it seems like we should try to get it right. =)

bors · 2016-12-26T16:16:19Z

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

bors · 2016-12-28T04:13:27Z

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

…tants.

eddyb · 2016-12-28T10:07:44Z

@bors r=nikomatsakis

bors · 2016-12-28T10:07:44Z

📌 Commit ee0ea95 has been approved by nikomatsakis

bors · 2016-12-28T12:24:48Z

⌛ Testing commit ee0ea95 with merge dd07f36...

bors · 2016-12-28T12:56:23Z

💔 Test failed - status-travis

eddyb · 2016-12-28T16:30:14Z

@bors retry

bors · 2016-12-28T20:19:30Z

⌛ Testing commit ee0ea95 with merge 4ecc85b...

[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*

bors · 2016-12-28T23:02:49Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 4ecc85b to master...

[11/n] Separate ty::Tables into one per each body. _This is part of a series ([prev](#38449) | [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> In order to track the results of type-checking and inference for incremental recompilation, they must be stored separately for each function or constant value, instead of lumped together. These side-`Tables` also have to be tracked by various passes, as they visit through bodies (all of which have `Tables`, even if closures share the ones from their parent functions). This is usually done by switching a `tables` field in an override of `visit_nested_body` before recursing through `visit_body`, to the relevant one and then restoring it - however, in many cases the nesting is unnecessary and creating the visitor for each body in the crate and then visiting that body, would be a much cleaner solution. To simplify handling of inlined HIR & its side-tables, their `NodeId` remapping and entries HIR map were fully stripped out, which means that `NodeId`s from inlined HIR must not be used where a local `NodeId` is expected. It might be possible to make the nodes (`Expr`, `Block`, `Pat`, etc.) that only show up within a `Body` have IDs that are scoped to that `Body`, which would also allow `Tables` to use `Vec`s. That last part also fixes #38790 which was accidentally introduced in a previous refactor.

rust-highfive assigned arielb1 Dec 18, 2016

eddyb mentioned this pull request Dec 18, 2016

[9/n] rustc: move type information out of AdtDef and TraitDef. #38053

Merged

rust-highfive assigned nikomatsakis and unassigned arielb1 Dec 18, 2016

eddyb requested a review from nikomatsakis December 18, 2016 04:16

eddyb force-pushed the lazy-10 branch from 4ac6cce to 707a022 Compare December 18, 2016 07:24

nikomatsakis reviewed Dec 19, 2016

View reviewed changes

eddyb mentioned this pull request Dec 20, 2016

Sort out body-less function argument names wrt incremental recompilation. #38501

Closed

eddyb force-pushed the lazy-10 branch from 707a022 to f5b622f Compare December 20, 2016 21:15

eddyb force-pushed the lazy-10 branch 2 times, most recently from b7286b9 to 53e745b Compare December 21, 2016 10:34

eddyb force-pushed the lazy-10 branch from 53e745b to cb87804 Compare December 22, 2016 06:25

eddyb force-pushed the lazy-10 branch 4 times, most recently from 31bd067 to 353a194 Compare December 27, 2016 13:02

eddyb added 7 commits December 28, 2016 11:21

hir: lower ImplicitSelf to resolved Self TyQPath's.

6ebb6fd

rustc: separate TraitItem from their parent Item, just like ImplItem.

8649282

rustc: separate bodies for static/(associated)const and embedded cons…

e64f64a

…tants.

rustc: move function arguments into hir::Body.

f89856b

rustc: simplify constant cross-crate loading and rustc_passes::consts.

f64e73b

rustc: always print nested nodes where a HIR map is available.

4aae835

rustdoc: pretty-print nested bodies in inlined constants.

ee0ea95

eddyb force-pushed the lazy-10 branch from 353a194 to ee0ea95 Compare December 28, 2016 09:29

bors merged commit ee0ea95 into rust-lang:master Dec 28, 2016

This was referenced Dec 28, 2016

i128 and u128 support #38482

Merged

exclusive range patterns #35712

Merged

run rustfmt on astconv.rs file #38567

Closed

Show span for trait that doesn't implement Copy #37493

Closed

Suggest solutions for fn foo(&foo: Foo) #38605

Merged

alexcrichton mentioned this pull request Dec 30, 2016

Illegal instruction in hamt-rs in Rust 1.15 #38683

Closed

eddyb deleted the lazy-10 branch January 2, 2017 16:15

This was referenced Jan 4, 2017

[11/n] Separate ty::Tables into one per each body. #38813

Merged

Separate trait items from trait #37712

Closed

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

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

Conversation

eddyb commented Dec 18, 2016 • edited Loading

rust-highfive commented Dec 18, 2016

eddyb commented Dec 18, 2016

michaelwoerister commented Dec 19, 2016

eddyb commented Dec 19, 2016

michaelwoerister commented Dec 19, 2016

eddyb commented Dec 19, 2016

michaelwoerister commented Dec 19, 2016 • edited Loading

eddyb commented Dec 19, 2016 • edited Loading

michaelwoerister commented Dec 19, 2016

eddyb commented Dec 19, 2016

michaelwoerister commented Dec 19, 2016

nikomatsakis 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

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

bors commented Dec 21, 2016

nikomatsakis commented Dec 21, 2016

bors commented Dec 26, 2016

bors commented Dec 28, 2016

eddyb commented Dec 28, 2016

bors commented Dec 28, 2016

bors commented Dec 28, 2016

bors commented Dec 28, 2016

eddyb commented Dec 28, 2016

bors commented Dec 28, 2016

bors commented Dec 28, 2016

eddyb commented Dec 18, 2016 •

edited

Loading

michaelwoerister commented Dec 19, 2016 •

edited

Loading

eddyb commented Dec 19, 2016 •

edited

Loading