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

Add error message for private fn #79291

Merged
merged 1 commit into from
Feb 1, 2021
Merged

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 force-pushed the ce_priv branch 7 times, most recently from 912bf03 to e12acb8 Compare December 2, 2020 21:35
@JulianKnodt JulianKnodt marked this pull request as ready for review December 2, 2020 21:36
@JulianKnodt JulianKnodt force-pushed the ce_priv branch 2 times, most recently from 3bb6317 to 387c1b9 Compare December 2, 2020 21:52
@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.

@JulianKnodt JulianKnodt force-pushed the ce_priv branch 3 times, most recently from 09bb403 to bf8224d Compare January 18, 2021 05:45
@rust-log-analyzer

This comment has been minimized.

@JulianKnodt JulianKnodt force-pushed the ce_priv branch 2 times, most recently from f86c195 to 8a19f44 Compare January 18, 2021 06:15
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@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
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
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 pushed a commit to henryboisdequin/rust that referenced this pull request Feb 1, 2021
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``
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 1, 2021
…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
@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
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 27, 2021
…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
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
Development

Successfully merging this pull request may close these issues.

8 participants