-
Notifications
You must be signed in to change notification settings - Fork 12.2k
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
[EXPERIMENT] rustdoc: Cleanup external_traits
#90365
Conversation
This is preparation for cleaning up `external_traits`.
{ | ||
let external_traits = { std::mem::take(&mut *c.external_traits.borrow_mut()) }; | ||
for (k, mut v) in external_traits { | ||
v.trait_.items = | ||
v.trait_.items.into_iter().filter_map(|i| self.fold_item(i)).collect(); | ||
c.external_traits.borrow_mut().insert(k, v); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the only part of the change that could break something. Locally, 2 tests failed, but with strange found crate
std compiled by an incompatible version of rustc
errors that may be unrelated. We'll see if CI passes or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this altogether does not seem right, you're no longer running any passes on associated trait items at all. Try keeping the same logic from before but just removing the borrow_mut().
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll try that. If it works, it'll at least be incremental progress over the current situation :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, just removing the borrow_mut
won't work. The main purpose of this change is to stop storing external_traits in Crate
, and thus allow it to not be a Rc<RefCell<...>>
. What change are you suggesting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have access to a DocContext from within this method? If not, you can change DocFolder to add a method returning a reference to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see what you mean. That's an interesting idea, I'll see if that's possible.
The job Click to see the possible cause of the failure (guessed by this bot)
|
@@ -494,7 +495,8 @@ crate fn run_global_ctxt( | |||
|
|||
let render_options = ctxt.render_options; | |||
let mut cache = ctxt.cache; | |||
krate = tcx.sess.time("create_format_cache", || cache.populate(krate, tcx, &render_options)); | |||
cache.traits = ctxt.external_traits; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this implies ctxt.cache
and ctxt.external_traits
don't need to be separate fields, right? Could we store them in the cache to start so it's less stateful? Right now this depends on the field being assigned just before populate
and nowhere else for correctness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try that.
tcx: TyCtxt<'_>, | ||
mut krate: clean::Crate, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: there's no need to reorder these parameters
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, though generally we pass tcx
as the first argument. I can revert this change if you'd prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't spotted much of a pattern for this; I would rather keep the old order to reduce churn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'll change it back then.
{ | ||
let external_traits = { std::mem::take(&mut *c.external_traits.borrow_mut()) }; | ||
for (k, mut v) in external_traits { | ||
v.trait_.items = | ||
v.trait_.items.into_iter().filter_map(|i| self.fold_item(i)).collect(); | ||
c.external_traits.borrow_mut().insert(k, v); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing this altogether does not seem right, you're no longer running any passes on associated trait items at all. Try keeping the same logic from before but just removing the borrow_mut().
☔ The latest upstream changes (presumably #90391) made this pull request unmergeable. Please resolve the merge conflicts. |
I'm going to close this for now since it's not the right approach. |
@camelid did you get a chance to try out #90365 (comment) ? I think with that change this would be the right approach :) |
Yeah, I did. The issue is that it causes multiple mutable borrows of I don't think there's a good way to solve that problem without coming up with a different approach. |
r? @ghost
cc @jyn514