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.

Sign up for GitHub

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

Jump to bottom

Add error message for private fn #79291

Merged
merged 1 commit into from
Feb 1, 2021
Merged

Add error message for private fn #79291

merged 1 commit into from
Feb 1, 2021

Conversation

JulianKnodt
Copy link
Contributor

Attempts to add a more detailed error when a const_evaluatable fn from another scope is used inside of a scope which cannot access it.

r? @lcnr

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 22, 2020
@JulianKnodt JulianKnodt marked this pull request as ready for review December 2, 2020 21:36
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 11, 2021
@bors

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@petrochenkov
Copy link
Contributor

Could someone explain me what's happening here from the language point of view? (Or even better send a patch to https://github.com/rust-lang/rfcs/blob/master/text/2145-type-privacy.md)
I've just noticed that this PR touches type privacy which is a very sensitive piece of infrastructure.
Why aren't existing checks enough?

(Also, I don't think the diagnostic change is an improvement, the message is talking about the function type specifically, the function itself may even be marked with pub.)

@petrochenkov petrochenkov added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 30, 2021
@petrochenkov
Copy link
Contributor

r=me with #79291 (comment) addressed and commits squashed, unless @lcnr has more comments.

@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 Jan 31, 2021
@JulianKnodt
Add error message for private fn
6a03f03 
Bless tests

Update with changes from comments
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Feb 1, 2021

📌 Commit 6a03f03 has been approved by petrochenkov

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 1, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Feb 1, 2021
@JohnTitor
Rollup merge of rust-lang#79291 - JulianKnodt:ce_priv, r=petrochenkov
c96d888 
Add error message for private fn

Attempts to add a more detailed error when a `const_evaluatable` fn from another scope is used inside of a scope which cannot access it.

r? `@lcnr`
@JohnTitor JohnTitor mentioned this pull request Feb 1, 2021
Closed
henryboisdequin pushed a commit to henryboisdequin/rust that referenced this pull request Feb 1, 2021
@henrytheswimmer
Rollup merge of rust-lang#79291 - JulianKnodt:ce_priv, r=petrochenkov
3ac8a7f 
Add error message for private fn

Attempts to add a more detailed error when a `const_evaluatable` fn from another scope is used inside of a scope which cannot access it.

r? ``@lcnr``
@henryboisdequin henryboisdequin mentioned this pull request Feb 1, 2021
Closed
@jonas-schievink jonas-schievink mentioned this pull request Feb 1, 2021
Closed
@jonas-schievink jonas-schievink mentioned this pull request Feb 1, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 1, 2021
@bors
Auto merge of rust-lang#81625 - jonas-schievink:rollup-mshpp2n, r=jon…
02b85d7 
…as-schievink

Rollup of 12 pull requests

Successful merges:

 - rust-lang#78641 (Let io::copy reuse BufWriter buffers)
 - rust-lang#79291 (Add error message for private fn)
 - rust-lang#81364 (Improve `rustc_mir_build::matches` docs)
 - rust-lang#81387 (Move some tests to more reasonable directories - 3)
 - rust-lang#81463 (Rename NLL* to Nll* accordingly to C-CASE)
 - rust-lang#81504 (Suggest accessing field when appropriate)
 - rust-lang#81529 (Fix invalid camel case suggestion involving unicode idents)
 - rust-lang#81536 (Indicate both start and end of pass RSS in time-passes output)
 - rust-lang#81592 (Rustdoc UI fixes)
 - rust-lang#81594 (Avoid building LLVM just for llvm-dwp)
 - rust-lang#81598 (Fix calling convention for CRT startup)
 - rust-lang#81618 (Sync rustc_codegen_cranelift)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@oli-obk oli-obk mentioned this pull request Feb 1, 2021
Merged
@bors bors merged commit 21d0e9b into rust-lang:master Feb 1, 2021
@rustbot rustbot added this to the 1.51.0 milestone Feb 1, 2021
@JulianKnodt JulianKnodt deleted the ce_priv branch February 1, 2021 19:48
@lcnr lcnr added A-const-generics Area: const generics (parameters and arguments) F-generic_const_exprs `#![feature(generic_const_exprs)]` labels Aug 26, 2021
@jswrenn jswrenn mentioned this pull request Nov 4, 2021
Merged
@lcnr lcnr mentioned this pull request Dec 11, 2021
Closed
16 tasks
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 27, 2021
@matthiaskrgr
Rollup merge of rust-lang#90586 - jswrenn:relax-privacy-lints, r=petr…
b57a6b3 
…ochenkov

Relax priv-in-pub lint on generic bounds and where clauses of trait impls.

The priv-in-pub lint is a legacy mechanism of the compiler, supplanted by a reachability-based [type privacy](https://github.com/rust-lang/rfcs/blob/master/text/2145-type-privacy.md) analysis. This PR does **not** relax type privacy; it only relaxes the lint (as proposed by the type privacy RFC) in the case of trait impls.

## Current Behavior
On public trait impls, it's currently an **error** to have a `where` bound constraining a private type with a trait:
```rust
pub trait Trait {}
pub struct Type {}

struct Priv {}
impl Trait for Priv {}

impl Trait for Type
where
    Priv: Trait // ERROR
{}
```

...and it's a **warning** to have have a public type constrained by a private trait:
```rust
pub trait Trait {}
pub struct Type {}

pub struct Pub {}
trait Priv {}
impl Priv for Pub {}

impl Trait for Type
where
    Pub: Priv // WARNING
{}
```

This lint applies to `where` clauses in other contexts, too; e.g. on free functions:
```rust
struct Priv<T>(T);
pub trait Pub {}
impl<T: Pub> Pub for Priv<T> {}

pub fn function<T>()
where
    Priv<T>: Pub // WARNING
{}
```

**These constraints could be relaxed without issue.**

## New Behavior
This lint is relaxed for `where` clauses on trait impls, such that it's okay to have a `where` bound constraining a private type with a trait:
```rust
pub trait Trait {}
pub struct Type {}

struct Priv {}
impl Trait for Priv {}

impl Trait for Type
where
    Priv: Trait // OK
{}
```

...and it's okay to have a public type constrained by a private trait:
```rust
pub trait Trait {}
pub struct Type {}

pub struct Pub {}
trait Priv {}
impl Priv for Pub {}

impl Trait for Type
where
    Pub: Priv // OK
{}
```

## Rationale
While the priv-in-pub lint is not essential for soundness, it *can* help programmers avoid pitfalls that would make their libraries difficult to use by others. For instance, such a lint *is* useful for free functions; e.g. if a downstream crate tries to call the `function` in the previous snippet in a generic context:
```rust
fn callsite<T>()
where
    Priv<T>: Pub // ERROR: omitting this bound is a compile error, but including it is too
{
    function::<T>()
}
```
...it cannot do so without repeating `function`'s `where` bound, which we cannot do because `Priv` is out-of-scope. A lint for this case is arguably helpful.

However, this same reasoning **doesn't** hold for trait impls. To call an unconstrained method on a public trait impl with private bounds, you don't need to forward those private bounds, you can forward the public trait:
```rust
mod upstream {
    pub trait Trait {
        fn method(&self) {}
    }
    pub struct Type<T>(T);

    pub struct Pub<T>(T);
    trait Priv {}
    impl<T: Priv> Priv for Pub<T> {}

    impl<T> Trait for Type<T>
    where
        Pub<T>: Priv // WARNING
    {}
}

mod downstream {
    use super::upstream::*;

    fn function<T>(value: Type<T>)
    where
        Type<T>: Trait // <- no private deets!
    {
        value.method();
    }
}
```

**This PR only eliminates the lint on trait impls.** It leaves it intact for all other contexts, including trait definitions, inherent impls, and function definitions. It doesn't need to exist in those cases either, but I figured I'd first target a case where it's mostly pointless.

## Other Notes
- See discussion [on zulip](https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/relax.20priv-in-pub.20lint.20for.20trait.20impl.20.60where.60.20bounds/near/222458397).
- This PR effectively reverts rust-lang#79291.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcnr lcnr lcnr left review comments

@petrochenkov petrochenkov petrochenkov left review comments

Assignees

@petrochenkov petrochenkov

@lcnr lcnr

Labels
A-const-generics Area: const generics (parameters and arguments) F-generic_const_exprs `#![feature(generic_const_exprs)]` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.51.0
Development

Successfully merging this pull request may close these issues.
8 participants
@JulianKnodt @bors @rust-log-analyzer @petrochenkov @lcnr @JohnCSimon @rust-highfive @rustbot