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

negative impls integrated into coherence #96

Closed
nikomatsakis opened this issue Jun 1, 2021 · 11 comments
Closed

negative impls integrated into coherence #96

nikomatsakis opened this issue Jun 1, 2021 · 11 comments
Labels
final-comment-period The FCP has started, most (if not all) team members are in agreement major-change Major change proposal T-lang

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jun 1, 2021

Proposal

Summary and problem statement

@yaahc and I have been investigating the possibility of integrating 'negative impls' into coherence. The precise motivation is to enable

impl !Error for &str { }

and then, in a downstream crate, support

impl From<&str> for Box<dyn Error>> { }
impl<E> From<E> for Box<dyn Error>>
where
    E: Error, { }

Note that this would not permit where T: !Trait syntax, although we could in principle support that, though we would have to be careful about what it means (in particular, it would be relatively easy to support if what it means is "there is a negative impl").

Motivation, use-cases, and solution sketches

  • Supporting the project group's work, as described above

Prioritization

Fits under error handling, and is also a kind of core enabler likely to arise elsewhere

Links and related work

Initial people involved

nikomatsakis, yaahc

What happens now?

This issue is part of the experimental MCP process described in RFC 2936. Once this issue is filed, a Zulip topic will be opened for discussion, and the lang-team will review open MCPs in its weekly triage meetings. You should receive feedback within a week or two.

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@nikomatsakis nikomatsakis added major-change Major change proposal T-lang labels Jun 1, 2021
@rustbot
Copy link
Collaborator

rustbot commented Jun 1, 2021

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@rustbot rustbot added the to-announce Not yet announced MCP proposals label Jun 1, 2021
@scottmcm
Copy link
Member

scottmcm commented Jun 1, 2021

This sounds great!

Related to the !Error for str, it would be cool if this also let us impl !FnOnce for str, instead of needing this #[fundamental]:
https://github.com/rust-lang/rust/blob/7f9ab0300cd66f6f616e03ea90b2d71af474bf28/library/core/src/ops/function.rs#L65-L67

@nikomatsakis
Copy link
Contributor Author

Ah, interesting, I hadn't considered the idea that we might use it to remove some fundamental, even if we can't remove them all. Yes, that would be good.

@scottmcm
Copy link
Member

scottmcm commented Jun 8, 2021

I don't know if this works or is needed here, but let's find out

@rustbot second

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Jun 8, 2021
@Mark-Simulacrum Mark-Simulacrum removed the to-announce Not yet announced MCP proposals label Jul 6, 2021
@nikomatsakis
Copy link
Contributor Author

We agreed to let this work proceed. I will create the relevant tracking issue.

@nikomatsakis
Copy link
Contributor Author

However, I'd like to discuss this as part of tomorrow's planning meeting. We would need a liaison, I think, and I'm not sure that @yaahc can realistically be the owner here (I think it is realistically me).

@pnkfelix
Copy link
Member

(I'm willing to be liason. We should revisit official assignment of liason in future lang team meeting; today's only has two team members present.)

bors added a commit to rust-lang-ci/rust that referenced this issue 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
@yoshuawuyts
Copy link
Member

As an experiment I implemented async fn main in the compiler using this feature (branch). This uses the following negative trait bounds to allow us to impl Termination for Future<T>:

impl !Future for ! {}
impl !Future for () {}
impl<E: ?Sized> !Future for Result<!, E> {}
impl<E: ?Sized> !Future for Result<(), E> {}

There was figuring out to do regarding dyn traits which might need to be improved; but overall this feature has been incredibly useful and is a missing piece to enable async fn main to be implemented.

@nikomatsakis
Copy link
Contributor Author

This is basically all set but we just need to create the real tracking issue. @pnkfelix will serve as liaison. There is a repository here for those interested in more details:

https://rust-lang.github.io/negative-impls-initiative/

@yaahc
Copy link
Member

yaahc commented Dec 14, 2021

This is basically all set but we just need to create the real tracking issue. @pnkfelix will serve as liaison. There is a repository here for those interested in more details:

https://rust-lang.github.io/negative-impls-initiative/

I couldn't find a link to the repo in the mdbook you linked but was able to guess it from the url: https://github.com/rust-lang/negative-impls-initiative

@nikomatsakis
Copy link
Contributor Author

Closing in favor of https://github.com/rust-lang/lang-team/issues/136

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period The FCP has started, most (if not all) team members are in agreement major-change Major change proposal T-lang
Projects
None yet
Development

No branches or pull requests

7 participants