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

Actually deprecate collections::Bound #82080

Open
bstrie opened this issue Feb 13, 2021 · 7 comments · Fixed by #82122
Open

Actually deprecate collections::Bound #82080

bstrie opened this issue Feb 13, 2021 · 7 comments · Fixed by #82122
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@bstrie
Copy link
Contributor

bstrie commented Feb 13, 2021

In f2c7917 , intrinsics::drop_in_place was supposedly deprecated in favor of ptr::drop_in_place:

#[rustc_deprecated(
    reason = "no longer an intrinsic - use `ptr::drop_in_place` directly",
    since = "1.18.0"
)]
pub use crate::ptr::drop_in_place;

However, this function does not show up as deprecated in the library docs (https://doc.rust-lang.org/nightly/std/intrinsics/fn.drop_in_place.html), nor does it trigger the deprecation warning in the following program:

fn main() {
    let x = &mut 42 as *mut i32;
    unsafe { core::intrinsics::drop_in_place(x); }
}

Presumably this is an unforeseen inability of #[rustc_deprecated] to operate on re-exports. Regardless it means that this function was never actually deprecated in practice, and it will need be deprecated for real with an updated since value.

This can be resolved by simply making intrinsics::drop_in_place into an actual function that merely wraps and calls ptr::drop_in_place, then and then applying #[rustc_deprecated] to it. Alternatively one could fix #[rustc_deprecated] to work when applied to re-exports, but that seems far more involved.


Edit: I've also discovered another example of this, which is collections::Bound:

#[rustc_deprecated(reason = "moved to `std::ops::Bound`", since = "1.26.0")]
#[doc(hidden)]
pub use crate::ops::Bound;

The problem in this case is a bit more difficult than with drop_in_place and requires a bit more care. To wit, function items aren't nominal types, so redefining intrinsics::drop_in_place as a wrapper over ptr::drop_in_place isn't a breaking change. However, redefining collections::Bound as a newtype over ops::Bound would create a new nominal type, and would be a breaking change. Furthermore, type aliases via type aren't fully at parity with "real" types, so there might be potential for breakage if one were to simply do pub type Bound = ops::Bound; (I can confirm that the #[deprecated] attribute does work on type aliases). However, I think in practice it should(?) be alright to use a type alias here (the only disparity I can think of with type aliases is that they can't be used as constructors for unit structs, but I'd like second opinions).

@bstrie bstrie added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Feb 13, 2021
@bstrie bstrie changed the title Actually deprecate intrinsics::drop_in_place Actually deprecate intrinsics::drop_in_place and collections::Bound, and possibly fix #[rustc_deprecated] not properly applying to re-exports Feb 14, 2021
@bstrie bstrie changed the title Actually deprecate intrinsics::drop_in_place and collections::Bound, and possibly fix #[rustc_deprecated] not properly applying to re-exports Actually deprecate intrinsics::drop_in_place and collections::Bound Feb 15, 2021
bstrie added a commit to bstrie/rust that referenced this issue Feb 15, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 16, 2021
Deprecate `intrinsics::drop_in_place` and `collections::Bound`, which accidentally weren't deprecated

Fixes rust-lang#82080.

I've taken the liberty of updating the `since` values to 1.52, since an unobservable deprecation isn't much of a deprecation (even the detailed release notes never bothered to mention these deprecations).

As mentioned in the issue I'm *pretty* sure that using a type alias for `Bound` is semantically equivalent to the re-export; [the reference implies](https://doc.rust-lang.org/reference/items/type-aliases.html) that type aliases only observably differ from types when used on unit structs or tuple structs, whereas `Bound` is an enum.
bors bot added a commit to rust-lang/rust-analyzer that referenced this issue Feb 17, 2021
7702: Remove use of deprecated `std::collections::Bound` r=Veykril a=bstrie

`std::collections::Bound` has been deprecated since Rust 1.26, but due to a bug (rust-lang/rust#82080) it never triggered a visible deprecation warning. Fixing this is being done in rust-lang/rust#82122 , but landing that requires rustc-analyzer to build without triggering any deprecation warnings (https://github.com/rust-lang-ci/rust/runs/1911884006#step:24:19361).

Co-authored-by: bstrie <[email protected]>
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Mar 9, 2021
Deprecate `intrinsics::drop_in_place` and `collections::Bound`, which accidentally weren't deprecated

Fixes rust-lang#82080.

I've taken the liberty of updating the `since` values to 1.52, since an unobservable deprecation isn't much of a deprecation (even the detailed release notes never bothered to mention these deprecations).

As mentioned in the issue I'm *pretty* sure that using a type alias for `Bound` is semantically equivalent to the re-export; [the reference implies](https://doc.rust-lang.org/reference/items/type-aliases.html) that type aliases only observably differ from types when used on unit structs or tuple structs, whereas `Bound` is an enum.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Mar 9, 2021
Deprecate `intrinsics::drop_in_place` and `collections::Bound`, which accidentally weren't deprecated

Fixes rust-lang#82080.

I've taken the liberty of updating the `since` values to 1.52, since an unobservable deprecation isn't much of a deprecation (even the detailed release notes never bothered to mention these deprecations).

As mentioned in the issue I'm *pretty* sure that using a type alias for `Bound` is semantically equivalent to the re-export; [the reference implies](https://doc.rust-lang.org/reference/items/type-aliases.html) that type aliases only observably differ from types when used on unit structs or tuple structs, whereas `Bound` is an enum.
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 17, 2021
Deprecate `intrinsics::drop_in_place` and `collections::Bound`, which accidentally weren't deprecated

Fixes rust-lang#82080.

I've taken the liberty of updating the `since` values to 1.52, since an unobservable deprecation isn't much of a deprecation (even the detailed release notes never bothered to mention these deprecations).

As mentioned in the issue I'm *pretty* sure that using a type alias for `Bound` is semantically equivalent to the re-export; [the reference implies](https://doc.rust-lang.org/reference/items/type-aliases.html) that type aliases only observably differ from types when used on unit structs or tuple structs, whereas `Bound` is an enum.
@bors bors closed this as completed in 49aa79e Mar 17, 2021
@bstrie
Copy link
Contributor Author

bstrie commented Mar 18, 2021

Reopening because the new deprecation of collections::Bound (but not intrinsics::drop_in_place) will need to be reverted (#83269). Basically, type aliases can't be used to import enum variants, so we either need to fix that (#83248) or we need to make it possible to deprecate re-exports (#30827) and avoid type aliases altogether.

See also https://rust-lang.zulipchat.com/#narrow/stream/219381-t-libs/topic/unavoidable.20breakage.20when.20deprecating.20an.20enum.3F .

@bstrie bstrie reopened this Mar 18, 2021
@bstrie bstrie changed the title Actually deprecate intrinsics::drop_in_place and collections::Bound Actually deprecate ~~intrinsics::drop_in_place and~~ collections::Bound Mar 18, 2021
@bstrie bstrie changed the title Actually deprecate ~~intrinsics::drop_in_place and~~ collections::Bound Actually deprecate collections::Bound Mar 18, 2021
flip1995 pushed a commit to flip1995/rust that referenced this issue Mar 25, 2021
Deprecate `intrinsics::drop_in_place` and `collections::Bound`, which accidentally weren't deprecated

Fixes rust-lang#82080.

I've taken the liberty of updating the `since` values to 1.52, since an unobservable deprecation isn't much of a deprecation (even the detailed release notes never bothered to mention these deprecations).

As mentioned in the issue I'm *pretty* sure that using a type alias for `Bound` is semantically equivalent to the re-export; [the reference implies](https://doc.rust-lang.org/reference/items/type-aliases.html) that type aliases only observably differ from types when used on unit structs or tuple structs, whereas `Bound` is an enum.
@bors bors closed this as completed in 7406c12 Mar 26, 2021
@RalfJung
Copy link
Member

Uh, I don't think this should have been closed again.

@flip1995 somehow, as part of #83480, you pushed a commit that caused this to be re-closed.

@RalfJung RalfJung reopened this Mar 26, 2021
@flip1995
Copy link
Member

Whoops, commit b62694b got synced back and forth for some reason.

@RalfJung
Copy link
Member

Oh I think I see what happened... the original commit did a lot of things, among them fixing clippy. The commit description contained "Fixes #82080". Each commit that affects clippy is synced to clippy (creating a new commit ID since all the non-clippy changes are removed from it), and then that is synced back (with the same commit ID as what it got in clippy). So now we got a fresh commit that still has "Fixes #82080" in its description.

This sounds like a general problem with the subtree approach...

@jplatte

This comment has been minimized.

@flip1995

This comment has been minimized.

@RalfJung

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
4 participants