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: Fix regression in impl reachability #57344

Merged
merged 3 commits into from Jan 7, 2019

Conversation

Projects
None yet
6 participants
@petrochenkov
Copy link
Contributor

petrochenkov commented Jan 4, 2019

Rollback to pre-#56878 logic of determining reachability.
reachability(impl Trait<Substs> for Type<Substs>) = reachability(Trait & Type), substs are ignored.

Fixes #57264

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 4, 2019

@rust-lang rust-lang deleted a comment from rust-highfive Jan 5, 2019

Show resolved Hide resolved src/librustc_privacy/lib.rs Outdated
@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented Jan 5, 2019

I think the invariant the impl rule tries to rely on should be documented somewhere:

The invariant is that type-inference won't let you use an impl without knowing the "shallow" version of its self type (which requires reaching the DefIds in it).

I noted this is not handled correctly with projections, so I have an example that ICEs even older rustcs:

aux.rs:

mod inner {
    #[derive(Default)]
    pub struct PubUnnameable;
}

trait Mirror { type Image; }
impl<T> Mirror for T { type Image = T; }

pub enum Pub<T> { Nothing, Just(T) }
pub trait Aux {}
impl Aux for <Pub<inner::PubUnnameable> as Mirror>::Image {}

pub fn assert_aux<T: Aux>(_t: &T) {}

impl inner::PubUnnameable {
    pub fn pub_method(self) {}
}

main.rs:

extern crate aux;

fn get_default<T: Default>(_t: &aux::Pub<T>) -> T { T::default() }

fn main() {
    let pub_ = aux::Pub::Nothing;
    aux::assert_aux(&pub_);
    let def = get_default(&pub_);
    def.pub_method();
}
@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented Jan 5, 2019

I would rename RECURSE into !SHALLOW, and make sure that the few other recursive calls in visit_ty don't occur in shallow mode. Also document the invariant.

@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented Jan 5, 2019

r=me with these changes

str4d added a commit to str4d/ire that referenced this pull request Jan 5, 2019

@petrochenkov petrochenkov force-pushed the petrochenkov:regreach branch from 94c3f88 to 46d53e5 Jan 5, 2019

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 5, 2019

@arielb1

I noted this is not handled correctly with projections, so I have an example that ICEs even older rustcs

Projections are not necessary in that case, the root reason is a public trait that's implemented for a single type, so type inference can make conclusion "T: Trait => T = ThatType".
9503c56 has a further minimized example and a fix.

@bors r=arielb1

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 5, 2019

📌 Commit 46d53e5 has been approved by arielb1

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 5, 2019

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #57230
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 5, 2019

📌 Commit 46d53e5 has been approved by arielb1

@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented Jan 5, 2019

I noted this is not handled correctly with projections, so I have an example that ICEs even older rustcs

Projections are not necessary in that case, the root reason is a public trait that's implemented for a single type, so type inference can make conclusion "T: Trait => T = ThatType".
9503c56 has a further minimized example and a fix.

Ok. And there's no inherent problem with projections because accessibility computation has to treat projections as public anyway.

@​bors r=arielb1

I would prefer you to not do a @​bors r=me after you make a change this size.

In any case:

Looking at the code, the one point that I think is missing an adequate explanation is the Ty::Opaque case where you are recurring into the contents. However, I think this is currently OK:

Treating opaque types with private predicates as private when computing the AccessLevel of impls is OK because:
1. First, I don't think we can have Ty::Opaque in the self-type of an impl, and if that becomes possible (abstract type), we'll have to rethink the privacy of pub abstract type MyType: PublicTrait + PrivateTrait anyway.
2. In any case, the privacy property we are checking is the privacy of the def id, not the privacy of the substs. If a user can get his hands on an instance of an Opaque, "inductively on the type-checking of the program" we can know that the def id of that Opaque is not private, which means that we won't mark it as private in our pass.

However, I think that is remarkable enough I'll want it as a comment.

@bors r-

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 5, 2019

📌 Commit 46d53e5 has been approved by arielb1

@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented Jan 5, 2019

@bors r-

@bors bors removed the S-waiting-on-bors label Jan 5, 2019

@arielb1

This comment has been minimized.

Copy link
Contributor

arielb1 commented Jan 5, 2019

r=me with a comment on ty::Opaque (it would be OK to copy what I wrote and put it in a comment).

(If you are only changing the comment, I'm ok with a @​bors r=arielb1).

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 5, 2019

@arielb1
From privacy point of view impl Trait1 + Trait2 should be checked identically to dyn Trait1 + Trait2, so we are not even interested in the def-id of the opaque itself (which either doesn't have attached visibility or it's insignificant, like visibility of a type alias).
pub abstract type MyType: PublicTrait + PrivateTrait is equivalent to pub type MyType = dyn PublicTrait + PrivateTrait in this sense.
It's private if any trait in the list is private.

This intent is somewhat hidden because impl-Trait and dyn-Trait use such different structures in ty and we have to recurse to get the trait list in one case, but not in another.

@petrochenkov petrochenkov force-pushed the petrochenkov:regreach branch from 46d53e5 to fb00313 Jan 6, 2019

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 6, 2019

@bors r=arielb1

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 6, 2019

📌 Commit fb00313 has been approved by arielb1

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 6, 2019

⌛️ Testing commit fb00313 with merge d39dddf...

bors added a commit that referenced this pull request Jan 6, 2019

Auto merge of #57344 - petrochenkov:regreach, r=arielb1
privacy: Fix regression in impl reachability

Rollback to pre-#56878 logic of determining reachability.
`reachability(impl Trait<Substs> for Type<Substs>) = reachability(Trait & Type)`, substs are ignored.

Fixes #57264
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 7, 2019

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing d39dddf to master...

@bors bors merged commit fb00313 into rust-lang:master Jan 7, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment