regression: Rustdoc shows pub(self) as pub(in a::b)

[jyn514]

This is a pre-emptive bug report for when #77820 lands.

mod a {
  mod b {
        pub(self) struct FooInSelfSuperB;
  }
}

I expected to see this happen: Rustdoc displays the privacy as written: pub(self) struct FooInSelfSuperB;

Instead, this happened: Rustdoc displays an absolute path: pub(in a::b) struct FooInSelfSuperB;

A similar bug exists for pub(super). The regressions for other paths (pub(super::super) -> pub(crate), pub(in crate::a::b) -> pub(in a::b), etc.) are explicitly not in scope and I don't plan to fix them.

Note that this always requires using --document-private-items.

Implementation Notes

This is not trivial to fix because html::render does not have access to a TyCtxt and impl Clean for ty::Visibility does not have access to the original DefId of the item being documented. impl Clean need to be turned into a free function, and #77820 (comment) needs to be debugged. See jyn514@b997e4f for partial progress on that; feel free to take over that branch if you're interested in fixing this.

Meta

This version of rustdoc has not yet landed; see #77820.

[camelid]

Assigning P-medium and removing I-prioritize as discussed in the prioritization working group.

[camelid]

Is there any we can just access the original source code using a span? I guess the downside of that is there might be weird formatting, though we could just strip whitespace. Not sure if that's considered too hacky though.

[jyn514]

I would prefer not to do this by mangling the source code itself, rustdoc should not be parsing source code twice. The way to get the original behavior back is to get the visibility from the HIR instead of from metadata, but that means both a slowdown (because it will no longer be a query) and more code overall because all code will be penalized for the rare case of restricted visibility.

[camelid]

Note from #80101: Another way this issue manifests is that private functions are shown as pub(in foo) where foo is the module they're defined in.

[jyn514]

This is not trivial to fix because html::render does not have access to a TyCtxt

Since #80090 this can be done in render directly.

[camelid]

@jyn514 I feel a bit confused: it seems that there are two impl Cleans, one for hir::Visibility and ty::Visibility. What's the difference between those, and which one needs to be changed or do both? It seems like the impl for hir::Visibility is used in 5 places, while the impl for ty::Visibility is used in 2 (I got this information by removing one and then running ./x.py check and then the same for the other).

[camelid]

Actually it seems like the part that needs to be changed is clean::Visibility::print_with_space? Or should I be adding more variants to clean::Visibility for pub(self), pub(super), etc.? Probably adding more variants would make the code worse, so I'm guessing I shouldn't do that.

[jyn514]

@camelid yes, I think the changing print_with_space is the best approach now that there's a TyCtxt in render; that's the main reason I didn't fix this before. Basically change this match to check for more cases:

rust/src/librustdoc/html/format.rs

Lines 1094 to 1096 in c34c015

	clean::Visibility::Restricted(did) if did.index == CRATE_DEF_INDEX => {
	write!(f, "pub(crate) ")
	}

The reason there's two different Clean impls is because both are necessary; see #77820 (comment). If you can add more comments in the code itself that would be great :)

[camelid]

This regression is now on beta. Nominated the PR for beta backport.