Use computed visibility in rustdoc

[inquisitivecrystal]

This PR changes librustdoc to use computed visibility instead of syntactic visibility. It was initially part of #88019, but was separated due to concerns that it might cause a regression somewhere we couldn't predict.

r? @jyn514
cc @cjgillot @petrochenkov

[jyn514]

There are a few other places in librustdoc that should be checked to see if they can use computed visibility. However, I don't understand them, so they're best left to someone who knows what they're doing.

Can you open an issue for this so it doesn't get forgotten? Ideally with links to the relevant code :) but no worries if that turns out to be a pain.

[jyn514]

This should fix the issue @petrochenkov mentioned in #88019 (comment), right, macro_export macros should now be displayed with the proper visibility? Can you add a test for that?

[inquisitivecrystal]

There are a few other places in librustdoc that should be checked to see if they can use computed visibility. However, I don't understand them, so they're best left to someone who knows what they're doing.

Can you open an issue for this so it doesn't get forgotten? Ideally with links to the relevant code :) but no worries if that turns out to be a pain.

It turns out I spoke too soon! There are fewer instances than I thought, and I should hopefully be able to fix all of them. The reason I thought they were above my paygrade was that I tried fixing one and it made tests fail, but it looks like I was just doing something wrong? It would still be good to have you check them over, of course.

This should fix the issue @petrochenkov mentioned in #88019 (comment), right, macro_export macros should now be displayed with the proper visibility? Can you add a test for that?

That already works, since I did a workaround in that PR (specifically, I checked for the macro_export directly). Do you still want me to do a test?

Oh, also, I think we need an is_public method on ty::Visibility. It seems to pop up all over the place. Should I add that here, or should that be a seperate PR?

[jyn514]

Do you still want me to do a test?

Yes, please.

Oh, also, I think we need an is_public method on ty::Visibility. It seems to pop up all over the place. Should I add that here, or should that be a seperate PR?

Here is fine.

[jyn514]

@inquisitivecrystal do you know when you'll get a chance to look at this?

[inquisitivecrystal]

@inquisitivecrystal do you know when you'll get a chance to look at this?

Sorry for dropping off the face of the earth! I've been dealing with a lot of stuff IRL.

I don't know exactly when I'll have a chance to work on this again, but I'm hoping it'll be in the next few weeks. The sooner I can find the time the happier I'll be.

[inquisitivecrystal]

Okay, first off, I apologize for taking such a long amount of time to get back to this.

Do you still want me to do a test?

Yes, please.

I've gotten myself kind of confused. (I'll also mention that most of this is drawn from notes I wrote up months ago, which probably isn't helping.)

The relevant line, as adjusted by #88019, is here:

rust/src/librustdoc/visit_ast.rs

Line 257 in c102653

let is_pub = item.vis.node.is_pub() || self.cx.tcx.has_attr(def_id, sym::macro_export);

This checks whether an AST item is public by checking its syntactic visibility. After this PR, it would instead use computed visibility. is_pub is then used in a few places:

rust/src/librustdoc/visit_ast.rs

Lines 259 to 261 in c102653

	if is_pub {
	self.store_path(item.def_id.to_def_id());
	}

rust/src/librustdoc/visit_ast.rs

Line 271 in c102653

_ if self.inlining && !is_pub => {}

rust/src/librustdoc/visit_ast.rs

Lines 287 to 293 in c102653

	if is_pub && self.inside_public_path {
	let please_inline = attrs.iter().any(|item| match item.meta_item_list() {
	Some(ref list) if item.has_name(sym::doc) => {
	list.iter().any(|i| i.has_name(sym::inline))
	}
	_ => false,
	});

The first one seems irrelevant to macros. All store_path does is write to exact_paths, which is only used for creating implementors sections. Implementors sections have nothing to do with macros. The third one also seems irrelevant, since it only concerns use declarations, though I'm less sure in that case.

The second case is applicable to macros. It looks like what it's doing is making sure that only public items are ever inlined, which makes sense, since they're the only items that can be imported via a use statement. This is backed up by the test inline_local/macro_by_example.rs. This effectively tests whether #[macro_export] macros are treated as public for inlining (they are). The test fails if I remove the #[macro_export] special casing from is_pub, though it starts working again once I switch to computed visibility.

I think this test may cover everything, but I'm not certain. Am I missing or misunderstanding anything?

[jyn514]

Your writeup is convincing to me, I think the existing test is fine :) just one question and then I think we can merge this.

[inquisitivecrystal]

For the last few days I've been having a look at expanding this to all of rustdoc, as we discussed above a while ago. I'm sorry I didn't update you on my status. I think I've almost got it working, but there's this one really tricky problem I don't know how to deal with.

rust/src/librustdoc/clean/mod.rs

Lines 1995 to 2007 in 5fa94f3

	let mut denied = !(import.vis.node.is_pub()
	|| (cx.render_options.document_private && import.vis.node.is_pub_restricted()))
	|| pub_underscore
	|| attrs.iter().any(|a| {
	a.has_name(sym::doc)
	&& match a.meta_item_list() {
	Some(l) => {
	attr::list_contains_name(&l, sym::no_inline)
	|| attr::list_contains_name(&l, sym::hidden)
	}
	None => false,
	}
	});

On that second line, it checks whether the syntactic visibility is either public or restricted. In the HIR, there are four VisibilityKinds: Public, Crate, Restricted, and Inherited. At the level of computed visibility, there are only three visibilities: Public, Restricted, and Invisible. My first attempt was to do a direct translation, where a VisibilityKind of public or restricted would be equivalent to a computed visibility of public or restricted. However, that's not accurate.

rust/src/test/rustdoc/reexports-priv.rs

Lines 50 to 53 in a49e38e

	pub mod foo {
	// @!has 'foo/foo/union.Union.html'
	use crate::reexports::Union;
	}

In the case of this test, the use declaration has a VisibilityKind of Inherited, but a computed visibility of Restricted. Note that the conversion is lossy: in this case, pub(self) use would have had a VisibilityKind of Restricted but the same computed visibility. Thus, in document private items mode, rustdoc currently documents the pub(self) use version but not the version here, despite them being equivalent in meaning. At least, if I'm understanding the situation correctly, which I very well might not be.

How should I handle this?

[jyn514]

Ugh, that logic is painful. It's trying to predict ahead of time whether the item will be stripped, but before the tcx is available and using the AST visibility.

Thus, in document private items mode, rustdoc currently documents the pub(self) use version but not the version here, despite them being equivalent in meaning.

That definitely seems like a bug, it should be consistent between the two. @GuillaumeGomez you added this in #86841 - what was your goal in sometimes not inlining here?

[jyn514]

@inquisitivecrystal I think it makes sense to land your other changes in the meantime if you want to push them :)

[GuillaumeGomez]

#86841 was because we have different macro kinds with different visibility settings. The macro 2.0 can be handled like any items whereas the old macros can't. And because of that, we have this weird handling. If it can be improved, I'm all for it! :D

[inquisitivecrystal]

#86841 was because we have different macro kinds with different visibility settings. The macro 2.0 can be handled like any items whereas the old macros can't. And because of that, we have this weird handling. If it can be improved, I'm all for it! :D

I think #88019 means they can be treated the same now. Does that open up any directions for improvement?

[jyn514]

@GuillaumeGomez I don't really understand your response; what was the goal of adding the special casing? What does it have to do with macros?

[GuillaumeGomez]

The issue fixed in #86841 was for the macro 2.0 and a small cleanup. Unless I missed something? Sorry if my explanations aren't clear... Let me try again:

We weren't rendering macros 2.0 correctly because we handled them like the old macros (their visibility too). So I simply changed how we handled them so it was working as expected.

There wasn't big changes in how we handle visibility as far as I can see but maybe I missed something?

[jyn514]

@GuillaumeGomez I'm asking specifically about this change: https://github.com/rust-lang/rust/pull/86841/files#diff-384affc1b4190940f114f3fcebbf969e7e18657a71ef9001da6b223a036687d9R2057
It seems unrelated to either macros or the rest of the PR.

[GuillaumeGomez]

Oh I see. Sorry, missed it. So for this one, the goal was to fix the display of reexported items if the "--document-private-items" option is used (it's described as is in the PR description). However in this case it seems like it's impacting all items, not just the reexported ones.

[jyn514]

visibility.is_accessible_from(parent_mod.to_def_id(), cx.tcx) does the same thing

Hmm, are you sure? That makes me worried we'll run into #64762. Can you add a test with a nested import?

[inquisitivecrystal]

visibility.is_accessible_from(parent_mod.to_def_id(), cx.tcx) does the same thing

Hmm, are you sure? That makes me worried we'll run into #64762. Can you add a test with a nested import?

Am I sure that visibility.is_accessible_from(parent_mod.to_def_id(), cx.tcx) is equivalent to vis.is_at_least(ty::Visibility::Restricted(parent_mod.to_def_id()), cx.tcx)? Yes, fairly, after reading the definition.

I'll confess I don't grasp how #64762 could affect this. Could you explain what that would look like and what I should be testing for? I'm sorry I keep having so many questions.

[jyn514]

The rustdoc changes LGTM. @petrochenkov how do you feel about the switch from vis == Visibility::Public to vis.is_pub() in the resolver?

[petrochenkov]

Mostly neutral, vis == Visibility::Public was already short enough, but I guess it's common enough to introduce a slightly shorter method instead.
r? @jyn514

[jyn514]

@bors r+

[bors]

📌 Commit 6622376 has been approved by jyn514

[jyn514]

@inquisitivecrystal thanks for working on this! :)

[nnethercote]

It was suggested that this may have caused a large regression in doc times for the webrender-wrench benchmark: #90769 (comment). Any thoughts about whether that's true?

[jyn514]

That is ... Quite a large difference, wow.

I wouldn't expect this to be the cause of all the regressions there, this shouldn't have affected get_blanket_impls or evaluate_obligation. I think it's plausible it makes clean_crate slower but that's not where most of the regressions are.

Maybe open a couple PRs reverting changes in the rollup and see which if any fix the regression?

[jyn514]

#88868 affects codegen, rolling that up seems suspicious. That or
#90727 would be my guess (altogether neither of those should have affected evaluate_obligation quite so much ...)

[jyn514]

#90361 also seems plausible.

[camelid]

Followup: IIUC, the perf regression was caused by the behavior regression in #90865. Then #91094 fixed the behavior regression, which in turn fixed the perf regression.