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.

Sign up for GitHub

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

Jump to bottom

Calculate privacy access only via query #57343

Merged
merged 2 commits into from
Jan 5, 2019
Merged

Calculate privacy access only via query #57343

merged 2 commits into from
Jan 5, 2019

Conversation

Xanewok
Copy link
Member

@Xanewok Xanewok commented Jan 4, 2019

Initially converted to query in a9f6bab and then changed to respect dependencies 8281e88.

I did this as an effort to prune CrateAnalysis from librustc_save_analysis, with the only thing remaining being the glob map (name is unused, existing crate_name is exposed in the compiler passes, instead).

Since calculating the glob map is opt-in, it'd be great if we could calculate that on-demand. However, it seems that it'd require converting resolution to queries, which I'm not sure how to do yet.

In an effort to get rid of CrateAnalysis altogether, could we try unconditionally calculating the glob_map in the resolver, thus completely removing CrateAnalysis struct, and doing a perf run?

r? @nikomatsakis

cc @petrochenkov do you have any idea how/if at all could we querify the resolver? I've stumbled upon a comment that's ~3? years old at the moment, so I'm guessing things might have changed and it actually may be feasible now.

rust/src/librustc_driver/driver.rs

Lines 589 to 593 in fe0c100

// Currently, we ignore the name resolution data structures for the purposes of dependency
// tracking. Instead we will run name resolution and include its output in the hash of each
// item, much like we do for macro expansion. In other words, the hash reflects not just
// its contents but the results of name resolution on those contents. Hopefully we'll push
// this back at some point.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 4, 2019
@nikomatsakis
Copy link
Contributor

@bors r+

Regarding making name resolution results into a query, I think that's definitely feasible but it's going to require some thought to try and do name resolution at anything other than the crate granularity. This conversion is a big part of what we often call "end-to-end queries" (along with parsing). Still, even converting it into a "crate-wide phase" would be awesome.

@bors
Copy link
Contributor

bors commented Jan 4, 2019

📌 Commit 480d0f3 has been approved by nikomatsakis

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 4, 2019
kennytm added a commit to kennytm/rust that referenced this pull request Jan 5, 2019
@kennytm
Rollup merge of rust-lang#57343 - Xanewok:querify-access-levels, r=ni…
d9885c4 
…komatsakis

Calculate privacy access only via query

Initially converted to query in rust-lang@a9f6bab and then changed to respect dependencies rust-lang@8281e88.

I did this as an effort to prune `CrateAnalysis` from librustc_save_analysis, with the only thing remaining being the glob map (`name` is unused, existing `crate_name` is exposed in the compiler passes, instead).

Since calculating the glob map is opt-in, it'd be great if we could calculate that on-demand. However, it seems that it'd require converting resolution to queries, which I'm not sure how to do yet.

In an effort to get rid of `CrateAnalysis` altogether, could we try unconditionally calculating the glob_map in the resolver, thus completely removing `CrateAnalysis` struct, and doing a perf run?

r? @nikomatsakis

cc @petrochenkov do you have any idea how/if at all could we querify the resolver? I've stumbled upon a comment that's ~3? years old at the moment, so I'm guessing things might have changed and it actually may be feasible now. https://github.com/rust-lang/rust/blob/fe0c10019d7ee96909cc42cc265ef999a6b5dd70/src/librustc_driver/driver.rs#L589-L593
@kennytm kennytm mentioned this pull request Jan 5, 2019
Merged
bors added a commit that referenced this pull request Jan 5, 2019
@bors
Auto merge of #57354 - kennytm:rollup, r=kennytm
68fe518 
Rollup of 17 pull requests

Successful merges:

 - #57219 (Remove some unused code)
 - #57229 (Fix #56806 by using `delay_span_bug` in object safety layout sanity checks)
 - #57233 (Rename and fix nolink-with-link-args test)
 - #57238 (Fix backtraces for inlined functions on Windows)
 - #57249 (Fix broken links to second edition TRPL.)
 - #57267 (src/jemalloc is gone, remove its mention from COPYRIGHT)
 - #57273 (Update the stdsimd submodule)
 - #57278 (Add Clippy to config.toml.example)
 - #57295 (Fix 'be be' constructs)
 - #57311 (VaList::copy should not require a mutable ref)
 - #57312 (`const fn` is no longer coming soon (const keyword docs))
 - #57313 (Improve Box<T> -> Pin<Box<T>> conversion)
 - #57314 (Fix repeated word typos)
 - #57326 (Doc rewording, use the same name `writer`)
 - #57338 (rustdoc: force binary filename for compiled doctests)
 - #57342 (librustc_mir: Make qualify_min_const_fn module public)
 - #57343 (Calculate privacy access only via query)

Failed merges:

 - #57340 (Use correct tracking issue for c_variadic)

r? @ghost
@Xanewok Xanewok mentioned this pull request Jan 5, 2019
Open
32 tasks
@bors bors merged commit 480d0f3 into rust-lang:master Jan 5, 2019
@Xanewok Xanewok deleted the querify-access-levels branch January 5, 2019 21:29
@Xanewok Xanewok mentioned this pull request Jan 8, 2019
Merged
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Jan 12, 2019
@pietroalbini
Rollup merge of rust-lang#57436 - Xanewok:save-analysis-access-ice-fi…
f014b94 
…x, r=nikomatsakis

save-analysis: use a fallback when access levels couldn't be computed

Fixing an RLS regression I introduced in rust-lang#57343 😢

I missed a case where we get [called back with analysis when type checking fails](https://github.com/rust-lang/rust/blob/9d54812829e9d92dac35a4a0f358cdc5a2475371/src/librustc_driver/driver.rs#L1264). Since privacy checking normally is done afterwards, when we execute the `privacy_access_levels` query inside the save_analysis callback we'll calculate it for the first time and since typeck info isn't complete, we'll crash there.

Double-checked locally and it seems to have fixed the problem.

r? @nikomatsakis
Centril added a commit to Centril/rust that referenced this pull request Jan 13, 2019
@Centril
Rollup merge of rust-lang#57436 - Xanewok:save-analysis-access-ice-fi…
8e3980b 
…x, r=nikomatsakis

save-analysis: use a fallback when access levels couldn't be computed

Fixing an RLS regression I introduced in rust-lang#57343 😢

I missed a case where we get [called back with analysis when type checking fails](https://github.com/rust-lang/rust/blob/9d54812829e9d92dac35a4a0f358cdc5a2475371/src/librustc_driver/driver.rs#L1264). Since privacy checking normally is done afterwards, when we execute the `privacy_access_levels` query inside the save_analysis callback we'll calculate it for the first time and since typeck info isn't complete, we'll crash there.

Double-checked locally and it seems to have fixed the problem.

r? @nikomatsakis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nikomatsakis nikomatsakis

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Xanewok @nikomatsakis @bors @rust-highfive