Skip to content

Remove RefCell from JsonRenderer.index #82354

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Remove RefCell from JsonRenderer.index #82354

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions src/librustdoc/json/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

mod conversions;

use std::cell::RefCell;
use std::fs::File;
use std::path::PathBuf;
use std::rc::Rc;
Expand All @@ -31,7 +30,7 @@ crate struct JsonRenderer<'tcx> {
tcx: TyCtxt<'tcx>,
/// A mapping of IDs that contains all local items for this crate which gets output as a top
/// level field of the JSON blob.
index: Rc<RefCell<FxHashMap<types::Id, types::Item>>>,
index: FxHashMap<types::Id, types::Item>,
/// The directory where the blob will be written to.
out_path: PathBuf,
cache: Rc<Cache>,
Expand Down Expand Up @@ -141,7 +140,7 @@ impl<'tcx> FormatRenderer<'tcx> for JsonRenderer<'tcx> {
Ok((
JsonRenderer {
tcx,
index: Rc::new(RefCell::new(FxHashMap::default())),
index: FxHashMap::default(),
out_path: options.output,
cache: Rc::new(cache),
},
Expand All @@ -165,7 +164,7 @@ impl<'tcx> FormatRenderer<'tcx> for JsonRenderer<'tcx> {
} else if let types::ItemEnum::EnumItem(ref mut e) = new_item.inner {
e.impls = self.get_impls(id)
}
let removed = self.index.borrow_mut().insert(from_def_id(id), new_item.clone());
let removed = self.index.insert(from_def_id(id), new_item.clone());
// FIXME(adotinthevoid): Currently, the index is duplicated. This is a sanity check
// to make sure the items are unique.
if let Some(old_item) = removed {
Expand Down Expand Up @@ -203,7 +202,7 @@ impl<'tcx> FormatRenderer<'tcx> for JsonRenderer<'tcx> {
_diag: &rustc_errors::Handler,
) -> Result<(), Error> {
debug!("Done with crate");
let mut index = (*self.index).clone().into_inner();
let mut index = self.index.clone();
Comment on lines -206 to +205
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not fully sure the behavior is the same here. It seems like before it was moving out of an Rc, which I didn't think was possible, so I suspect I'm misunderstanding what this is doing.

I wonder if we could avoid the clone altogether and just push directly into self.index, but I don't know this code, so that may break stuff.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, looks like I did change the behavior, because every test is panicking 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't move outside the Rc, it just forces to dereference it and make it so that clone is called on the RefCell instead of the Rc. This code should have kept its meaning.

What changed however is that now cloneing a JsonRenderer will create a totally separate index. In particular the insert in the item method will have effect only on self's index while before it affected any JsonRenderer with the same origin.

Copy link
Member

Choose a reason for hiding this comment

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

The only way for this to work is to remove all calls to clone(). If you get that to work I would expect it to have the same behavior as before.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the explanation! Probably this should wait then on #82356.

index.extend(self.get_trait_items());
// This needs to be the default HashMap for compatibility with the public interface for
// rustdoc-json
Expand Down