Check paths for privacy as they are resolved and refactor away LastPrivate

[jseyfried]

This PR privacy checks paths as they are resolved instead of in librustc_privacy (fixes #12334 and fixes #31779). This removes the need for the LastPrivate system introduced in PR #9735, the limitations of which cause #31779.

This PR also reports privacy violations in paths to intra- and inter-crate items the same way -- it always reports the first inaccessible segment of the path.

Since it fixes #31779, this is a [breaking-change]. For example, the following code would break:

mod foo {
    pub use foo::bar::S;
    mod bar { // `bar` should be private to `foo`
        pub struct S;
    }
}

impl foo::S {
    fn f() {}
}

fn main() {
    foo::bar::S::f(); // This is now a privacy error
}

r? @alexcrichton

[jseyfried]

cc @nrc @petrochenkov @nikomatsakis

[petrochenkov]

Reviewed.

[alexcrichton]

Thanks for the PR @jseyfried! I may unfortunately not be the best reviewer for this as so much of resolve and privacy have changed since this was originally implemented, but I can at least add my 2cents!

Historically privacy and resolve were entangled, but this ended up leading to tons of really weird resolution problems. The major motivation for moving all of privacy to its own separate pass was to improve all these errors. At that point, the only privacy-related parts of resolve were that glob imports didn't bring private names into a module.

Since then, though, maybe resolve has changed to be much more robust? I've definitely seen some awesome work of yours fly by in the interim :).

I suspect that the buggy program you've linked here can possibly be fixed by not jettisoning LastPrivate but patching up support for it. It looks like this approach may be cleaner, though? It at least has a negative diffstat!

[alexcrichton]

Looking over the tests, it looks like the error messages related to privacy get even better than where they are today. Specifically the messages seem much more precise in terms of what's actually private along the chain of a path.

With that in mind, if you're confident that this fits nicely into resolve as-is today and it won't regress resolution error messages, then it seems like a fine shift to me! I forget now what the rustc_privacy pass would actually do if this is removed, but it seems large enough that it surely does something.

[jseyfried]

@alexcrichton Thanks for the history and feedback! Resolve is indeed more robust now :)

I'm confident that this PR won't regress resolution error messages since it doesn't change the observable behavior of the import resolver or crate resolver; instead, it silently collects privacy errors during resolution and reports them after resolution is complete.

rustc_privacy still does a couple of passes that are unaffected by this PR, including checking for public types in private interfaces. The pass that used to privacy check paths, PrivacyChecker, still privacy checks struct fields (including the sometimes implicit fields in struct constructors) and non-UFCS methods. Some field checking can be moved into resolve and the rest can be moved into typeck, but that is for another PR.

[jseyfried]

I may unfortunately not be the best reviewer for this

r? @nikomatsakis

[jseyfried]

I added a commit addressing @petrochenkov's comments (and a comment of mine).

[gereeter]

Since privacy_errors is a HashMap, this will result in nondeterministic error ordering. Maybe the map should be changed to an FnvHashMap or a BTreeMap?

[jseyfried]

Good point, I'll change it.

[bors]

☔ The latest upstream changes (presumably #31882) made this pull request unmergeable. Please resolve the merge conflicts.

[petrochenkov]

I've added a couple more comments since you are going to rebase this anyway.

[nikomatsakis]

This seems like a big improvement. r=me.

[nikomatsakis]

To be clear: r=me after rebase + nits that others have already listed are addressed.

[jseyfried]

@nikomatsakis I rebased, addressed the comments, and cleaned up the history. Given that there was breakage in rustc_metadata, do you think this needs a crater run?

[nikomatsakis]

@jseyfried unclear, but I kicked off one just in case

[nikomatsakis]

The crater run somehow failed. I'm inclined to just land this, since the next beta has branched.

[nikomatsakis]

@bors r+

[bors]

📌 Commit e67590b has been approved by nikomatsakis

[bors]

⌛ Testing commit e67590b with merge 7cee8b9...

[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