privacy: Mark reachable but unnameable items as reachable

[petrochenkov]

Fixes #16734 and probably some other issues

This is a continuation of #29822, but the algorithm is mostly a copy of #29973, so
r? @alexcrichton or @nikomatsakis

[alexcrichton]

Thanks @petrochenkov! Knocking out <20k bugs now should get like an extra badge or something...

I'll be honest though in that I'm not really following everything that's going on here, there's quite a few branches which try to add lots of items to the reachable set. I may just not be following what's going on here, but could this perhaps add a few more tests? There's a boatload of test cases dangling off #16734, and I suspect that each addition to the reachable set in this PR corresponds to some test case which wouldn't otherwise be caught.

[petrochenkov]

Updated with more tests.

[nikomatsakis]

Does this mean that you should go back and call visit_generics too (i.e., as would have happened on line 323?) If so, maybe we should promote this special case upward somewhere?

[nikomatsakis]

Also, do we have a regression test for this scenario? (Seems like a good one). If so, you should cite it in the comment here.

[nikomatsakis]

Ah, I see this scenario is covered in the test you added.

[petrochenkov]

Does this mean that you should go back and call visit_generics too (i.e., as would have happened on line 323?)

The whole EmbargoVisitor::visit_crate runs in a loop until it saturates and nothing is updated anymore (yeah, it should probably be optimized into some DFS or BFS, but it's a separate task).
So, the worst thing can happen here is one more loop cycle in exceedingly rare case of unnameable enum with nameable variant.

[nikomatsakis]

OK, I think I would r+ modulo my question about the variant case above.

[alexcrichton]

It's been awhile since I've done much with this code, but could you help clarify what's going on here? In other places here it looks like you're checking for "is this reachable" via checking self.get(item.id).is_some(). I think that get returns the level of exported-ness of the id, right?

If that's the case, how come this checks for Public instead of leveraging self.get?

[petrochenkov]

Variants and foreign items have their own accessibility levels - they can be reexported independently from their parents enums and foreign modules. So, we have to check them with self.get(id).is_some().
pub struct fields and inherent impl items are always reachable if their parents are reachable, we could check them with self.get(id).is_some() as well, but it wouldn't make any difference.

[alexcrichton]

Ah ok, thanks for the clarification! For readability could this move to self.get as the same pattern as well?

[alexcrichton]

I guess so another thing I'm a little confused about is why two visitors are needed? It looks like the EmbargoVisitor should walk the entire AST, right? In that case, couldn't this just be part of that?

I guess to me this algorithm should look like:

1. Initialize a reachable set with all public items
2. While the set has an item in it:
   1. Walk the reachable interface of the item
   2. For each item encountered, add it to the global reachable

This is then repeated iteratively until the set is saturated. There's only one "recurse the interface" step, however, so I'm not entirely sure where this second visitor fits in?

I may and probably am missing something though! Could you help clarify?

[petrochenkov]

This PR merely extends the existing pretty inefficient implementation of EmbargoVisito which runs walk_crate several times in a loop without any working set and graph-like traversal. I plan to rewrite it eventually, but don't have time to do it right now.

Regardless of that, having two visitors with different purposes is just convenient. One big visitor walks items and recurses into them, another small and shallow visitor walks item's surfaces without going too deep. Private-in-public visitor has the same separation of concerns.

[alexcrichton]

Ok, that sounds good to me, thanks for the clarification!

[alexcrichton]

Also, thanks for expanding the test! I'm certainly much more comfortable with this now :)

[alexcrichton]

ah yeah and to clarify r=me as well, thanks again @petrochenkov!

[petrochenkov]

@alexcrichton
Updated.

[alexcrichton]

@bors: r+ fadc95e

[bors]

⌛️ Testing commit fadc95e with merge a9430a3...

[bors]

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt