query for def_span by hackeryarn · Pull Request #41593 · rust-lang/rust

hackeryarn · 2017-04-28T02:50:51Z

Resolves fn def_span(&self, sess: &Session, def: DefId) -> Span; of #41417.

I had to change the query name to def_sess_span since ty::TyCtxt already has a method def_span implemented.

This also will probably have merge conflicts with #41534 but I will resolves those once it's merged and wanted to start a code review on this one now.

r? @eddyb

rust-highfive · 2017-04-28T02:50:57Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (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.

eddyb · 2017-04-28T06:18:48Z

The session is not the interesting part of tcx.sess.cstore, cstore is ("crate store").
You can replace the tcx method with this one, just need a local provider too.

bors · 2017-04-28T07:30:16Z

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

hackeryarn · 2017-04-28T18:26:18Z

@eddyb, I am not sure why this failed. I replaced the function as you suggested and included a provider. I am guessing it has to do with how I'm handling the tcx.sess.cstore span in the provider, but I could not think of a different way of getting a Span if it's not available as a local node.

eddyb · 2017-04-28T19:50:16Z


- pub describe_def: meta_data_node(DefId) -> Option<Def>
+ pub describe_def: meta_data_node(DefId) -> Option<Def>,
+ pub def_span: meta_data_node(DefId) -> Span


Wait, what is meta_data_node? First off, it doesn't need a function, it could just be MetaData.
Secondly, if you have a local crate implementation, you kind of need a node that's not MetaData.
But if @nikomatsakis doesn't think this impacts incremental this can be left as-is for now.

This was actually added as part of my last PR. I will remove it form both instances and replace it with just MetaData.

eddyb · 2017-04-28T19:52:20Z

+ match tcx.hir.span_if_local(def_id) {
+ Some(span) => span,
+ None => {
+ let node_id = tcx.sess.cstore.item_body(tcx, def_id).id().node_id;


I'm surprised you found item_body - that's really only meant to be used by const evaluation.
You should really be panicking if the DefId isn't local (i.e. you can .unwrap() the result of span_if_local instead of handling the None case) - because the line you added in src/librustc_metadata/cstore_impl.rs is what actually handles that case.

eddyb · 2017-04-28T19:56:30Z

Aaaaah, the stack overflow is because by default the span used in the "query cycle" printout comes from tcx.def_span(def_id) - you have to somehow override that for def_span.
Not sure this is even easy atm, since we do some things through Key or Value, not something implemented on the query itself - you can probably use match on the Query enum created from the query type and see if it's specifically the def_span variant.

hackeryarn · 2017-04-28T21:30:12Z

I've updated your suggestions, but I will need to spend some more time on the stack overflow error.

bors · 2017-04-28T21:41:30Z

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

hackeryarn · 2017-04-29T04:35:43Z

@eddyb I believe this addresses all your requests and the stack overflow issue.

eddyb · 2017-04-29T08:17:38Z

 // FIXME(eddyb) Get more valid Span's on queries.
- if span == DUMMY_SP {
- span = key.default_span(tcx);
+ if span == DUMMY_SP && stringify!($name) != "def_span" {


I guess this is not too bad of a workaround - could you leave a comment above as to why this is done?

eddyb · 2017-04-29T08:18:16Z


- [] describe_def: meta_data_node(DefId) -> Option<Def>
+
+ [] describe_def: MetaData(DefId) -> Option<Def>,


Unnecessary extra empty line.

eddyb · 2017-04-29T08:21:51Z

You're getting failures because you're using MetaData which is special - may I suggest adding a DescribeDef and a DefSpan variants to DepNode?

hackeryarn · 2017-04-29T14:57:43Z

@eddyb this should resolve everything. Thanks for the DepNode suggestion I like the explicitness of that approach 😄

eddyb · 2017-04-29T17:11:41Z

 // not a hotspot.
 ProjectionCache { def_ids: Vec<D> },
+
+ // Depnodes for MetaData


Heh, you can leave the comment off - the reason we need these is becauseMetaData can't be used, as it's reserved for cross-crate queries, e.g. ItemSignature(def_id) will eventually depend on either Hir(def_id) iff def_id.is_local() or MetaData(def_id) otherwise.

Ah that makes sense the title of the issue was deceiving. I assumed all of these were just about crate metadata.

eddyb · 2017-04-29T17:12:48Z

+}
+
+fn def_span(def_id: DefId) -> DepNode<DefId> {
+ DepNode::DefSpan(def_id)


These aren't needed, because they're just a DepNode variant - the explicit functions are in cases where you can't just use DepNode::VariantName(def_id).

eddyb · 2017-04-29T18:14:39Z

@bors r+

bors · 2017-04-29T18:14:39Z

📌 Commit d3b7af0 has been approved by eddyb

bors · 2017-04-30T02:48:17Z

⌛ Testing commit d3b7af0 with merge ba5b911...

@eddyb

query for def_span Resolves `fn def_span(&self, sess: &Session, def: DefId) -> Span;` of #41417. I had to change the query name to `def_sess_span` since `ty::TyCtxt` already has a method `def_span` implemented. This also will probably have merge conflicts with #41534 but I will resolves those once it's merged and wanted to start a code review on this one now. r? @eddyb

bors · 2017-04-30T05:25:42Z

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing ba5b911 to master...

rust-highfive assigned eddyb Apr 28, 2017

shepmaster added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 28, 2017

query for def_span
932d251

hackeryarn force-pushed the def_span branch from 7d5621a to 932d251Compare April 28, 2017 16:00

eddyb reviewed Apr 28, 2017
View reviewed changes

review updateds
93ac5df

hackeryarn added 2 commits April 28, 2017 22:42

found the stack overflow culprit
ba90718

Merge branch 'master' into def_span
2f73b17

eddyb reviewed Apr 29, 2017
View reviewed changes

added dep nodes and comment
ad1959e

chaned dep nodes impl
00d02cf

eddyb reviewed Apr 29, 2017
View reviewed changes

removed custom functions and comment
d3b7af0

frewsxcv mentioned this pull request Apr 29, 2017
Rollup of 8 pull requests #41641
Closed

bors merged commit d3b7af0 into rust-lang:masterApr 30, 2017

This was referenced Apr 30, 2017
rework the queries for the MIR pipeline #41625
Merged
Queryify crate metadata #41611
Merged

hackeryarn deleted the def_span branch April 30, 2017 13:42

hackeryarn mentioned this pull request May 2, 2017
Queries for Crate Metadata #41653
Merged

nikomatsakis mentioned this pull request May 5, 2017
incr.comp.: Crate Metadata not corresponding to a DefId is untracked. #41417
Closed


		[] describe_def: meta_data_node(DefId) -> Option<Def>

		[] describe_def: MetaData(DefId) -> Option<Def>,

Uh oh!

Uh oh!

Conversation

hackeryarn commented Apr 28, 2017

Uh oh!

rust-highfive commented Apr 28, 2017

Uh oh!

eddyb commented Apr 28, 2017

Uh oh!

bors commented Apr 28, 2017

Uh oh!

hackeryarn commented Apr 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddyb commented Apr 28, 2017

Uh oh!

hackeryarn commented Apr 28, 2017

Uh oh!

bors commented Apr 28, 2017

Uh oh!

hackeryarn commented Apr 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddyb commented Apr 29, 2017

Uh oh!

hackeryarn commented Apr 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eddyb commented Apr 29, 2017

Uh oh!

bors commented Apr 29, 2017

Uh oh!

bors commented Apr 30, 2017

Uh oh!

bors commented Apr 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants