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

make *most* maps private #41368

Merged
merged 6 commits into from
Apr 28, 2017
Merged

Conversation

nikomatsakis
Copy link
Contributor

Currently we access the DepTrackingMap fields directly rather than using the query accessors. This seems bad. This branch removes several such uses, but not all, and extends the macro so that queries can hide their maps (so we can prevent regressions). The extension to the macro is kind of ugly :/ but couldn't find a simple way to do it otherwise (I guess I could use a nested macro...). Anyway I figure it's only temporary.

r? @eddyb

span: Span,
cycle: RefMut<'a, [(Span, Query<'tcx>)]>,
cycle: Vec<(Span, Query<'tcx>)>,
Copy link
Member

Choose a reason for hiding this comment

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

Why? This is going to clone all over the place now in early coherence and whatnot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure about this change. Coherence generates (and recovers from) cycles regularly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for it was that, when we call report_cycle, we hold the borrow-mut lock, which then messes up my attempts to use try_get from within the pretty-printing code (it fails to acquire the RefCell lock).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Maybe we can downgrade to a read-only borrow or something...?)

Copy link
Member

Choose a reason for hiding this comment

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

We don't have downgrading, but queries need mutable anyway so you wouldn't get anything.
Why do you want to query from the printing code? You could clone the stack and drop the RefMut when reporting a cycle error if you want, that's fine (since reporting itself costs anyway).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do you want to query from the printing code?

I don't necessarily want that. But the printing code currently pokes at the query's internal maps to find out if trait_ref and ty are filled out for particular def-ids (or something like that). I guess I could add an accessor for reading the current state of the query that you are really, really not supposed to use (because it lets you observe things you shouldn't be able to observe), just for use in printing things out -- but really it seems like it might make sense for printing things out to run bits of type-check that may not have run, if it is needed to pretty-print better (i.e., if we were in a totally on-demand scenario, and some error arose, it seems plausible it might need to pretty print something that hasn't yet been "collected"--- well, maybe that would never happen in practice).

Copy link
Contributor Author

@nikomatsakis nikomatsakis Apr 18, 2017

Choose a reason for hiding this comment

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

I guess if I added such a method I could use it for typeck_tables too (in save analysis).

Copy link
Member

Choose a reason for hiding this comment

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

Ohhh, you were talking about item_path_str, now I know exactly what you're talking about.
Yeah just do the stack.to_vec(); drop(stack) thing when reporting.

Copy link
Member

Choose a reason for hiding this comment

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

Still unaddressed.

.expect("not an associated item")
}

pub fn opt_associated_item(self, def_id: DefId) -> Option<AssociatedItem> {
Copy link
Member

Choose a reason for hiding this comment

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

Can't we avoid introducing queries like these? i.e. actually check what kind of DefId you have before calling associated_item.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, probably, is there a non-tedious way to do this that works across crates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The method would probably stay the same, I suppose, but it would be wrapped in code like "check type of def_id then call associated_item()".

Copy link
Member

Choose a reason for hiding this comment

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

You can check the HIR map (TraitItem and ImplItem) for local and describe_def for external.

}
None => f(self),
}
let tables = ty::queries::typeck_tables::get(self.tcx, DUMMY_SP, item_def_id);
Copy link
Member

Choose a reason for hiding this comment

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

This is actually @nrc's attempt at error recovery, heh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering about that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think this will work?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, since it's on-demand... let me find where this was added maybe @nrc provided an example.

Copy link
Member

Choose a reason for hiding this comment

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

lolsob there's no example in #39285

Copy link
Member

Choose a reason for hiding this comment

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

Now that I think more about it... it will probably work anyway.

@bors
Copy link
Contributor

bors commented Apr 18, 2017

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

@aidanhs aidanhs added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 19, 2017
@nikomatsakis nikomatsakis force-pushed the incr-comp-dep-tracking-map branch 2 times, most recently from 0f3c43e to 854a5dd Compare April 25, 2017 16:22
@nikomatsakis
Copy link
Contributor Author

@eddyb I think I addressed all of your requests; I also rebased atop your branch.

@nikomatsakis nikomatsakis force-pushed the incr-comp-dep-tracking-map branch 2 times, most recently from e2d55be to 41ad2d2 Compare April 25, 2017 16:29
@eddyb
Copy link
Member

eddyb commented Apr 25, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Apr 25, 2017

📌 Commit 41ad2d2 has been approved by eddyb

@nikomatsakis
Copy link
Contributor Author

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Apr 25, 2017

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Apr 25, 2017

📌 Commit 41ad2d2 has been approved by eddyb

@bors
Copy link
Contributor

bors commented Apr 26, 2017

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

@nikomatsakis
Copy link
Contributor Author

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Apr 26, 2017

📌 Commit 054642e has been approved by eddyb

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 27, 2017
…-map, r=eddyb

make *most* maps private

Currently we access the `DepTrackingMap` fields directly rather than using the query accessors. This seems bad. This branch removes several such uses, but not all, and extends the macro so that queries can hide their maps (so we can prevent regressions). The extension to the macro is kind of ugly :/ but couldn't find a simple way to do it otherwise (I guess I could use a nested macro...). Anyway I figure it's only temporary.

r? @eddyb
@bors
Copy link
Contributor

bors commented Apr 27, 2017

⌛ Testing commit 054642e with merge 569d203...

@bors
Copy link
Contributor

bors commented Apr 27, 2017

💔 Test failed - status-appveyor

@aidanhs
Copy link
Member

aidanhs commented Apr 27, 2017

@bors retry

Appveyor network issues https://appveyor.statuspage.io/incidents/06gzq846jl9x

@bors
Copy link
Contributor

bors commented Apr 27, 2017

⌛ Testing commit 054642e with merge 86f699c...

@bors
Copy link
Contributor

bors commented Apr 27, 2017

💔 Test failed - status-travis

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Apr 28, 2017
…-map, r=eddyb

make *most* maps private

Currently we access the `DepTrackingMap` fields directly rather than using the query accessors. This seems bad. This branch removes several such uses, but not all, and extends the macro so that queries can hide their maps (so we can prevent regressions). The extension to the macro is kind of ugly :/ but couldn't find a simple way to do it otherwise (I guess I could use a nested macro...). Anyway I figure it's only temporary.

r? @eddyb
@frewsxcv
Copy link
Member

This hit the same failure on dist-aarch64-linux that @arielb1 mentioned earlier

@bors r-

https://travis-ci.org/rust-lang/rust/jobs/226667988

@bors
Copy link
Contributor

bors commented Apr 28, 2017

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

Didn't get around to removing all public access.
This requires copying out the cycle error to avoid a cyclic borrow.  Is
this a problem? Are there paths where we expect cycles to arise and not
result in errors? (In such cases, we could add a faster way to test for
cycle.)
@nikomatsakis nikomatsakis force-pushed the incr-comp-dep-tracking-map branch 3 times, most recently from 949f214 to cb618e3 Compare April 28, 2017 13:56
And use this in save-analysis, which used to read the map directly.
This is an attempt to sidestep the failure occuring on homu that I
cannot reproduce.
@nikomatsakis
Copy link
Contributor Author

@bors r=eddyb

@bors
Copy link
Contributor

bors commented Apr 28, 2017

📌 Commit d7d3f19 has been approved by eddyb

@bors
Copy link
Contributor

bors commented Apr 28, 2017

⌛ Testing commit d7d3f19 with merge 543f365...

@bors
Copy link
Contributor

bors commented Apr 28, 2017

💔 Test failed - status-appveyor

@TimNN
Copy link
Contributor

TimNN commented Apr 28, 2017

@nikomatsakis
Copy link
Contributor Author

@bors rollup-

@bors
Copy link
Contributor

bors commented Apr 28, 2017

⌛ Testing commit d7d3f19 with merge 128aa26...

bors added a commit that referenced this pull request Apr 28, 2017
make *most* maps private

Currently we access the `DepTrackingMap` fields directly rather than using the query accessors. This seems bad. This branch removes several such uses, but not all, and extends the macro so that queries can hide their maps (so we can prevent regressions). The extension to the macro is kind of ugly :/ but couldn't find a simple way to do it otherwise (I guess I could use a nested macro...). Anyway I figure it's only temporary.

r? @eddyb
@bors
Copy link
Contributor

bors commented Apr 28, 2017

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

@bors bors merged commit d7d3f19 into rust-lang:master Apr 28, 2017
This was referenced Apr 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants