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

resolve: Populate external modules in more automatic and lazy way #63149

Merged
merged 4 commits into from Aug 17, 2019

Conversation

@petrochenkov
Copy link
Contributor

commented Jul 30, 2019

So, resolve had this function populate_module_if_necessary for loading module children from other crates from metadata.
I never really understood when it should've been called and when not.
This PR removes the function and loads the module children automatically on the first access instead.

r? @eddyb

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2019

⌛️ Trying commit 2245577 with merge ac89e03...

bors added a commit that referenced this pull request Jul 30, 2019

Auto merge of #63149 - petrochenkov:lazypop2, r=<try>
resolve: Populate external modules in more automatic and lazy way

So, resolve had this function `populate_module_if_necessary` for loading module children from other crates from metadata.
I never really understood when it should've been called and when not.
This PR removes the function and loads the module children automatically on the first access instead.

r? @eddyb
@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

Hmm, no notification, but looks like the build completed successfully.
@rust-timer build ac89e03

@rust-timer

This comment has been minimized.

Copy link

commented Jul 31, 2019

Success: Queued ac89e03 with parent f690098, comparison URL.

@rust-timer

This comment has been minimized.

Copy link

commented Jul 31, 2019

Finished benchmarking try commit ac89e03, comparison URL.

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

Ok, perf looks mostly like a noise, but the change still makes sense logically.

@petrochenkov petrochenkov force-pushed the petrochenkov:lazypop2 branch from d2fd5f9 to cbaf397 Jul 31, 2019

@ProgrammaticNajel

This comment has been minimized.

Copy link

commented Aug 9, 2019

Ping from triage. @eddyb any updates on this? Thanks.

@eddyb

eddyb approved these changes Aug 9, 2019

@eddyb

This comment has been minimized.

Copy link
Member

commented Aug 9, 2019

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

📌 Commit cbaf397 has been approved by eddyb

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

⌛️ Testing commit cbaf397 with merge 047f9fa...

bors added a commit that referenced this pull request Aug 9, 2019

Auto merge of #63149 - petrochenkov:lazypop2, r=eddyb
resolve: Populate external modules in more automatic and lazy way

So, resolve had this function `populate_module_if_necessary` for loading module children from other crates from metadata.
I never really understood when it should've been called and when not.
This PR removes the function and loads the module children automatically on the first access instead.

r? @eddyb
@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

💔 Test failed - checks-azure

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Aug 9, 2019

