Calculate visibilities once in resolve #78077

petrochenkov · 2020-10-18T16:48:58Z

Then use them through a query based on resolver outputs.

Item visibilities were previously calculated in three places - initially in rustc_resolve, then in rustc_privacy during type privacy checkin, and then in rustc_metadata during metadata encoding.
The visibility logic is not entirely trivial, especially for things like constructors or enum variants, and all of it was duplicated.

This PR deduplicates all the visibility calculations, visibilities are determined once during early name resolution and then stored in ResolverOutputs and are later available through tcx as a query tcx.visibility(def_id).
(This query existed previously, but only worked for other crates.)

Some special cases (e.g. visibilities for closure types, which are needed for type privacy checking) are not processed in resolve, but deferred and performed directly in the query instead.

rust-highfive · 2020-10-18T16:49:01Z

r? @lcnr

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

petrochenkov · 2020-10-18T16:49:12Z

@bors try

bors · 2020-10-18T16:49:22Z

⌛ Trying commit 127846e4b8f7fe34f753c1f6018e291fd38bf171 with merge 7bc5bf62df28f36352ac7daacddb8017d94d9f8a...

lcnr

So looking through this I didn't see anything which looked out of place, but I am not familiar enough with this code to actually review these changes 🤔

compiler/rustc_privacy/src/lib.rs

lcnr · 2020-10-18T17:25:02Z

https://github.com/rust-lang/compiler-team/blob/master/content/experts/map.toml mentions @davidtwco but feel free to reassign

r? @davidtwco for now

bors · 2020-10-18T17:34:00Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: 7bc5bf62df28f36352ac7daacddb8017d94d9f8a (7bc5bf62df28f36352ac7daacddb8017d94d9f8a)

petrochenkov · 2020-10-18T18:02:52Z

Question: is it possible to make a query eval_always for the current crate and not eval_always for other crates?
(Or even make it use some more complex condition?)

As I understand, the query must be eval_always because it accesses data from ResolverOutputs which is built before query system starts working, but for visibilities from other crates it would be undesirable to decode them from metadata again and again.

Does it have to be split into multiple queries in this case - some eval_always and some not?

UPD: It looks like eval_always only affects the incremental case (multiple compilations), and queries are always memoized during a single compilation.

petrochenkov · 2020-10-18T18:15:46Z

@bors try @rust-timer queue

rust-timer · 2020-10-18T18:15:47Z

Awaiting bors try build completion

bors · 2020-10-18T18:15:57Z

⌛ Trying commit 49e528cdbe56463eaaadaa10e7397f3127bb0203 with merge 0b7c494c748e1465d977c8fa3a39cd7e1944787b...

src/test/ui/pub/issue-33174-restricted-type-in-public-interface.rs

src/test/ui/proc-macro/issue-50493.stderr

bors · 2020-10-18T18:58:41Z

☀️ Try build successful - checks-actions, checks-azure
Build commit: 0b7c494c748e1465d977c8fa3a39cd7e1944787b (0b7c494c748e1465d977c8fa3a39cd7e1944787b)

rust-timer · 2020-10-18T18:58:43Z

Queued 0b7c494c748e1465d977c8fa3a39cd7e1944787b with parent 834821e, future comparison URL.

rust-timer · 2020-10-18T23:10:59Z

Finished benchmarking try commit (0b7c494c748e1465d977c8fa3a39cd7e1944787b): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

Then use them through a query based on resolver outputs

davidtwco

LGTM, r=me if you're happy with this.

petrochenkov · 2020-10-19T11:28:56Z

@bors r=davidtwco

bors · 2020-10-19T11:28:58Z

📌 Commit cee5521 has been approved by davidtwco

bors · 2020-10-21T20:23:45Z

⌛ Testing commit cee5521 with merge 1eaadeb...

bors · 2020-10-21T22:37:20Z

☀️ Test successful - checks-actions, checks-azure
Approved by: davidtwco
Pushing 1eaadeb to master...

Mark-Simulacrum · 2020-10-27T15:06:22Z

Final perf results were approximately equivalent to the perf run on this PR; some solid improvement on deeply-nested-async benchmark in particular. Good work!

