Skip to content

Add error message for private fn#79291

Merged
bors merged 1 commit intorust-lang:masterfrom
JulianKnodt:ce_priv
Feb 1, 2021
Merged

Add error message for private fn#79291
bors merged 1 commit intorust-lang:masterfrom
JulianKnodt:ce_priv

Conversation

@JulianKnodt
Copy link
Contributor

@JulianKnodt JulianKnodt commented Nov 22, 2020

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
lcnr
lcnr reviewed Dec 3, 2020
View reviewed changes
JulianKnodt
JulianKnodt commented Dec 4, 2020
View reviewed changes
@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
@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.

Sign in to view
@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.

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

petrochenkov commented Jan 18, 2021

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
petrochenkov reviewed Jan 31, 2021
View reviewed changes
@petrochenkov
Copy link
Contributor

petrochenkov commented Jan 31, 2021

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

petrochenkov commented Feb 1, 2021

@bors r+

@bors
Copy link
Collaborator

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

@petrochenkov petrochenkov petrochenkov left review comments

@lcnr lcnr lcnr 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