Move next node ID to Resolver by Mark-Simulacrum · Pull Request #66072 · rust-lang/rust

Mark-Simulacrum · 2019-11-04T02:46:18Z

This moves the next_node_id method(s) and related tracking information to the resolver. By doing so, we also remove the OneThread and Cell on next_node_id in Session in this move, which means that the new code is simpler and less "interesting" as it doesn't tie itself to a single thread.

This required moving some of the pretty-printing logic around, but this was just copying the code without any semantic changes, so it's just a second commit instead of a separate PR; I can polish it up a bit more if desired.

Mark-Simulacrum · 2019-11-04T02:47:52Z

cc @petrochenkov -- this is hopefully "obviously correct" but I could imagine us actually wanting to embed the field somewhere other than resolver, I'm not sure. Happy to move it further back or something else.

rust-highfive · 2019-11-04T03:17:59Z

The job x86_64-gnu-llvm-6.0 of your PR failed (pretty log, 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-11-04T02:59:48.0675705Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-11-04T02:59:48.0888102Z ##[command]git config gc.auto 0
2019-11-04T02:59:48.0963503Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-11-04T02:59:48.1018526Z ##[command]git config --get-all http.proxy
2019-11-04T02:59:48.1176083Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/66072/merge:refs/remotes/pull/66072/merge
---
2019-11-04T03:06:11.8713236Z Compiling serde_json v1.0.40
2019-11-04T03:06:13.8548970Z Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-11-04T03:06:26.0011400Z Finished release [optimized] target(s) in 1m 34s
2019-11-04T03:06:26.0095722Z tidy check
2019-11-04T03:06:26.5489726Z tidy error: /checkout/src/librustc_interface/pretty.rs: empty file
2019-11-04T03:06:26.5490566Z tidy error: /checkout/src/librustc_interface/pretty.rs: leading newline
2019-11-04T03:06:26.6915224Z tidy error: /checkout/src/librustc/session/config.rs:2625: line longer than 100 chars
2019-11-04T03:06:26.6916196Z tidy error: /checkout/src/librustc/session/config.rs: too many lines (3040) (add `// ignore-tidy-filelength` to the file to suppress this error)
2019-11-04T03:06:28.5993865Z Found 485 error codes
2019-11-04T03:06:28.5996216Z Found 0 error codes with no tests
2019-11-04T03:06:28.5997013Z Done!
2019-11-04T03:06:28.5997306Z some tidy checks failed
2019-11-04T03:06:28.5997306Z some tidy checks failed
2019-11-04T03:06:28.5997476Z 2019-11-04T03:06:28.5997625Z 2019-11-04T03:06:28.5998856Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-11-04T03:06:28.5999378Z 2019-11-04T03:06:28.5999548Z 2019-11-04T03:06:28.6005886Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-11-04T03:06:28.6006208Z Build completed unsuccessfully in 0:01:38
2019-11-04T03:06:28.6006208Z Build completed unsuccessfully in 0:01:38
2019-11-04T03:06:28.6059993Z == clock drift check ==
2019-11-04T03:06:28.6069627Z local time: Mon Nov 4 03:06:28 UTC 2019
2019-11-04T03:06:28.7579880Z network time: Mon, 04 Nov 2019 03:06:28 GMT
2019-11-04T03:06:28.7580025Z == end clock drift check ==
2019-11-04T03:06:30.2093586Z 2019-11-04T03:06:30.2209414Z ##[error]Bash exited with code '1'.
2019-11-04T03:06:30.2236532Z ##[section]Starting: Checkout
2019-11-04T03:06:30.2238286Z ==============================================================================
2019-11-04T03:06:30.2238356Z Task : Get sources
2019-11-04T03:06:30.2238405Z Description : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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)

Centril

It would be good to include the "why" of this change in the PR description.

Centril · 2019-11-04T04:32:08Z

Could we add a method next_node_id on LoweringContext that just delegates to self.resolver so that the diff becomes smaller in case we want to move it again some time?

Centril · 2019-11-04T04:38:06Z