Pkgsrc changes: * Adjust patches, convert tabs to spaces so that tests pass. * Remove patches which are no longer needed (upstream changed) * Minor adjustments for SunOS, e.g. disable stack probes. * Adjust cargo checksum patching accordingly. * Remove commented-out use of PATCHELF on NetBSD, which doesn't work anyway... Upstream changes: Version 1.49.0 (2020-12-31) ============================ Language ----------------------- - [Unions can now implement `Drop`, and you can now have a field in a union with `ManuallyDrop<T>`.][77547] - [You can now cast uninhabited enums to integers.][76199] - [You can now bind by reference and by move in patterns.][76119] This allows you to selectively borrow individual components of a type. E.g. ```rust #[derive(Debug)] struct Person { name: String, age: u8, } let person = Person { name: String::from("Alice"), age: 20, }; // `name` is moved out of person, but `age` is referenced. let Person { name, ref age } = person; println!("{} {}", name, age); ``` Compiler ----------------------- - [Added tier 1\* support for `aarch64-unknown-linux-gnu`.][78228] - [Added tier 2 support for `aarch64-apple-darwin`.][75991] - [Added tier 2 support for `aarch64-pc-windows-msvc`.][75914] - [Added tier 3 support for `mipsel-unknown-none`.][78676] - [Raised the minimum supported LLVM version to LLVM 9.][78848] - [Output from threads spawned in tests is now captured.][78227] - [Change os and vendor values to "none" and "unknown" for some targets][78951] \* Refer to Rust's [platform support page][forge-platform-support] for more information on Rust's tiered platform support. Libraries ----------------------- - [`RangeInclusive` now checks for exhaustion when calling `contains` and indexing.][78109] - [`ToString::to_string` now no longer shrinks the internal buffer in the default implementation.][77997] - [`ops::{Index, IndexMut}` are now implemented for fixed sized arrays of any length.][74989] Stabilized APIs --------------- - [`slice::select_nth_unstable`] - [`slice::select_nth_unstable_by`] - [`slice::select_nth_unstable_by_key`] The following previously stable methods are now `const`. - [`Poll::is_ready`] - [`Poll::is_pending`] Cargo ----------------------- - [Building a crate with `cargo-package` should now be independently reproducible.][cargo/8864] - [`cargo-tree` now marks proc-macro crates.][cargo/8765] - [Added `CARGO_PRIMARY_PACKAGE` build-time environment variable.] [cargo/8758] This variable will be set if the crate being built is one the user selected to build, either with `-p` or through defaults. - [You can now use glob patterns when specifying packages & targets.][cargo/8752] Compatibility Notes ------------------- - [Demoted `i686-unknown-freebsd` from host tier 2 to target tier 2 support.][78746] - [Macros that end with a semi-colon are now treated as statements even if they expand to nothing.][78376] - [Rustc will now check for the validity of some built-in attributes on enum variants.][77015] Previously such invalid or unused attributes could be ignored. - Leading whitespace is stripped more uniformly in documentation comments, which may change behavior. You read [this post about the changes][rustdoc-ws-post] for more details. - [Trait bounds are no longer inferred for associated types.][79904] Internal Only ------------- These changes provide no direct user facing benefits, but represent significant improvements to the internals and overall performance of rustc and related tools. - [rustc's internal crates are now compiled using the `initial-exec` Thread Local Storage model.][78201] - [Calculate visibilities once in resolve.][78077] - [Added `system` to the `llvm-libunwind` bootstrap config option.][77703] - [Added `--color` for configuring terminal color support to bootstrap.][79004] [75991]: rust-lang/rust#75991 [78951]: rust-lang/rust#78951 [78848]: rust-lang/rust#78848 [78746]: rust-lang/rust#78746 [78376]: rust-lang/rust#78376 [78228]: rust-lang/rust#78228 [78227]: rust-lang/rust#78227 [78201]: rust-lang/rust#78201 [78109]: rust-lang/rust#78109 [78077]: rust-lang/rust#78077 [77997]: rust-lang/rust#77997 [77703]: rust-lang/rust#77703 [77547]: rust-lang/rust#77547 [77015]: rust-lang/rust#77015 [76199]: rust-lang/rust#76199 [76119]: rust-lang/rust#76119 [75914]: rust-lang/rust#75914 [74989]: rust-lang/rust#74989 [79004]: rust-lang/rust#79004 [78676]: rust-lang/rust#78676 [79904]: rust-lang/rust#79904 [cargo/8864]: rust-lang/cargo#8864 [cargo/8765]: rust-lang/cargo#8765 [cargo/8758]: rust-lang/cargo#8758 [cargo/8752]: rust-lang/cargo#8752 [`slice::select_nth_unstable`]: https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.select_nth_unstable [`slice::select_nth_unstable_by`]: https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.select_nth_unstable_by [`slice::select_nth_unstable_by_key`]: https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.select_nth_unstable_by_key [`hint::spin_loop`]: https://doc.rust-lang.org/stable/std/hint/fn.spin_loop.html [`Poll::is_ready`]: https://doc.rust-lang.org/stable/std/task/enum.Poll.html#method.is_ready [`Poll::is_pending`]: https://doc.rust-lang.org/stable/std/task/enum.Poll.html#method.is_pending [rustdoc-ws-post]: https://blog.guillaume-gomez.fr/articles/2020-11-11+New+doc+comment+handling+in+rustdoc

rust-highfive assigned lcnr Oct 18, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 18, 2020

petrochenkov mentioned this pull request Oct 18, 2020

Make tcx.visibility() work for items in the local crate #77889

Closed

lcnr reviewed Oct 18, 2020

View reviewed changes

compiler/rustc_privacy/src/lib.rs Outdated Show resolved Hide resolved

rust-highfive assigned davidtwco and unassigned lcnr Oct 18, 2020

jyn514 added A-visibility Area: Visibility / privacy T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels Oct 18, 2020

jyn514 reviewed Oct 18, 2020

View reviewed changes

src/test/ui/pub/issue-33174-restricted-type-in-public-interface.rs Show resolved Hide resolved

src/test/ui/proc-macro/issue-50493.stderr Show resolved Hide resolved

Calculate visibilities once in resolve

cee5521

Then use them through a query based on resolver outputs

petrochenkov force-pushed the qvis branch from 49e528c to cee5521 Compare October 19, 2020 09:07

davidtwco approved these changes Oct 19, 2020

View reviewed changes

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 19, 2020

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 19, 2020

jonas-schievink added the relnotes-perf Performance improvements that should be mentioned in the release notes. label Oct 19, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 21, 2020

bors merged commit 1eaadeb into rust-lang:master Oct 21, 2020

rustbot added this to the 1.49.0 milestone Oct 21, 2020

petrochenkov mentioned this pull request Oct 21, 2020

Add Item::from_def_id_and_kind to reduce duplication in rustdoc #77820

Merged

This was referenced Oct 21, 2020

rustc_metadata: Deduplicate code between encode_info_for_item, encode_info_for_foreign_item, encode_info_for_trait_item, and encode_info_for_impl_item #78081

Closed

TypeVisitor: use std::ops::ControlFlow instead of bool #78182

Merged

JohnTitor mentioned this pull request Oct 22, 2020

ICE from public Macro 2.0 + use crate as #77475

Closed

jyn514 mentioned this pull request Oct 22, 2020

ICE 'called Option::unwrap() on a None value', compiler/rustc_metadata/src/rmeta/decoder.rs:975:64 #78236

Open

JohnTitor mentioned this pull request Oct 23, 2020

Add regression test for issue-77475 #78264

Merged

jyn514 mentioned this pull request Dec 1, 2020

Diagnostic regression (1.47 -> 1.48): public enum fields incorrectly described as "inaccessible fields". #79593

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Calculate visibilities once in resolve #78077

Calculate visibilities once in resolve #78077

petrochenkov commented Oct 18, 2020

rust-highfive commented Oct 18, 2020

petrochenkov commented Oct 18, 2020

bors commented Oct 18, 2020

lcnr left a comment

lcnr commented Oct 18, 2020

bors commented Oct 18, 2020

petrochenkov commented Oct 18, 2020 •

edited

Loading

petrochenkov commented Oct 18, 2020

rust-timer commented Oct 18, 2020

bors commented Oct 18, 2020

bors commented Oct 18, 2020

rust-timer commented Oct 18, 2020

rust-timer commented Oct 18, 2020

davidtwco left a comment

petrochenkov commented Oct 19, 2020

bors commented Oct 19, 2020

bors commented Oct 21, 2020

bors commented Oct 21, 2020

Mark-Simulacrum commented Oct 27, 2020

Calculate visibilities once in resolve #78077

Calculate visibilities once in resolve #78077

Conversation

petrochenkov commented Oct 18, 2020

rust-highfive commented Oct 18, 2020

petrochenkov commented Oct 18, 2020

bors commented Oct 18, 2020

lcnr left a comment

Choose a reason for hiding this comment

lcnr commented Oct 18, 2020

bors commented Oct 18, 2020

petrochenkov commented Oct 18, 2020 • edited Loading

petrochenkov commented Oct 18, 2020

rust-timer commented Oct 18, 2020

bors commented Oct 18, 2020

bors commented Oct 18, 2020

rust-timer commented Oct 18, 2020

rust-timer commented Oct 18, 2020

davidtwco left a comment

Choose a reason for hiding this comment

petrochenkov commented Oct 19, 2020

bors commented Oct 19, 2020

bors commented Oct 21, 2020

bors commented Oct 21, 2020

Mark-Simulacrum commented Oct 27, 2020

petrochenkov commented Oct 18, 2020 •

edited

Loading