The job mingw-check of your PR failed (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-08-09T18:46:29.5461739Z do so (now or later) by using -b with the checkout command again. Example:
2019-08-09T18:46:29.5461926Z 
2019-08-09T18:46:29.5462310Z   git checkout -b <new-branch-name>
2019-08-09T18:46:29.5462511Z 
2019-08-09T18:46:29.5462941Z HEAD is now at 047f9faa8 Auto merge of #63149 - petrochenkov:lazypop2, r=eddyb
2019-08-09T18:46:29.5629402Z ##[section]Starting: Collect CPU-usage statistics in the background
2019-08-09T18:46:29.5632337Z ==============================================================================
2019-08-09T18:46:29.5632443Z Task         : Bash
2019-08-09T18:46:29.5632525Z Description  : Run a Bash script on macOS, Linux, or Windows
---
2019-08-09T18:54:39.9359151Z     Checking rustc_ast_borrowck v0.0.0 (/checkout/src/librustc_ast_borrowck)
2019-08-09T18:54:41.1093100Z     Checking rustc_codegen_utils v0.0.0 (/checkout/src/librustc_codegen_utils)
2019-08-09T18:54:41.6917094Z     Checking rustc_plugin v0.0.0 (/checkout/src/librustc_plugin)
2019-08-09T18:54:41.9702335Z     Checking rustc_resolve v0.0.0 (/checkout/src/librustc_resolve)
2019-08-09T18:54:43.2198530Z error[E0599]: no variant or associated item named `AssocExistential` found for type `rustc::hir::def::DefKind` in the current scope
2019-08-09T18:54:43.2204323Z    --> src/librustc_resolve/build_reduced_graph.rs:665:33
2019-08-09T18:54:43.2210247Z     |
2019-08-09T18:54:43.2211591Z 665 |             | Res::Def(DefKind::AssocExistential, _)
2019-08-09T18:54:43.2217825Z     |                                 ^^^^^^^^^^^^^^^^ variant or associated item not found in `rustc::hir::def::DefKind`
2019-08-09T18:54:43.4002363Z error: aborting due to previous error
2019-08-09T18:54:43.4002501Z 
2019-08-09T18:54:43.4003124Z For more information about this error, try `rustc --explain E0599`.
2019-08-09T18:54:43.4188001Z error: Could not compile `rustc_resolve`.
2019-08-09T18:54:43.4188001Z error: Could not compile `rustc_resolve`.
2019-08-09T18:54:43.4188400Z warning: build failed, waiting for other jobs to finish...
2019-08-09T18:54:53.7419786Z error: build failed
2019-08-09T18:54:53.7446416Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "x86_64-unknown-linux-gnu" "-j" "2" "--release" "--color" "always" "--features" "" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json"
2019-08-09T18:54:53.7447144Z expected success, got: exit code: 101
2019-08-09T18:54:53.7456765Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
2019-08-09T18:54:53.7457172Z Build completed unsuccessfully in 0:05:36
2019-08-09T18:54:55.0424325Z ##[error]Bash exited with code '1'.
2019-08-09T18:54:55.0455823Z ##[section]Starting: Upload CPU usage statistics
2019-08-09T18:54:55.0458957Z ==============================================================================
2019-08-09T18:54:55.0459055Z Task         : Bash
2019-08-09T18:54:55.0459430Z Description  : Run a Bash script on macOS, Linux, or Windows

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@petrochenkov petrochenkov force-pushed the petrochenkov:lazypop2 branch from cbaf397 to 2cb9207 Aug 9, 2019

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2019

@bors r=eddyb

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2019

📌 Commit 2cb9207 has been approved by eddyb

Centril added a commit to Centril/rust that referenced this pull request Aug 10, 2019

Rollup merge of rust-lang#63149 - petrochenkov:lazypop2, r=eddyb
resolve: Populate external modules in more automatic and lazy way

So, resolve had this function `populate_module_if_necessary` for loading module children from other crates from metadata.
I never really understood when it should've been called and when not.
This PR removes the function and loads the module children automatically on the first access instead.

r? @eddyb
@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

This might've caused the failure in #63465, though it might be a rebase error:

@bors r-

https://dev.azure.com/rust-lang/rust/_build/results?buildId=5623&view=logs&jobId=b7a895b1-8909-5023-4a22-40a4698fe576&taskId=6b895087-7934-57eb-e59a-7bf9144da3a0&lineStart=15294&lineEnd=15308&colStart=1&colEnd=1

sample:

---- [ui] ui/issues/issue-27033.rs stdout ----
diff of stderr:

3	   |
4	LL |         None @ _ => {}
5	   |         ^^^^ cannot be named the same as a unit variant
-	   | 
-	  ::: $SRC_DIR/libstd/prelude/v1.rs:LL:COL
-	   |
-	LL | pub use crate::option::Option::{self, Some, None};
-	   |                                             ---- the unit variant `None` is defined here
11	
12	error[E0530]: match bindings cannot shadow constants
13	  --> $DIR/issue-27033.rs:7:9
@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

No, not a rebase, the previous error was the same.
Looks like either randomly changing or platform-dependent diagnostics.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

(To be clear, I'm not positive that this PR was the cause of that failure, it's just my best guess. Feel free to re-r+ if you feel confident).

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

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

@petrochenkov petrochenkov force-pushed the petrochenkov:lazypop2 branch from a087df6 to 4331bf5 Aug 15, 2019

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

Span changes causing issues with diagnostics reproducibility are backed out.

@bors r=eddyb

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2019

📌 Commit 4331bf5 has been approved by eddyb

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

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

petrochenkov added some commits Jul 29, 2019

resolve: Populate external modules in more automatic and lazy way
The modules are now populated implicitly on the first access
Fix rebase
Move some code into `build_reduced_graph.rs` to keep `BuildReducedGraphVisitor` it private
Also move the def collector call to `build_reduced_graph.rs`, it belongs there.

@petrochenkov petrochenkov force-pushed the petrochenkov:lazypop2 branch from 4331bf5 to 5e47cd4 Aug 16, 2019

@petrochenkov

This comment has been minimized.

Copy link
Contributor Author

commented Aug 16, 2019

@bors r=eddyb

@bors

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2019

📌 Commit 5e47cd4 has been approved by eddyb

Centril added a commit to Centril/rust that referenced this pull request Aug 16, 2019

Rollup merge of rust-lang#63149 - petrochenkov:lazypop2, r=eddyb
resolve: Populate external modules in more automatic and lazy way

So, resolve had this function `populate_module_if_necessary` for loading module children from other crates from metadata.
I never really understood when it should've been called and when not.
This PR removes the function and loads the module children automatically on the first access instead.

r? @eddyb

bors added a commit that referenced this pull request Aug 16, 2019

Auto merge of #63644 - Centril:rollup-kapx76n, r=Centril
Rollup of 7 pull requests

Successful merges:

 - #63149 (resolve: Populate external modules in more automatic and lazy way)
 - #63175 (rustc: implement argsfiles for command line)
 - #63545 (Feature gate 'yield $expr?' pre-expansion)
 - #63548 (Update rustc-demangle to 0.1.16.)
 - #63558 (Remap paths for proc-macro crates.)
 - #63641 (add git keyword to submodule comments in config.example.toml)
 - #63642 (Rename overflowing_{add,sub,mul} intrinsics to wrapping_{add,sub,mul}.)

Failed merges:

r? @ghost

Centril added a commit to Centril/rust that referenced this pull request Aug 17, 2019

Rollup merge of rust-lang#63149 - petrochenkov:lazypop2, r=eddyb
resolve: Populate external modules in more automatic and lazy way

So, resolve had this function `populate_module_if_necessary` for loading module children from other crates from metadata.
I never really understood when it should've been called and when not.
This PR removes the function and loads the module children automatically on the first access instead.

r? @eddyb

bors added a commit that referenced this pull request Aug 17, 2019

Auto merge of #63648 - Centril:rollup-2kpdrj1, r=Centril
Rollup of 6 pull requests

Successful merges:

 - #63149 (resolve: Populate external modules in more automatic and lazy way)
 - #63545 (Feature gate 'yield $expr?' pre-expansion)
 - #63548 (Update rustc-demangle to 0.1.16.)
 - #63558 (Remap paths for proc-macro crates.)
 - #63641 (add git keyword to submodule comments in config.example.toml)
 - #63642 (Rename overflowing_{add,sub,mul} intrinsics to wrapping_{add,sub,mul}.)

Failed merges:

r? @ghost

@bors bors merged commit 5e47cd4 into rust-lang:master Aug 17, 2019

4 checks passed

pr Build #20190816.28 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-6.0) Linux x86_64-gnu-llvm-6.0 succeeded
Details
pr (LinuxTools) LinuxTools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.