Aw :( -- Ideally we should move stuff out of librustc instead of the other direction... perhaps we can move most of this file out of librustc...

I think it's fine to do so but it's also a pretty involved change (perhaps we want to create a librustc_pretty or something) -- I don't think it should be done in this PR.

Yea; didn't mean in this PR. :)

bors · 2019-11-04T12:18:57Z

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

rust-highfive · 2019-11-04T13:44:09Z

The job x86_64-gnu-llvm-6.0 of your PR failed (pretty log, 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-11-04T13:25:35.3092483Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-11-04T13:25:35.3293810Z ##[command]git config gc.auto 0
2019-11-04T13:25:35.3372696Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-11-04T13:25:35.3425322Z ##[command]git config --get-all http.proxy
2019-11-04T13:25:35.3602390Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/66072/merge:refs/remotes/pull/66072/merge
---
2019-11-04T13:31:52.5313822Z Compiling serde_json v1.0.40
2019-11-04T13:31:54.1786324Z Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2019-11-04T13:32:05.0048737Z Finished release [optimized] target(s) in 1m 24s
2019-11-04T13:32:05.0128416Z tidy check
2019-11-04T13:32:05.4803052Z tidy error: /checkout/src/librustc_interface/pretty.rs: empty file
2019-11-04T13:32:05.4803125Z tidy error: /checkout/src/librustc_interface/pretty.rs: leading newline
2019-11-04T13:32:05.6027670Z tidy error: /checkout/src/librustc/session/config.rs:2625: line longer than 100 chars
2019-11-04T13:32:05.6028523Z tidy error: /checkout/src/librustc/session/config.rs: too many lines (3040) (add `// ignore-tidy-filelength` to the file to suppress this error)
2019-11-04T13:32:07.2757884Z some tidy checks failed
2019-11-04T13:32:07.2758681Z Found 485 error codes
2019-11-04T13:32:07.2758955Z Found 0 error codes with no tests
2019-11-04T13:32:07.2759253Z Done!
2019-11-04T13:32:07.2759253Z Done!
2019-11-04T13:32:07.2788202Z 2019-11-04T13:32:07.2788561Z 2019-11-04T13:32:07.2789614Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2019-11-04T13:32:07.2790048Z 2019-11-04T13:32:07.2790208Z 2019-11-04T13:32:07.2790437Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2019-11-04T13:32:07.2790606Z Build completed unsuccessfully in 0:01:28
2019-11-04T13:32:07.2790606Z Build completed unsuccessfully in 0:01:28
2019-11-04T13:32:07.2820340Z == clock drift check ==
2019-11-04T13:32:07.2828978Z local time: Mon Nov 4 13:32:07 UTC 2019
2019-11-04T13:32:07.3668688Z network time: Mon, 04 Nov 2019 13:32:07 GMT
2019-11-04T13:32:07.3670727Z == end clock drift check ==
2019-11-04T13:32:08.7502682Z 2019-11-04T13:32:08.7603267Z ##[error]Bash exited with code '1'.
2019-11-04T13:32:08.7629184Z ##[section]Starting: Checkout
2019-11-04T13:32:08.7630978Z ==============================================================================
2019-11-04T13:32:08.7631043Z Task : Get sources
2019-11-04T13:32:08.7631082Z Description : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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)

nikomatsakis · 2019-11-08T21:03:27Z

@bors r+

bors · 2019-11-08T21:03:28Z

📌 Commit 85f08b5 has been approved by nikomatsakis

…omatsakis Move next node ID to Resolver This moves the `next_node_id` method(s) and related tracking information to the resolver. By doing so, we also remove the OneThread and Cell on next_node_id in Session in this move, which means that the new code is simpler and less "interesting" as it doesn't tie itself to a single thread. This required moving some of the pretty-printing logic around, but this was just copying the code without any semantic changes, so it's just a second commit instead of a separate PR; I can polish it up a bit more if desired.

Centril · 2019-11-09T00:12:52Z

Failed in #66231 (comment), @bors r-

This doesn't migrate the pretty-printing everybody loops, which will be done in the next few commits.

This allows us to query whether PpmEveryBodyLoops is set during expansion and run the everybody loops pass.

This function was only ever called with 1 so there's little point in it; this isn't an expensive operation (essentially a checked add) so we're not really "reserving" anything either.

Mark-Simulacrum · 2019-11-09T16:38:58Z

@bors r=nikomatsakis

bors · 2019-11-09T16:39:00Z

📌 Commit 43a7405 has been approved by nikomatsakis

bors · 2019-11-10T07:47:04Z

⌛ Testing commit 43a7405 with merge 86c2832...

Move next node ID to Resolver This moves the `next_node_id` method(s) and related tracking information to the resolver. By doing so, we also remove the OneThread and Cell on next_node_id in Session in this move, which means that the new code is simpler and less "interesting" as it doesn't tie itself to a single thread. This required moving some of the pretty-printing logic around, but this was just copying the code without any semantic changes, so it's just a second commit instead of a separate PR; I can polish it up a bit more if desired.

bors · 2019-11-10T11:15:54Z

☀️ Test successful - checks-azure
Approved by: nikomatsakis
Pushing 86c2832 to master...

rust-highfive assigned nikomatsakis Nov 4, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 4, 2019

This comment has been minimized.
Sign in to view

Mark-Simulacrum force-pushed the next-node-id branch from eadf82a to 60d5f4fCompare November 4, 2019 02:59

Centril reviewed Nov 4, 2019
View reviewed changes

petrochenkov assigned petrochenkov and unassigned petrochenkov Nov 4, 2019

Mark-Simulacrum force-pushed the next-node-id branch from 60d5f4f to 5bbcd85Compare November 4, 2019 13:20

Mark-Simulacrum force-pushed the next-node-id branch from 181fafa to 85f08b5Compare November 4, 2019 14:47

nikomatsakis approved these changes Nov 8, 2019
View reviewed changes

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 Nov 8, 2019

Centril mentioned this pull request Nov 8, 2019
Rollup of 6 pull requests #66231
Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 9, 2019

Mark-Simulacrum added 3 commits November 9, 2019 11:10

Move next_node_id to Resolver
516a817
This doesn't migrate the pretty-printing everybody loops, which will be done in the next few commits.

Move pretty parsing into Session options
dd6df0d
This allows us to query whether PpmEveryBodyLoops is set during expansion and run the everybody loops pass.

Inline reserve_node_ids
43a7405
This function was only ever called with 1 so there's little point in it; this isn't an expensive operation (essentially a checked add) so we're not really "reserving" anything either.

Mark-Simulacrum force-pushed the next-node-id branch from 85f08b5 to 43a7405Compare November 9, 2019 16:10

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 9, 2019

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 10, 2019

bors merged commit 43a7405 into rust-lang:masterNov 10, 2019

Uh oh!

Uh oh!

Conversation

Mark-Simulacrum commented Nov 4, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Mark-Simulacrum commented Nov 4, 2019

Uh oh!

This comment has been minimized.

rust-highfive commented Nov 4, 2019

Uh oh!

Centril left a comment

Choose a reason for hiding this comment

Uh oh!

CentrilNov 4, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CentrilNov 4, 2019

Choose a reason for hiding this comment

Uh oh!

Mark-SimulacrumNov 4, 2019

Choose a reason for hiding this comment

Uh oh!

CentrilNov 9, 2019

Choose a reason for hiding this comment

Uh oh!

bors commented Nov 4, 2019

Uh oh!

rust-highfive commented Nov 4, 2019

Uh oh!

nikomatsakis commented Nov 8, 2019

Uh oh!

bors commented Nov 8, 2019

Uh oh!

Centril commented Nov 9, 2019

Uh oh!

Mark-Simulacrum commented Nov 9, 2019

Uh oh!

bors commented Nov 9, 2019

Uh oh!

bors commented Nov 10, 2019

Uh oh!

bors commented Nov 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Mark-Simulacrum commented Nov 4, 2019•
edited
Loading