Skip to content
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

privacy: no nominal visibility for assoc fns #114099

Conversation

davidtwco
Copy link
Member

@davidtwco davidtwco commented Jul 26, 2023

Fixes #113860.

When staged_api is enabled, effective visibilities are computed earlier and this can trigger an ICE in some cases.

In particular, if a impl of a trait method has a visibility then an error will be reported for that, but when privacy invariants are being checked, the effective visibility will still be greater than the nominal visbility and that will trigger a span_bug!.

However, this invariant - that effective visibilites are limited to nominal visibility - doesn't make sense for associated functions.

@rustbot
Copy link
Collaborator

rustbot commented Jul 26, 2023

r? @fee1-dead

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 26, 2023
@petrochenkov
Copy link
Contributor

I'd like too look in more detail why the invariant is broken in this scenario, maybe it can affect non-erroneous cases too.
r? @petrochenkov

@rustbot rustbot assigned petrochenkov and unassigned fee1-dead Jul 26, 2023
@petrochenkov
Copy link
Contributor

Minimized reproduction:

#![feature(staged_api)]

pub trait Trait {
    fn fun() {}
}

impl Trait for u8 {
    pub(self) fn fun() {}
}

fn main() {}

When staged_api is enabled, effective visibilities are computed for a greater number of items

That shouldn't be the case, from what I remember the effective_visibilities query just runs at a different (earlier) point in compilation in this case.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 27, 2023
@davidtwco davidtwco force-pushed the issue-113860-staged-api-effective-vis-gt-nominal-vis-when-trait-method-vis branch from 6860108 to 422cae7 Compare July 28, 2023 12:13
@davidtwco davidtwco changed the title privacy: span_bug! -> delay_span_bug privacy: no nominal visibility for assoc fns Jul 28, 2023
tests/ui/privacy/issue-113860-1.rs Outdated Show resolved Hide resolved
tests/ui/privacy/issue-113860.rs Outdated Show resolved Hide resolved
When `staged_api` is enabled, effective visibilities are computed earlier
and this can trigger an ICE in some cases.

In particular, if a impl of a trait method has a visibility then an error
will be reported for that, but when privacy invariants are being checked,
the effective visibility will still be greater than the nominal visbility
and that will trigger a `span_bug!`.

However, this invariant - that effective visibilites are limited to
nominal visibility - doesn't make sense for associated functions.

Signed-off-by: David Wood <[email protected]>
@davidtwco davidtwco force-pushed the issue-113860-staged-api-effective-vis-gt-nominal-vis-when-trait-method-vis branch from 422cae7 to e051a32 Compare July 28, 2023 13:28
@davidtwco davidtwco added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 28, 2023
@petrochenkov
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 28, 2023

📌 Commit e051a32 has been approved by petrochenkov

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 28, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 28, 2023
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#114099 (privacy: no nominal visibility for assoc fns )
 - rust-lang#114128 (When flushing delayed span bugs, write to the ICE dump file even if it doesn't exist)
 - rust-lang#114138 (Adjust spans correctly for fn -> method suggestion)
 - rust-lang#114146 (Skip reporting item name when checking RPITIT GAT's associated type bounds hold)
 - rust-lang#114147 (Insert RPITITs that were shadowed by missing ADTs that resolve to [type error])
 - rust-lang#114155 (Replace a lazy `RefCell<Option<T>>` with `OnceCell<T>`)
 - rust-lang#114164 (Add regression test for `--cap-lints allow` and trait bounds warning)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a1fb861 into rust-lang:master Jul 29, 2023
11 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Jul 29, 2023
@davidtwco davidtwco deleted the issue-113860-staged-api-effective-vis-gt-nominal-vis-when-trait-method-vis branch July 31, 2023 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: DefId() reachable Restricted(DefId()) > nominal Restricted(DefId())
5 participants