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

Implement coherence checks for negative trait impls #85764

Closed
wants to merge 8 commits into from

Conversation

yaahc
Copy link
Member

@yaahc yaahc commented May 27, 2021

Motivation

This change is needed as part of rust-lang/project-error-handling#3. We need to be able to define From<&str> for Box<dyn Error> in order to maintain our existing stable APIs but moving the Error trait upstream of this implementation introduces a future incompatibility error. rustc can't tell that future incompatibility is meaningless between core and std nor that we never intend to implement Error for &str. Adjusting the coherence rules to consider negative trait impls lets us add impl !Error for &str to core to make an API guarantee that we will never implement Error for &str and let's us implement the previously mentioned From impl without getting errors of it potentially overlapping with From<E> for Box<dyn Error> where E: Error.

Alternatives Considered

  • Design and Implement a feature for wider coherence boundaries that allows multiple crates to be treated like one crate for coherence purposes then use this feature on core and alloc
    • This feels like a substantial amount of work and I don't even have confidence I could come up with something and get it through the RFC process in a reasonable amount of time.
  • Add a special case exception for the future incompatibility error which ignores it when the crates involved are part of the rust distribution and thus versioned in lockstep.
    • Sounds icky, not confident people would even want this in the compiler, feels like a low budget version of the wider coherence feature

Current Status

these notes are as much for myself as for anyone else, and are just recording the state as of the last time I paused for the day on this PR

Recent convo: https://rust-lang.zulipchat.com/#narrow/stream/144729-wg-traits/topic/negative.20impls.20in.20coherence

I met up with niko to talk about the approach a bit and we sketched out how we'd implement this in rustc:

  • add a new kind of predicate that is T: !Trait
    • or extend the existing predicate with a polarity
  • teach the trait checker to prove T: !Trait if there are negative impls
    • update overlap_with_probe to check for negation before checking for overlap
.find(|o| {
    if let Some(o_neg) = negate_obligation(o) {
        // given o = `T: Trait` this produces `T: !Trait`
        if selcx.predicate_must_hold(o_neg) {
            // we can prove `T: !Trait` is true based on the impls we see
            return true;
        }
    }
    !selcx.predicate_may_hold_fatal(o)
})

Still figuring out what the second step means for now.

@rust-highfive
Copy link
Collaborator

r? @jackh726

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 27, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@yaahc yaahc force-pushed the negative-coherence branch from 5d2f0ee to 93856e5 Compare June 1, 2021 22:14
@jackh726 jackh726 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 Jun 30, 2021
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 23, 2021
…it, r=nikomatsakis

Implement coherence checks for negative trait impls

The main purpose of this PR is to be able to [move Error trait to core](rust-lang/project-error-handling#3).

This feature is necessary to handle the following from impl on box.

```rust
impl From<&str> for Box<dyn Error> { ... }
```

Without having negative traits affect coherence moving the error trait into `core` and moving that `From` impl to `alloc` will cause the from impl to no longer compiler because of a potential future incompatibility. The compiler indicates that `&str` _could_ introduce an `Error` impl in the future, and thus prevents the `From` impl in `alloc` that would cause overlap with `From<E: Error> for Box<dyn Error>`. Adding `impl !Error for &str {}` with the negative trait coherence feature will disable this error by encoding a stability guarantee that `&str` will never implement `Error`, making the `From` impl compile.

We would have this in `alloc`:

```rust
impl From<&str> for Box<dyn Error> {} // A
impl<E> From<E> for Box<dyn Error> where E: Error {} // B
```

and this in `core`:

```rust
trait Error {}
impl !Error for &str {}
```

r? `@nikomatsakis`

This PR was built on top of `@yaahc` PR rust-lang#85764.

Language team proposal: to rust-lang/lang-team#96
@spastorino
Copy link
Member

Closing this one given that #90104 is already merged.

@spastorino spastorino closed this Oct 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants