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 Clean take &mut DocContext #82020

Merged
merged 2 commits into from
Feb 19, 2021
Merged

Make Clean take &mut DocContext #82020

merged 2 commits into from
Feb 19, 2021

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Feb 12, 2021

  • Take FnMut in rustc_trait_selection::find_auto_trait_generics
  • Take &mut DocContext in most of clean
  • Collect the iterator in auto_trait_impls instead of iterating lazily; the lifetimes were really bad.

This combined with #82018 should hopefully help with #82014 by allowing cx.cache.exported_traits to be modified in register_res. Previously it had to use interior mutability, which required either adding a RefCell to cache.exported_traits on top of the existing RefCell<Cache> or mixing reads and writes between cx.exported_traits and cx.cache.exported_traits. I don't currently have that working but I expect it to be reasonably easy to add after this.

@jyn514 jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Feb 12, 2021
@rust-highfive

This comment has been minimized.

@rust-highfive
Copy link
Collaborator

r? @ollie27

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 12, 2021
@jyn514
Copy link
Member Author

jyn514 commented Feb 12, 2021

Hmm, maybe r? @camelid? The changes are all fairly simple, there's just a lot of them 😅 sorry for all the work

@rust-highfive rust-highfive assigned camelid and unassigned ollie27 Feb 12, 2021
@bors

This comment has been minimized.

@camelid camelid changed the title Make Clean take &mut DocContext Make Clean take &mut DocContext Feb 13, 2021
Copy link
Member

@camelid camelid left a comment

Choose a reason for hiding this comment

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

Left an initial review on some parts (haven't yet reviewed some of the files with massive changes like clean/mod.rs).

There were several places with statements like let cx = self.cx; or let tcx = self.cx.tcx and changes from cx.sess() to cx.tcx.sess. I'm assuming these changes are necessary to please the borrow-checker?

src/librustdoc/clean/auto_trait.rs Outdated Show resolved Hide resolved

debug!("get_auto_trait_impls({:?})", ty);
let auto_traits = self.cx.auto_traits.iter().cloned();
let auto_traits: Vec<_> = self.cx.borrow().auto_traits.iter().cloned().collect();
Copy link
Member

Choose a reason for hiding this comment

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

Is this collect being added because otherwise there were borrowck errors?

Copy link
Member Author

@jyn514 jyn514 Feb 15, 2021

Choose a reason for hiding this comment

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

Yes, otherwise self.cx is used both here and inside the filter_map closure at the same time (auto_traits borrows from cx and .clean(cx) requires unique access to the reference).

src/librustdoc/clean/auto_trait.rs Show resolved Hide resolved
src/librustdoc/clean/auto_trait.rs Outdated Show resolved Hide resolved
src/librustdoc/clean/utils.rs Outdated Show resolved Hide resolved
@jyn514
Copy link
Member Author

jyn514 commented Feb 15, 2021

There were several places with statements like let cx = self.cx; or let tcx = self.cx.tcx and changes from cx.sess() to cx.tcx.sess. I'm assuming these changes are necessary to please the borrow-checker?

Yes.

error[E0500]: closure requires unique access to `self` but it is already borrowed
  --> src/librustdoc/clean/auto_trait.rs:43:25
   |
41 |         let auto_traits = self.cx.auto_traits.iter().cloned();
   |                           ------------------- borrow occurs here
42 |         auto_traits
43 |             .filter_map(|trait_def_id| {
   |              ---------- ^^^^^^^^^^^^^^ closure construction occurs here
   |              |
   |              first borrow later used by call
...
46 |                     substs: self.cx.tcx.mk_substs_trait(ty, &[]),
   |                             ---- second borrow occurs due to use of `self` in closure

@jyn514
Copy link
Member Author

jyn514 commented Feb 15, 2021

Well, these were certainly necessary (#82020 (comment)):

statements like let tcx = self.cx.tcx and changes from cx.sess() to cx.tcx.sess.

but these may have been extraneous:

let cx = self.cx;

I'll go through the changes again and see if I can revert them.

Comment on lines 474 to 473
let replaced = p.fold_with(&mut replacer);
let (orig_p, p) = (replaced, replaced.clean(cx));

let (orig_p, p) = (p, p.clean(self.cx));
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a behavior change.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not actually - it's just that the predicate is folded above (in clean_where_predicates) instead of here. It looks confusing because you're only looking at 2 commits, not the full diff.

src/librustdoc/clean/auto_trait.rs Show resolved Hide resolved
@camelid
Copy link
Member

camelid commented Feb 15, 2021

Am I right in thinking that this will make getting rid of the RefCells in DocContext a lot easier?

@jyn514
Copy link
Member Author

jyn514 commented Feb 15, 2021

Am I right in thinking that this will make getting rid of the RefCells in DocContext a lot easier?

Much so, yes. That would be a good follow up pr, but I didn't want to do it here because it's already +-300.

@camelid
Copy link
Member

camelid commented Feb 15, 2021

Much so, yes. That would be a good follow up pr, but I didn't want to do it here because it's already +-300.

Yeah, I might try doing it after this PR if that's okay with you :)

@rust-log-analyzer

This comment has been minimized.

@camelid
Copy link
Member

camelid commented Feb 16, 2021

What do you think of undoing the move to free functions? It's probably better overall to use free functions, but it makes reviewing the diffs a lot harder. I used the git show -w --reverse --color-moved=dimmed-zebra --color-moved-ws=allow-indentation-change command, but it means I can't review easily on GitHub. And then you can always open a PR after this one to move them.

@jyn514 jyn514 mentioned this pull request Feb 16, 2021
@jyn514
Copy link
Member Author

jyn514 commented Feb 16, 2021

@camelid I tried switching this to have all associated functions and got a lifetime error I don't understand:

error: lifetime may not live long enough
   --> src/librustdoc/clean/auto_trait.rs:144:9
    |
27  | impl<'a, 'tcx> AutoTraitFinder<'a, 'tcx> {
    |      --  ---- lifetime `'tcx` defined here
    |      |
    |      lifetime `'a` defined here
...
144 |         Self::region_name(region)
    |         ^^^^^^^^^^^^^^^^^ requires that `'tcx` must outlive `'a`
    |
    = help: consider adding the following bound: `'tcx: 'a`

error: lifetime may not live long enough
   --> src/librustdoc/clean/auto_trait.rs:202:24
    |
27  | impl<'a, 'tcx> AutoTraitFinder<'a, 'tcx> {
    |      --  ---- lifetime `'tcx` defined here
    |      |
    |      lifetime `'a` defined here
...
202 |                     if Self::region_name(r1) != Self::region_name(r2) {
    |                        ^^^^^^^^^^^^^^^^^ requires that `'tcx` must outlive `'a`
    |
    = help: consider adding the following bound: `'tcx: 'a`

error: aborting due to 2 previous errors

Would you be ok with keeping region_name as a free function and leaving the others as associated functions, even though it's inconsistent? i.e. just drop the last commit.

@camelid
Copy link
Member

camelid commented Feb 16, 2021

Yikes, that is an intimidating error!

Would you be ok with keeping region_name as a free function and leaving the others as associated functions, even though it's inconsistent? i.e. just drop the last commit.

Sure, sounds fine :)

@camelid
Copy link
Member

camelid commented Feb 16, 2021

Would you be ok with keeping region_name as a free function and leaving the others as associated functions, even though it's inconsistent? i.e. just drop the last commit.

Sure, sounds fine :)

Even aside from the lifetimes issues, region_name is so small and general-purpose compared to at least some of the others that it's significantly better as a free function.

@camelid camelid 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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 16, 2021
@bors
Copy link
Contributor

bors commented Feb 19, 2021

⌛ Testing commit 2bc5a0a with merge 4feaa67310cf0a309ec956cfb090eba893203fc9...

@bors
Copy link
Contributor

bors commented Feb 19, 2021

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 19, 2021
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@jyn514
Copy link
Member Author

jyn514 commented Feb 19, 2021

@bors retry

Reported that this keeps timing out, I find it hard to believe it could be related to this change: https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/i686-gnu*.20jobs.20keep.20timing.20out

@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 Feb 19, 2021
@bors
Copy link
Contributor

bors commented Feb 19, 2021

⌛ Testing commit 2bc5a0a with merge 9b471a3...

@bors
Copy link
Contributor

bors commented Feb 19, 2021

☀️ Test successful - checks-actions
Approved by: camelid,GuillaumeGomez
Pushing 9b471a3 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 19, 2021
@bors bors merged commit 9b471a3 into rust-lang:master Feb 19, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 19, 2021
@jyn514 jyn514 deleted the mut-passes branch February 19, 2021 19:40
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 9b471a3f5 Auto merge of #82020 - jyn514:mut-passes, r=camelid,GuillaumeGomez
##[group]Run src/ci/publish_toolstate.sh
src/ci/publish_toolstate.sh
env:
  SCCACHE_BUCKET: rust-lang-ci-sccache2
  DEPLOY_BUCKET: rust-lang-ci2
  TOOLSTATE_REPO: https://github.com/rust-lang-nursery/rust-toolstate
---
  CACHE_DOMAIN: ci-caches.rust-lang.org
  TOOLSTATE_REPO_ACCESS_TOKEN: ***
##[endgroup]
Cloning into 'rust-toolstate'...
/home/runner/work/rust/rust/src/tools/publish_toolstate.py:121: DeprecationWarning: 'U' mode is deprecated
📣 Toolstate changed by rust-lang/rust#82020!
  with open(path, 'rU') as f:

Tested on commit rust-lang/rust@9b471a3f5fe57e5c6e08acf665f2094422415a3d.
Direct link to PR: <https://github.com/rust-lang/rust/pull/82020>

💔 miri on windows: test-fail → build-fail (cc @RalfJung @eddyb @oli-obk).
💔 miri on linux: test-fail → build-fail (cc @RalfJung @eddyb @oli-obk).
Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/runner/work/rust/rust/src/tools/publish_toolstate.py", line 338, in <module>
    response = urllib2.urlopen(urllib2.Request(
  File "/usr/lib/python3.8/urllib/request.py", line 222, in urlopen
    return opener.open(url, data, timeout)
  File "/usr/lib/python3.8/urllib/request.py", line 522, in open
    req = meth(req)
  File "/usr/lib/python3.8/urllib/request.py", line 1281, in do_request_
    raise TypeError(msg)
TypeError: POST data should be bytes, an iterable of bytes, or a file object. It cannot be of type str.
##[error]Process completed with exit code 1.

@camelid
Copy link
Member

camelid commented Feb 19, 2021

A job failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 9b471a3f5 Auto merge of #82020 - jyn514:mut-passes, r=camelid,GuillaumeGomez
##[group]Run src/ci/publish_toolstate.sh
src/ci/publish_toolstate.sh
env:
  SCCACHE_BUCKET: rust-lang-ci-sccache2
  DEPLOY_BUCKET: rust-lang-ci2
  TOOLSTATE_REPO: https://github.com/rust-lang-nursery/rust-toolstate
---
  CACHE_DOMAIN: ci-caches.rust-lang.org
  TOOLSTATE_REPO_ACCESS_TOKEN: ***
##[endgroup]
Cloning into 'rust-toolstate'...
/home/runner/work/rust/rust/src/tools/publish_toolstate.py:121: DeprecationWarning: 'U' mode is deprecated
📣 Toolstate changed by rust-lang/rust#82020!
  with open(path, 'rU') as f:

Tested on commit rust-lang/rust@9b471a3f5fe57e5c6e08acf665f2094422415a3d.
Direct link to PR: <https://github.com/rust-lang/rust/pull/82020>

💔 miri on windows: test-fail → build-fail (cc @RalfJung @eddyb @oli-obk).
💔 miri on linux: test-fail → build-fail (cc @RalfJung @eddyb @oli-obk).
Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/runner/work/rust/rust/src/tools/publish_toolstate.py", line 338, in <module>
    response = urllib2.urlopen(urllib2.Request(
  File "/usr/lib/python3.8/urllib/request.py", line 222, in urlopen
    return opener.open(url, data, timeout)
  File "/usr/lib/python3.8/urllib/request.py", line 522, in open
    req = meth(req)
  File "/usr/lib/python3.8/urllib/request.py", line 1281, in do_request_
    raise TypeError(msg)
TypeError: POST data should be bytes, an iterable of bytes, or a file object. It cannot be of type str.
##[error]Process completed with exit code 1.

Seems like a bug in the toolstate program. cc @rust-lang/infra

@camelid
Copy link
Member

camelid commented Feb 19, 2021

Also, it seems surprising that Miri no longer builds after this PR. The only rustc change shouldn't affect Miri and should be backwards-compatible.

@jyn514
Copy link
Member Author

jyn514 commented Feb 19, 2021

@camelid this is a known issue: #82254

camelid added a commit to camelid/rust that referenced this pull request Feb 22, 2021
I left some of them so this change doesn't balloon in size and because
removing the RefCell in `DocContext.resolver` would require compiler
changes.

Thanks to `@jyn514` for making this a lot easier with rust-lang#82020!
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 23, 2021
Remove many RefCells from DocContext

I left some of them so this change doesn't balloon in size and because
removing the RefCell in `DocContext.resolver` would require compiler
changes.

Thanks to `@jyn514` for making this a lot easier with rust-lang#82020!

r? `@jyn514`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants