Stream: rustdoc

Topic: getting rid of cell and refcell usage


view this post on Zulip Noah Lev (Jan 28 2021 at 22:21):

What is our plan for the usage of Cell and RefCell in DocContext? I'm guessing long-term we want to get rid of those?

view this post on Zulip Noah Lev (Jan 28 2021 at 22:21):

I know Joshua was working on this at one point.

view this post on Zulip Noah Lev (Jan 28 2021 at 22:23):

I'm starting off by trying to remove the Cell wrapper around DocContext.param_env and I'm getting 263 errors :laughter_tears:

view this post on Zulip Noah Lev (Jan 28 2021 at 22:27):

Down to 56.

view this post on Zulip Dániel Buga (Jan 28 2021 at 22:28):

I can help by removing a whole field in https://github.com/rust-lang/rust/pull/81398/files#diff-2af9d5298f727457226da68b0dc427149fc68b3aa0d0fe64cca15b63c34af997L67 :wink:

view this post on Zulip Noah Lev (Jan 28 2021 at 22:29):

Yeah, I was wondering why there were two fields rather than one...

view this post on Zulip Dániel Buga (Jan 28 2021 at 22:30):

Three if you count the global

view this post on Zulip Noah Lev (Jan 28 2021 at 22:32):

For some reason %s/cx: &DocContext/cx: &mut DocContext/g in Vim (and the equivalent in sed) is producing stuff like cx: cx: &DocContextmut DocContext<'_>. Any idea why?

view this post on Zulip Noah Lev (Jan 28 2021 at 22:32):

: and & don't seem to be metachars in this context...

view this post on Zulip Noah Lev (Jan 28 2021 at 22:33):

However, Sublime Text regex find & replace is doing it correctly...

view this post on Zulip Dániel Buga (Jan 28 2021 at 22:33):

Sorry, I can't use anything more complicated than nano

view this post on Zulip Noah Lev (Jan 28 2021 at 22:34):

You don't use Vim?

view this post on Zulip Noah Lev (Jan 28 2021 at 22:34):

(or emacs)

view this post on Zulip Dániel Buga (Jan 28 2021 at 22:34):

Nah, hard on my nerves

view this post on Zulip Noah Lev (Jan 28 2021 at 22:35):

I used to use nano before I learned how to use Vim. "Why can't I exit this thing...?"

view this post on Zulip Noah Lev (Jan 28 2021 at 22:35):

Maybe I'll try using nano for this then...

view this post on Zulip Dániel Buga (Jan 28 2021 at 22:35):

there's always the power button... anyway, I do most of my work on Windows, maybe a bit on WSL, so I wasn't ever forced to learn. Maybe I should, though.

view this post on Zulip Dániel Buga (Jan 28 2021 at 22:36):

sorry it's not the solution you're looking for :smile:

view this post on Zulip Dániel Buga (Jan 28 2021 at 22:40):

Also I think you'll find some places where the borrowchecker will be hard to please

view this post on Zulip Noah Lev (Jan 28 2021 at 23:36):

Wow, this is so hard :sweat_smile:

view this post on Zulip Noah Lev (Jan 29 2021 at 02:08):

Well at least I got #81495 along the way.

view this post on Zulip simulacrum (Jan 29 2021 at 03:08):

@Camelid you want to escape &, vim interprets that as "matched text" or something similar IIRC

view this post on Zulip Noah Lev (Jan 29 2021 at 03:28):

Huh, okay. I'll try that! Thanks for your help :)

view this post on Zulip Noah Lev (Jan 29 2021 at 03:29):

Got rid of one use of Cell :tada: #81497

view this post on Zulip Noah Lev (Jan 29 2021 at 04:01):

simulacrum said:

Camelid you want to escape &, vim interprets that as "matched text" or something similar IIRC

Yeah, it looks like you're right about that: https://stackoverflow.com/a/41508243

view this post on Zulip Joshua Nelson (Jan 29 2021 at 15:08):

Camelid said:

For some reason %s/cx: &DocContext/cx: &mut DocContext/g in Vim (and the equivalent in sed) is producing stuff like cx: cx: &DocContextmut DocContext<'_>. Any idea why?

you need to escape &: \&

view this post on Zulip Joshua Nelson (Jan 29 2021 at 15:08):

also I think I have a branch with this halfway done I linked a while back, I have so many branches I can't find it lol

view this post on Zulip Noah Lev (Jan 29 2021 at 19:35):

Yeah, you might have DM'd it to me at one point. It's probably easier to just restart though because it was a while ago.

view this post on Zulip Noah Lev (Feb 04 2021 at 20:54):

What do you all think would be the best places to start getting rid of interior mutability? These are the places that currently use Cell or RefCell:

src/librustdoc/json/mod.rs
34:    index: Rc<RefCell<FxHashMap<types::Id, types::Item>>>,

src/librustdoc/clean/types.rs
48:thread_local!(crate static MAX_DEF_IDX: RefCell<FxHashMap<CrateNum, DefIndex>> = Default::default());
60:    crate external_traits: Rc<RefCell<FxHashMap<DefId, Trait>>>,
1557:        static CELL: OnceCell<FxHashMap<PrimitiveType, ArrayVec<[DefId; 4]>>> = OnceCell::new();

src/librustdoc/passes/collect_intra_doc_links.rs
269:    kind_side_channel: Cell<Option<(DefKind, DefId)>>,

src/librustdoc/html/render/mod.rs
116:    id_map: Rc<RefCell<IdMap>>,
119:    deref_id_map: Rc<RefCell<FxHashMap<DefId, String>>>,
121:    all: Rc<RefCell<AllTypes>>,
149:    crate created_dirs: RefCell<FxHashSet<PathBuf>>,
360:thread_local!(crate static CURRENT_DEPTH: Cell<usize> = Cell::new(0));

src/librustdoc/html/format.rs
1337:struct WithFormatter<F>(Cell<Option<F>>);

src/librustdoc/core.rs
47:    crate resolver: Rc<RefCell<interface::BoxedResolver>>,
51:    crate param_env: Cell<ParamEnv<'tcx>>,
53:    crate renderinfo: RefCell<RenderInfo>,
55:    crate external_traits: Rc<RefCell<FxHashMap<DefId, clean::Trait>>>,
58:    crate active_extern_traits: RefCell<FxHashSet<DefId>>,
62:    crate ty_substs: RefCell<FxHashMap<DefId, clean::Type>>,
64:    crate lt_substs: RefCell<FxHashMap<DefId, clean::Lifetime>>,
66:    crate ct_substs: RefCell<FxHashMap<DefId, clean::Constant>>,
68:    crate impl_trait_bounds: RefCell<FxHashMap<ImplTraitParam, Vec<clean::GenericBound>>>,
69:    crate fake_def_ids: RefCell<FxHashMap<CrateNum, DefIndex>>,
72:    crate generated_synthetics: RefCell<FxHashSet<(Ty<'tcx>, DefId)>>,
80:    crate module_trait_cache: RefCell<FxHashMap<DefId, FxHashSet<DefId>>>,
425:) -> Rc<RefCell<interface::BoxedResolver>> {
462:    resolver: Rc<RefCell<interface::BoxedResolver>>,

view this post on Zulip Noah Lev (Feb 04 2021 at 20:55):

Maybe getting rid of those thread_local!s would be good since they're global mutable state?

view this post on Zulip Joshua Nelson (Feb 04 2021 at 21:07):

Cell has no performance impact so I would focus on RefCell where possible

view this post on Zulip Joshua Nelson (Feb 04 2021 at 21:08):

MAX_DEF_ID I want to get rid of but it's a giant project: basically my idea is to make a enum RustdocId { Real(DefId), Fake(usize) } so you can tell when compiler APIs will panic (rather than constantly having to guess or read the code).

view this post on Zulip Joshua Nelson (Feb 04 2021 at 21:09):

you can get rid of the Cell in WithFormatter by changing the bound from FnOnce to FnMut, but that may cause other errors

view this post on Zulip Joshua Nelson (Feb 04 2021 at 21:10):

I think you may be able to get rid of CURRENT_DEPTH by making it a field on FormatRenderer?

view this post on Zulip Joshua Nelson (Feb 04 2021 at 21:10):

none of these are easy btw

view this post on Zulip Noah Lev (Feb 04 2021 at 21:17):

Joshua Nelson said:

you can get rid of the Cell in WithFormatter by changing the bound from FnOnce to FnMut, but that may cause other errors

I don't think so; IIUC the Cell is needed because it needs to mutate inside of a Display::fmt impl.

view this post on Zulip Noah Lev (Feb 04 2021 at 21:17):

Joshua Nelson said:

MAX_DEF_ID I want to get rid of but it's a giant project: basically my idea is to make a enum RustdocId { Real(DefId), Fake(usize) } so you can tell when compiler APIs will panic (rather than constantly having to guess or read the code).

That's a good idea!

view this post on Zulip Noah Lev (Feb 04 2021 at 21:17):

Thanks for the help!

view this post on Zulip Joshua Nelson (Feb 04 2021 at 21:33):

Camelid said:

Joshua Nelson said:

you can get rid of the Cell in WithFormatter by changing the bound from FnOnce to FnMut, but that may cause other errors

I don't think so; IIUC the Cell is needed because it needs to mutate inside of a Display::fmt impl.

only because it calls take on the Option, if you have FnMut you wouldn't need the option and you could just reuse it

view this post on Zulip Joshua Nelson (Feb 04 2021 at 21:33):

maybe you would need Fn and not FnMut

view this post on Zulip Noah Lev (Feb 04 2021 at 21:46):

Yeah, you can't call a FnMut with only an & reference to it.

view this post on Zulip Noah Lev (Feb 04 2021 at 21:47):

I tried changing it to Fn, but: https://github.com/rust-lang/rust/pull/81497#discussion_r566617150 :smile:

view this post on Zulip Joshua Nelson (Feb 04 2021 at 21:49):

ah, that would do it

view this post on Zulip Joshua Nelson (Feb 04 2021 at 21:49):

I don't think it needs to change then

view this post on Zulip Joshua Nelson (Feb 04 2021 at 21:49):

maybe leave a comment

view this post on Zulip Noah Lev (Feb 04 2021 at 23:21):

What is CURRENT_DEPTH used for? It seems like it's for ../ in links?

view this post on Zulip Noah Lev (Feb 05 2021 at 00:19):

Joshua Nelson said:

also I think I have a branch with this halfway done I linked a while back, I have so many branches I can't find it lol

I can sympathize: apparently I have 157 branches in my local repo :sweat_smile: (I think most of them were merged, but I never deleted the branch locally)

view this post on Zulip Joshua Nelson (Feb 05 2021 at 01:15):

you can run git fetch -p which shows you which branches were deleted

view this post on Zulip Joshua Nelson (Feb 05 2021 at 01:15):

and then delete them locally too

view this post on Zulip Noah Lev (Feb 19 2021 at 23:39):

Would also be nice to get rid of all the thread_local!s.

view this post on Zulip Noah Lev (Feb 19 2021 at 23:39):

Oops, looks like I said that already :laughing: https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/getting.20rid.20of.20cell.20and.20refcell.20usage/near/225221075


Last updated: Oct 21 2021 at 20:33 UTC