Make `Clean` take &mut DocContext #82020

jyn514 · 2021-02-12T07:54:40Z

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.

rust-highfive · 2021-02-12T07:54:44Z

r? @ollie27

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

jyn514 · 2021-02-12T07:55:14Z

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

camelid

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

camelid · 2021-02-15T01:54:25Z

src/librustdoc/clean/auto_trait.rs


        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();


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

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

src/librustdoc/clean/utils.rs

jyn514 · 2021-02-15T02:43:07Z

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 · 2021-02-15T03:07:54Z

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.

src/librustdoc/passes/collect_intra_doc_links.rs

src/librustdoc/passes/doc_test_lints.rs

camelid · 2021-02-15T04:37:51Z

src/librustdoc/clean/auto_trait.rs

-            let replaced = p.fold_with(&mut replacer);
-            let (orig_p, p) = (replaced, replaced.clean(cx));
-
+            let (orig_p, p) = (p, p.clean(self.cx));


This seems like a behavior change.

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

camelid · 2021-02-15T04:41:11Z

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

jyn514 · 2021-02-15T04:49:45Z

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 · 2021-02-15T04:53:34Z

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 :)

camelid · 2021-02-16T05:04:27Z

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 · 2021-02-16T05:22:57Z

@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 · 2021-02-16T05:45:05Z

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 · 2021-02-16T05:49:33Z

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.

bors · 2021-02-19T11:24:05Z

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

bors · 2021-02-19T15:39:05Z

💥 Test timed out

rust-log-analyzer · 2021-02-19T15:59:46Z

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

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

jyn514 · 2021-02-19T16:08:01Z

@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 · 2021-02-19T16:39:12Z

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

bors · 2021-02-19T19:39:50Z

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

rust-log-analyzer · 2021-02-19T19:41:06Z

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 · 2021-02-19T19:56:36Z

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 · 2021-02-19T19:59:29Z

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 · 2021-02-19T21:25:28Z

@camelid this is a known issue: #82254

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!

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`

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 assigned ollie27 Feb 12, 2021

This comment has been minimized.

Sign in to view

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

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

This comment has been minimized.

Sign in to view

jyn514 force-pushed the mut-passes branch from 0288476 to 3a07139 Compare February 12, 2021 18:56

camelid changed the title ~~Make Clean take &mut DocContext~~ Make Clean take &mut DocContext Feb 13, 2021

camelid reviewed Feb 15, 2021

View reviewed changes

jyn514 commented Feb 15, 2021

View reviewed changes

src/librustdoc/passes/collect_intra_doc_links.rs Show resolved Hide resolved

src/librustdoc/passes/collect_intra_doc_links.rs Show resolved Hide resolved

src/librustdoc/passes/doc_test_lints.rs Show resolved Hide resolved

camelid reviewed Feb 15, 2021

View reviewed changes

jyn514 mentioned this pull request Feb 15, 2021

Remove unnecessary tcx parameter to param_env_to_generics #82137

Closed

jyn514 force-pushed the mut-passes branch from 9b980f4 to 7aae98d Compare February 15, 2021 05:15

This comment has been minimized.

Sign in to view

jyn514 force-pushed the mut-passes branch from 7aae98d to 3e8f4b1 Compare February 15, 2021 05:32

jyn514 mentioned this pull request Feb 16, 2021

Shorten html::render #82088

Merged

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 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

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 added the merged-by-bors This PR was explicitly merged by bors. label Feb 19, 2021

bors merged commit 9b471a3 into rust-lang:master Feb 19, 2021

rustbot added this to the 1.52.0 milestone Feb 19, 2021

jyn514 deleted the mut-passes branch February 19, 2021 19:40

This was referenced Feb 19, 2021

Remove unnecessary self_ty parameter to get_blanket_impls #81349

Merged

Add Sized trait display when implemented on type #78181

Merged

bors mentioned this pull request Feb 19, 2021

Stabilize rustdoc::bare_urls lint #81764

Merged

camelid mentioned this pull request Feb 19, 2021

Remove many RefCells from DocContext #82305

Merged

jyn514 mentioned this pull request Feb 22, 2021

Remove the dummy cache in DocContext; delete RenderInfo #82018

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `Clean` take &mut DocContext #82020

Make `Clean` take &mut DocContext #82020

jyn514 commented Feb 12, 2021 •

edited

Loading

This comment has been minimized.

rust-highfive commented Feb 12, 2021

jyn514 commented Feb 12, 2021

This comment has been minimized.

camelid left a comment

camelid Feb 15, 2021

jyn514 Feb 15, 2021 •

edited

Loading

jyn514 commented Feb 15, 2021

jyn514 commented Feb 15, 2021

camelid Feb 15, 2021

jyn514 Feb 15, 2021

camelid commented Feb 15, 2021

jyn514 commented Feb 15, 2021

camelid commented Feb 15, 2021

This comment has been minimized.

camelid commented Feb 16, 2021

jyn514 commented Feb 16, 2021 •

edited

Loading

camelid commented Feb 16, 2021

camelid commented Feb 16, 2021

bors commented Feb 19, 2021

bors commented Feb 19, 2021

rust-log-analyzer commented Feb 19, 2021

jyn514 commented Feb 19, 2021

bors commented Feb 19, 2021

bors commented Feb 19, 2021

rust-log-analyzer commented Feb 19, 2021

camelid commented Feb 19, 2021

camelid commented Feb 19, 2021

jyn514 commented Feb 19, 2021 •

edited

Loading

Make Clean take &mut DocContext #82020

Make Clean take &mut DocContext #82020

Conversation

jyn514 commented Feb 12, 2021 • edited Loading

This comment has been minimized.

rust-highfive commented Feb 12, 2021

jyn514 commented Feb 12, 2021

This comment has been minimized.

camelid left a comment

Choose a reason for hiding this comment

camelid Feb 15, 2021

Choose a reason for hiding this comment

jyn514 Feb 15, 2021 • edited Loading

Choose a reason for hiding this comment

jyn514 commented Feb 15, 2021

jyn514 commented Feb 15, 2021

camelid Feb 15, 2021

Choose a reason for hiding this comment

jyn514 Feb 15, 2021

Choose a reason for hiding this comment

camelid commented Feb 15, 2021

jyn514 commented Feb 15, 2021

camelid commented Feb 15, 2021

This comment has been minimized.

camelid commented Feb 16, 2021

jyn514 commented Feb 16, 2021 • edited Loading

camelid commented Feb 16, 2021

camelid commented Feb 16, 2021

bors commented Feb 19, 2021

bors commented Feb 19, 2021

rust-log-analyzer commented Feb 19, 2021

jyn514 commented Feb 19, 2021

bors commented Feb 19, 2021

bors commented Feb 19, 2021

rust-log-analyzer commented Feb 19, 2021

camelid commented Feb 19, 2021

camelid commented Feb 19, 2021

jyn514 commented Feb 19, 2021 • edited Loading

Make `Clean` take &mut DocContext #82020

Make `Clean` take &mut DocContext #82020

jyn514 commented Feb 12, 2021 •

edited

Loading

jyn514 Feb 15, 2021 •

edited

Loading

jyn514 commented Feb 16, 2021 •

edited

Loading

jyn514 commented Feb 19, 2021 •

edited

Loading