Track more crate metadata #42598

cramertj · 2017-06-12T08:00:05Z

bors · 2017-06-12T14:00:56Z

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

eddyb · 2017-06-12T14:36:30Z

src/librustc/ty/maps.rs

+    [] is_allocator: MetaDataByCrateNum(CrateNum) -> bool,
+    [] is_panic_runtime: MetaDataByCrateNum(CrateNum) -> bool,
+
+    [] extern_crate: MetaDataByCrateNum(CrateNum) -> Rc<Option<ExternCrate>>,


I think I'd rather all of these use DefId with DEF_INDEX_CRATE. What do you think, @nikomatsakis?

I would think that CrateNum would be preferred -- it seems clearer -- why do you think DefId, @eddyb?

One note: under the new dep-node system, a DepNode(CrateNum) is perfectly fine, and we should be able to handle it well (in the past, the "retracing" only worked for DefId, so we avoided using CrateNum in a DepNode.)

Some of these are literally the same as querying the attributes of the crate root of some extern crate (they should also probably be replaced with an actual attribute query but that's another question), so to me they make more sense as a property of that DefId instead of that of the CrateNum (the latter would make sense for e.g. all impl items in a crate).

OK. I don't see it that way -- that is, I see putting the attributes on the crate root as a way of specifying properties that apply to the crate as a whole, but I have no strong opinion here. I'd be happy either way.

eddyb · 2017-06-12T14:37:54Z

src/librustc_mir/transform/qualify_consts.rs

@@ -117,7 +117,7 @@ pub fn is_const_fn(tcx: TyCtxt, def_id: DefId) -> bool {
            false
        }
    } else {
-        tcx.sess.cstore.is_const_fn(def_id)
+        tcx.is_const_fn(def_id)


You could replace all of these "check locally or externally if a fn is const" helpers if you had is_const_fn also for the local crate.

👍, that seems like it would be nicer

nikomatsakis · 2017-06-13T15:08:26Z

src/librustc/dep_graph/dep_node.rs

@@ -93,6 +94,7 @@ pub enum DepNode<D: Clone + Debug> {
    ItemSignature(D),
    ItemVarianceConstraints(D),
    ItemVariances(D),
+    IsConstFn(D),


note that this whole mechanism recently changed, so you'll have to rebase it-- in the new version, you would just write DefId here

nikomatsakis · 2017-06-13T15:09:06Z

src/librustc/middle/dependency_format.rs

        fmts.insert(ty, linkage);
    }
    sess.abort_if_errors();
 }

-fn calculate_type(sess: &session::Session,
+fn calculate_type<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
                  ty: config::CrateType) -> DependencyList {


Nit: indentation

nikomatsakis · 2017-06-13T15:09:29Z

src/librustc/middle/dependency_format.rs

        fmts.insert(ty, linkage);
    }
    sess.abort_if_errors();
 }

-fn calculate_type(sess: &session::Session,
+fn calculate_type<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,


Random aside: I feel like a GlobalTyCtxt<'a, 'tcx> alias might be nice.

At that point, the Ty part is maybe a bit unnecessary and we should move towards a generalized idea of a "global context" (aka god object).

nikomatsakis · 2017-06-13T15:17:43Z

This generally looks good to me. Needs a rebase, and we have to decide on CrateNum vs DefId. I don't have a strong opinion there. It'd be nice to also do the refactoring that @eddyb suggested.

cramertj · 2017-06-14T16:08:56Z

src/librustc_metadata/cstore_impl.rs

+
+            $(fn $cnum_fn_name<'a, $lt:$lt>($tcx: TyCtxt<'a, $lt, $lt>, $cnum: CrateNum)
+                        -> <ty::queries::$cnum_fn_name<$lt> as
+                            DepTrackingMapConfig>::Value {


I didn't see a way to explicitly generate a dep_node from a CrateNum under the new system, so this section is missing a call to $tcx.dep_graph.read(dep_node).

@nikomatsakis @eddyb Did you decide whether or not this should be a DefId instead?

It would just be easier to use DefId IMO...

I think we settled on DefId.

@michaelwoerister can probably tell you how to make a dep-node from a CrateNum. I'll try to figure it out later but don't have time for digging right now.

@michaelwoerister can probably tell you how to make a dep-node from a CrateNum. I'll try to figure it out later but don't have time for digging right now.

If we're just going to use DefIds for all of them, there's no need. Thanks, though!

cramertj · 2017-06-15T16:09:35Z

Currently triggering this assertion after switching to DefId.

nikomatsakis · 2017-06-15T16:43:08Z

Oh? Let me check out the commit...

nikomatsakis · 2017-06-15T19:52:25Z

oh, I See what's happening. Yeah, that code assumes that there are some dep-nodes classified as base inputs, and that these have no predecessors in the depdendency graph. But when you use MetaData(DefId) as the query dep-node, it will have inputs. You should probably make some other dep node like MetaDataQuery(DefId).

eddyb · 2017-06-15T19:57:14Z

I think last time we either opted for more specific query DepNodes or added some catchall.
Definitely do not use MetaData as a query DepNode (I don't think I've seen the current state).

cramertj · 2017-06-15T19:58:27Z

Gotcha. I'll split them apart into separate DepNodes.

nikomatsakis · 2017-06-16T18:49:09Z

@bors r+

bors · 2017-06-16T18:49:11Z

📌 Commit e6dd869 has been approved by nikomatsakis

bors · 2017-06-16T21:42:24Z

⌛ Testing commit e6dd869 with merge 3cb8034...

@nikomatsakis

Track more crate metadata Part of #41417 r? @nikomatsakis

bors · 2017-06-17T00:05:45Z

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

eddyb · 2017-06-23T11:53:44Z

src/librustc_metadata/cstore_impl.rs

+    extern_crate => { Rc::new(cdata.extern_crate.get()) }
+}
+
+pub fn provide_local<'tcx>(providers: &mut Providers<'tcx>) {


Ahhh I missed this. No, this has no business being here, it should be in rustc or rustc_typeck.

Ah sorry-- I was just moving all of the former crate metadata functions into this spot.

Well this one is for the local crate, which rustc_metadata is not about. Only noticed it because I wanted to change it (didn't have to in the end) and I couldn't find it 😆.

which rustc_metadata is not about

Why did it used to be a method on CStore?

That's only the cross-crate half of it.

rust-highfive assigned nikomatsakis Jun 12, 2017

cramertj force-pushed the track-more-metadata branch from fa4c3bc to c8c90e3 Compare June 12, 2017 08:15

carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 12, 2017

eddyb reviewed Jun 12, 2017

View reviewed changes

nikomatsakis reviewed Jun 13, 2017

View reviewed changes

arielb1 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 13, 2017

On-demand is_const_fn

9f71053

cramertj force-pushed the track-more-metadata branch 2 times, most recently from 39ae2a3 to c53193e Compare June 14, 2017 06:14

cramertj added 4 commits June 14, 2017 00:12

Add CrateNum-taking ability to the provide macro

532a08b

on-demand dylib dependency formats

328c6c8

On-demandify is_allocator and is_panic_runtime

b0f05d4

On-demandify extern_crate

4835698

cramertj force-pushed the track-more-metadata branch from c53193e to 4835698 Compare June 14, 2017 07:13

cramertj commented Jun 14, 2017

View reviewed changes

Switch CrateNum queries to DefId

c98ca95

Divide up metadata into separate DepNodes

e6dd869

nikomatsakis approved these changes Jun 16, 2017

View reviewed changes

bors added a commit that referenced this pull request Jun 16, 2017

Auto merge of #42598 - cramertj:track-more-metadata, r=nikomatsakis

3cb8034

Track more crate metadata Part of #41417 r? @nikomatsakis

bors merged commit e6dd869 into rust-lang:master Jun 17, 2017

cramertj deleted the track-more-metadata branch June 17, 2017 00:07

eddyb reviewed Jun 23, 2017

View reviewed changes

kennytm mentioned this pull request Aug 16, 2017

incr.comp.: Crate Metadata not corresponding to a DefId is untracked. #41417

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track more crate metadata #42598

Track more crate metadata #42598

cramertj commented Jun 12, 2017

bors commented Jun 12, 2017

eddyb Jun 12, 2017

nikomatsakis Jun 13, 2017

eddyb Jun 13, 2017

nikomatsakis Jun 13, 2017

eddyb Jun 12, 2017

nikomatsakis Jun 13, 2017

nikomatsakis Jun 13, 2017

nikomatsakis Jun 13, 2017

nikomatsakis Jun 13, 2017

eddyb Jun 13, 2017

nikomatsakis commented Jun 13, 2017

cramertj Jun 14, 2017 •

edited

Loading

eddyb Jun 14, 2017

nikomatsakis Jun 14, 2017

cramertj Jun 14, 2017

cramertj commented Jun 15, 2017

nikomatsakis commented Jun 15, 2017

nikomatsakis commented Jun 15, 2017

eddyb commented Jun 15, 2017

cramertj commented Jun 15, 2017

nikomatsakis commented Jun 16, 2017

bors commented Jun 16, 2017

bors commented Jun 16, 2017

bors commented Jun 17, 2017

eddyb Jun 23, 2017

cramertj Jun 23, 2017

eddyb Jun 23, 2017

cramertj Jun 23, 2017

eddyb Jun 23, 2017

Track more crate metadata #42598

Track more crate metadata #42598

Conversation

cramertj commented Jun 12, 2017

bors commented Jun 12, 2017

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

nikomatsakis commented Jun 13, 2017

cramertj Jun 14, 2017 • 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

cramertj commented Jun 15, 2017

nikomatsakis commented Jun 15, 2017

nikomatsakis commented Jun 15, 2017

eddyb commented Jun 15, 2017

cramertj commented Jun 15, 2017

nikomatsakis commented Jun 16, 2017

bors commented Jun 16, 2017

bors commented Jun 16, 2017

bors commented Jun 17, 2017

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

cramertj Jun 14, 2017 •

edited

Loading