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

Impl stability is not checked #55436

Open
qnighy opened this issue Oct 28, 2018 · 9 comments
Open

Impl stability is not checked #55436

qnighy opened this issue Oct 28, 2018 · 9 comments
Labels
A-stability Area: `#[stable]`, `#[unstable]` etc. A-trait-system Area: Trait system T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@qnighy
Copy link
Contributor

qnighy commented Oct 28, 2018

Found this implementing #55431. There I want to make an unstable impl for a stable trait with stable types. We can mark impls #[unstable], but they seem to be just ignored.

#[unstable(feature = "boxed_closure_impls", ...)] // This seems to be ignored!
impl<A, F: FnOnce<A> + ?Sized> FnOnce<A> for Box<F> { ... }

A similar case is the CoerceUnsized impl for NonNull types. This is marked #[unstable] but can be used indirectly through coercion. These #[unstable] markers don't show up in rustdoc as well.

Questions:

  • Is this a bug (or an unimplemented feature)? I currently suspect that it's the case, because stability will get complicated when trait solver is involved.
  • If we implement this...
    • When should we consider them unstable? In my mind, coherence checking should always see all impls as visible. The hidden impls RFC (RFC: Hidden trait implementations rfcs#2529) may have a similar idea.
    • How should unstable traits behave with respect to type inference? I think they should be removed from candidates in winnowing if there are multiple options, and selected impls should be double-checked.
    • Do we need an RFC in this case?
@nagisa
Copy link
Member

nagisa commented Oct 28, 2018

Implementations are intentionally insta-stable. It is a long-standing, and well known, limitation.

@qnighy
Copy link
Contributor Author

qnighy commented Oct 28, 2018

Thank you!

Implementations are intentionally insta-stable. It is a long-standing, and well known, limitation.

I'd like to know the intention behind that, but I couldn't find related discussions by myself. I'll be happy if you have any references.

@petrochenkov
Copy link
Contributor

I'd like to know the intention behind that

The "intention" is that it was hard to implement, so nobody implemented it, so stability attributes on impls now used only as documentation.

@qnighy
Copy link
Contributor Author

qnighy commented Oct 28, 2018

The "intention" is that it was hard to implement, so nobody implemented it, so stability attributes on impls now used only as documentation.

Thanks! So there's no known problem with actually implementing it, but it's just (maybe) hard.

@petrochenkov
Copy link
Contributor

petrochenkov commented Oct 28, 2018

There's just so many ways to use an impl - starting from direct impl method calls, to checking that a type passed as a generic argument satisfies a bound, to conversion of a concrete type with an unstable impl into a trait object.
All this felt radically different from pretty simple things that the stability pass did (you see a path/method call/field access, get its definition, check it for stability).
IIUC, the trait system was completely rewritten since then, so perhaps this checking can be done in some more centralized way now.

@cyplo
Copy link
Contributor

cyplo commented Jan 18, 2019

Would it need an RFC or is it good for someone to just go and implement ?

@jonas-schievink
Copy link
Contributor

@cyplo I don't think it needs an RFC, but the implementation is not going to be easy. It might be better to wait for chalk integration, which could simplify this.

@jonas-schievink jonas-schievink added A-trait-system Area: Trait system A-stability Area: `#[stable]`, `#[unstable]` etc. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 27, 2019
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Sep 12, 2020
…ess-unstable-trait-impl, r=lcnr

Warn for #[unstable] on trait impls when it has no effect.

Earlier today I sent a PR with an `#[unstable]` attribute on a trait `impl`, but was informed that this attribute has no effect there. (comment: rust-lang#76525 (comment), issue: rust-lang#55436)

This PR adds a warning for this situation. Trait `impl` blocks with `#[unstable]` where both the type and the trait are stable will result in a warning:

```
warning: An `#[unstable]` annotation here has no effect. See issue rust-lang#55436 <rust-lang#55436> for more information.
   --> library/std/src/panic.rs:235:1
    |
235 | #[unstable(feature = "integer_atomics", issue = "32976")]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```

---

It detects three problems in the existing code:

1. A few `RefUnwindSafe` implementations for the atomic integer types in `library/std/src/panic.rs`. Example:
https://github.com/rust-lang/rust/blob/d92155bf6ae0b7d79fc83cbeeb0cc0c765353471/library/std/src/panic.rs#L235-L236
2. An implementation of `Error` for `LayoutErr` in `library/std/srd/error.rs`:
https://github.com/rust-lang/rust/blob/d92155bf6ae0b7d79fc83cbeeb0cc0c765353471/library/std/src/error.rs#L392-L397
3. `From` implementations for `Waker` and `RawWaker` in `library/alloc/src/task.rs`. Example:
https://github.com/rust-lang/rust/blob/d92155bf6ae0b7d79fc83cbeeb0cc0c765353471/library/alloc/src/task.rs#L36-L37

Case 3 interesting: It has a bound with an `#[unstable]` trait (`W: Wake`), so appears to have much effect on stable code. It does however break similar blanket implementations. It would also have immediate effect if `Wake` was implemented for any stable type. (Which is not the case right now, but there are no warnings in place to prevent it.) Whether this case is a problem or not is not clear to me. If it isn't, adding a simple `c.visit_generics(..);` to this PR will stop the warning for this case.
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 12, 2020
…s-unstable-trait-impl, r=lcnr

Warn for #[unstable] on trait impls when it has no effect.

Earlier today I sent a PR with an `#[unstable]` attribute on a trait `impl`, but was informed that this attribute has no effect there. (comment: rust-lang#76525 (comment), issue: rust-lang#55436)

This PR adds a warning for this situation. Trait `impl` blocks with `#[unstable]` where both the type and the trait are stable will result in a warning:

```
warning: An `#[unstable]` annotation here has no effect. See issue rust-lang#55436 <rust-lang#55436> for more information.
   --> library/std/src/panic.rs:235:1
    |
235 | #[unstable(feature = "integer_atomics", issue = "32976")]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
```

---

It detects three problems in the existing code:

1. A few `RefUnwindSafe` implementations for the atomic integer types in `library/std/src/panic.rs`. Example:
https://github.com/rust-lang/rust/blob/d92155bf6ae0b7d79fc83cbeeb0cc0c765353471/library/std/src/panic.rs#L235-L236
2. An implementation of `Error` for `LayoutErr` in `library/std/srd/error.rs`:
https://github.com/rust-lang/rust/blob/d92155bf6ae0b7d79fc83cbeeb0cc0c765353471/library/std/src/error.rs#L392-L397
3. `From` implementations for `Waker` and `RawWaker` in `library/alloc/src/task.rs`. Example:
https://github.com/rust-lang/rust/blob/d92155bf6ae0b7d79fc83cbeeb0cc0c765353471/library/alloc/src/task.rs#L36-L37

Case 3 interesting: It has a bound with an `#[unstable]` trait (`W: Wake`), so appears to have much effect on stable code. It does however break similar blanket implementations. It would also have immediate effect if `Wake` was implemented for any stable type. (Which is not the case right now, but there are no warnings in place to prevent it.) Whether this case is a problem or not is not clear to me. If it isn't, adding a simple `c.visit_generics(..);` to this PR will stop the warning for this case.
m-ou-se added a commit to m-ou-se/rust that referenced this issue Feb 7, 2022
…l, r=m-ou-se

Impl {Add,Sub,Mul,Div,Rem,BitXor,BitOr,BitAnd}Assign<$t> for Wrapping<$t> for rust 1.60.0

Tracking issue rust-lang#93204

This is about adding basic integer operations to the `Wrapping` type:

```rust
let mut value = Wrapping(2u8);
value += 3u8;
value -= 1u8;
value *= 2u8;
value /= 2u8;
value %= 2u8;
value ^= 255u8;
value |= 123u8;
value &= 2u8;
```

Because this adds stable impls on a stable type, it runs into the following issue if an `#[unstable(...)]` attribute is used:

```
an `#[unstable]` annotation here has no effect
note: see issue rust-lang#55436 <rust-lang#55436> for more information
```

This means - if I understood this correctly - the new impls have to be stabilized instantly.
Which in turn means, this PR has to kick of an FCP on the tracking issue as well?

This impl is analog to 1c0dc18 rust-lang#92356 for the `Saturating` type `@dtolnay`  `@Mark-Simulacrum`
m-ou-se added a commit to m-ou-se/rust that referenced this issue Feb 7, 2022
…l, r=m-ou-se

Impl {Add,Sub,Mul,Div,Rem,BitXor,BitOr,BitAnd}Assign<$t> for Wrapping<$t> for rust 1.60.0

Tracking issue rust-lang#93204

This is about adding basic integer operations to the `Wrapping` type:

```rust
let mut value = Wrapping(2u8);
value += 3u8;
value -= 1u8;
value *= 2u8;
value /= 2u8;
value %= 2u8;
value ^= 255u8;
value |= 123u8;
value &= 2u8;
```

Because this adds stable impls on a stable type, it runs into the following issue if an `#[unstable(...)]` attribute is used:

```
an `#[unstable]` annotation here has no effect
note: see issue rust-lang#55436 <rust-lang#55436> for more information
```

This means - if I understood this correctly - the new impls have to be stabilized instantly.
Which in turn means, this PR has to kick of an FCP on the tracking issue as well?

This impl is analog to 1c0dc18 rust-lang#92356 for the `Saturating` type ``@dtolnay``  ``@Mark-Simulacrum``
@jhpratt
Copy link
Member

jhpratt commented May 10, 2023

I believe this should be closed in favor of #39935, as the issue is largely the same.

@JohnScience
Copy link
Contributor

JohnScience commented Jun 21, 2023

Hi, I'm trying to provide feature-gated implementations of Extend<Group>, Extend<Literal>, etc, for proc_macro::TokenStream.

For that I've created a ts_extend_w_tt_item feature and right now I'm trying to do add the lines below to rust/library/proc_macro/src/lib.rs:

#[unstable(feature = "ts_extend_w_tt_item", issue = "112815")]
impl Extend<Group> for TokenStream {
    fn extend<I: IntoIterator<Item = Group>>(&mut self, groups: I) {
        self.extend(groups.into_iter().map(TokenTree::Group))
    }
}

#[unstable(feature = "ts_extend_w_tt_item", issue = "112815")]
impl Extend<Literal> for TokenStream {
    fn extend<I: IntoIterator<Item = Literal>>(&mut self, literals: I) {
        self.extend(literals.into_iter().map(TokenTree::Literal))
    }
}

#[unstable(feature = "ts_extend_w_tt_item", issue = "112815")]
impl Extend<Ident> for TokenStream {
    fn extend<I: IntoIterator<Item = Ident>>(&mut self, idents: I) {
        self.extend(idents.into_iter().map(TokenTree::Ident))
    }
}

#[unstable(feature = "ts_extend_w_tt_item", issue = "112815")]
impl Extend<Punct> for TokenStream {
    fn extend<I: IntoIterator<Item = Punct>>(&mut self, puncts: I) {
        self.extend(puncts.into_iter().map(TokenTree::Punct))
    }
}

However, I'm getting such errors:

error: an `#[unstable]` annotation here has no effect
   --> library\proc_macro\src\lib.rs:375:1
    |
375 | #[unstable(feature = "ts_extend_w_tt_item", issue = "112815")]
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
    = note: see issue #55436 <https://github.com/rust-lang/rust/issues/55436> for more information

error: could not compile `proc_macro` (lib) due to 4 previous errors

I checked this issue and https://rustc-dev-guide.rust-lang.org/stability.html but I still don't understand what to do. Should I remove the feature-gate and provide the implementations unconditionally?

Update

I received a reply in the #contribute channel of the official Discord server of Rust programming language. There, jyn (jynelson) confirmed that I need to remove the feature-gate and make the implementation insta-stable.

If I understood him correctly, it means that I should proceed with the stabilization process immediately as described in Standard library developers Guide > 4. The feature lifecycle > 4.3 Stabilizing features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-stability Area: `#[stable]`, `#[unstable]` etc. A-trait-system Area: Trait system T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants