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.

Sign up for GitHub

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

Jump to bottom

Stabilize const for integer {to,from}_{be,le,ne}_bytes methods #69373

Merged
merged 2 commits into from
Mar 11, 2020
Merged

Stabilize const for integer {to,from}_{be,le,ne}_bytes methods #69373

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d15a98b
Stabilize const for integer {to,from}_{be,le,ne}_bytes methods
tspiteri Feb 22, 2020
87f0dc6
use unions instead of transmute and add const safety comments
tspiteri Feb 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/libcore/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,6 @@
#![feature(rtm_target_feature)]
#![feature(f16c_target_feature)]
#![feature(hexagon_target_feature)]
#![feature(const_int_conversion)]
#![feature(const_transmute)]
#![feature(structural_match)]
#![feature(abi_unadjusted)]
Expand Down
64 changes: 48 additions & 16 deletions src/libcore/num/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2195,7 +2195,7 @@ let bytes = ", $swap_op, stringify!($SelfT), ".to_be_bytes();
assert_eq!(bytes, ", $be_bytes, ");
```"),
#[stable(feature = "int_to_from_bytes", since = "1.32.0")]
#[rustc_const_unstable(feature = "const_int_conversion", issue = "53718")]
#[rustc_const_stable(feature = "const_int_conversion", since = "1.43.0")]
#[inline]
pub const fn to_be_bytes(self) -> [u8; mem::size_of::<Self>()] {
self.to_be().to_ne_bytes()
Expand All @@ -2215,7 +2215,7 @@ let bytes = ", $swap_op, stringify!($SelfT), ".to_le_bytes();
assert_eq!(bytes, ", $le_bytes, ");
```"),
#[stable(feature = "int_to_from_bytes", since = "1.32.0")]
#[rustc_const_unstable(feature = "const_int_conversion", issue = "53718")]
#[rustc_const_stable(feature = "const_int_conversion", since = "1.43.0")]
#[inline]
pub const fn to_le_bytes(self) -> [u8; mem::size_of::<Self>()] {
self.to_le().to_ne_bytes()
Expand Down Expand Up @@ -2250,12 +2250,20 @@ assert_eq!(
);
```"),
#[stable(feature = "int_to_from_bytes", since = "1.32.0")]
#[rustc_const_unstable(feature = "const_int_conversion", issue = "53718")]
#[rustc_const_stable(feature = "const_int_conversion", since = "1.43.0")]
// SAFETY: const sound because integers are plain old datatypes so we can always
// transmute them to arrays of bytes
#[allow_internal_unstable(const_fn_union)]
#[inline]
pub const fn to_ne_bytes(self) -> [u8; mem::size_of::<Self>()] {
#[repr(C)]
union Bytes {
val: $SelfT,
bytes: [u8; mem::size_of::<$SelfT>()],
}
// SAFETY: integers are plain old datatypes so we can always transmute them to
Centril marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use the union based implementation as done in the aforementioned PR instead of the transmute based one with the macro, as that seems more hacky.

Copy link
Member

@kennytm kennytm Feb 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Centril This code doesn't compile. Transmute or not you'll either need that hack macro or fix #[allow_internal_unstable].

#[allow_internal_unstable(const_fn)] // <-- no effect
const fn h(a: u64) -> [u8; 8] {
    union U {
        a: u64,
        b: [u8; 8],
    }
    let u = U { a };
    unsafe {
        u.b //~ ERROR [E0723]: accessing union fields is unstable
    }
}

(Also, a union doesn't check the type size. Using a union is equivalent to doing transmute_copy which is more dangerous than transmute)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The macro doesn't do the same thing as #[allow_internal_unstable] on a const fn, at all.

The const fn is a different feature that's reusing the attribute and given how confused I was by it, it should be using a different attribute.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whether "fixing #[allow_internal_unstable]" or "create another attribute for const fn" requires changing the compiler and blocks this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, changing the attribute's name to not be confusing won't make it work for non-language features anyway.

My point was that there's no reason to use a macro, because it's the same as adding the feature at the crate level, just unnecessarily complicated.
It doesn't have the special semantics @oli-obk added to the attribute when applied to const fn, it's just a free pass.

Overall I am pretty confused at how this system is supposed to work and I don't think I want to merge this PR until we hear from @oli-obk.

Also, we don't need to wait for a change to the attribute to trickle into beta because we can always #![cfg_attr(bootstrap, feature(const_transmute))] or w/e.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least to someone foreign with the details like me, that sounds a lot like "controlling which unstable features a macro exposes to stable code".

  1. the macro could very well be unstable, that changes nothing (unlike the const fn situation which is only relevant for "stably const fn")
  2. it's not for control, it's out of necessity, we don't have the necessary hygiene information to be able to do this without the attribute
  3. the feature list in the attribute is a recent development, before that we wouldn't have thought twice about throwing the attribute out if we had gotten stability hygiene

If anything this probably shows we might want #![feature(...)] on arbitrary scopes, not just crate-level, and then you can lint against const_* features being applied at a scope larger than an individual const fn, if you want to.

const fn definitions then wouldn't need a separate attribute, since the tight feature-gate scoping would just be a general feature at that point that they can just use instead of something special for them.

While macro_rules! macros would continue to require an attribute, although it might make sense to have #[feature(foo)] #[rustc_inherit_unstable] instead of #[allow_internal_unstable(foo)].

Anyway this is all just speculation, if nothing else, I want to use a different name that's clearly about const(fn) to make it searchable and more of a warning sign.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine just naming it #[rustc_allow_const_feature(foo)], but the reason I reused the existing attribute was because it seems like the same thing to me. There's some thing callable by users (stably or unstably) that uses an unstable feature inside and we don't want the user to have to specify the feature. It's slightly better isolated in the const fn case than in the macro case, but it's still essentially the same logic to me.

Anyway, as said at the start of this post, I'm fine renaming it. Especially since people are confused by const fn having the same attribute as macro_rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I converted from transmute to union as suggested by @Centril, and that incidentally allowed this to build as unions are language features not library features.

However, now I'm reading the point by @kennytm that a union doesn't check the type size, so if that causes an issue I can revert that bit. (I don't think it's an issue here, the union is between $SelfT and [u8; mem::size_of::<$SelfT>].)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea the change looks alright. We'll want to move back to transmute at some point, so I added the const-hack label to this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I converted from transmute to union as suggested by @Centril, and that incidentally allowed this to build as unions are language features not library features.

Fwiw, this was not incidental at all; that's why I suggested it. ;)

// arrays of bytes
unsafe { mem::transmute(self) }
unsafe { Bytes { val: self }.bytes }
}
}

Expand Down Expand Up @@ -2284,7 +2292,7 @@ fn read_be_", stringify!($SelfT), "(input: &mut &[u8]) -> ", stringify!($SelfT),
}
```"),
#[stable(feature = "int_to_from_bytes", since = "1.32.0")]
#[rustc_const_unstable(feature = "const_int_conversion", issue = "53718")]
#[rustc_const_stable(feature = "const_int_conversion", since = "1.43.0")]
#[inline]
pub const fn from_be_bytes(bytes: [u8; mem::size_of::<Self>()]) -> Self {
Self::from_be(Self::from_ne_bytes(bytes))
Expand Down Expand Up @@ -2317,7 +2325,7 @@ fn read_le_", stringify!($SelfT), "(input: &mut &[u8]) -> ", stringify!($SelfT),
}
```"),
#[stable(feature = "int_to_from_bytes", since = "1.32.0")]
#[rustc_const_unstable(feature = "const_int_conversion", issue = "53718")]
#[rustc_const_stable(feature = "const_int_conversion", since = "1.43.0")]
#[inline]
pub const fn from_le_bytes(bytes: [u8; mem::size_of::<Self>()]) -> Self {
Self::from_le(Self::from_ne_bytes(bytes))
Expand Down Expand Up @@ -2360,11 +2368,19 @@ fn read_ne_", stringify!($SelfT), "(input: &mut &[u8]) -> ", stringify!($SelfT),
}
```"),
#[stable(feature = "int_to_from_bytes", since = "1.32.0")]
#[rustc_const_unstable(feature = "const_int_conversion", issue = "53718")]
#[rustc_const_stable(feature = "const_int_conversion", since = "1.43.0")]
// SAFETY: const sound because integers are plain old datatypes so we can always
// transmute to them
#[allow_internal_unstable(const_fn_union)]
#[inline]
pub const fn from_ne_bytes(bytes: [u8; mem::size_of::<Self>()]) -> Self {
#[repr(C)]
union Bytes {
val: $SelfT,
bytes: [u8; mem::size_of::<$SelfT>()],
}
// SAFETY: integers are plain old datatypes so we can always transmute to them
unsafe { mem::transmute(bytes) }
unsafe { Bytes { bytes }.val }
}
}
}
Expand Down Expand Up @@ -4132,7 +4148,7 @@ let bytes = ", $swap_op, stringify!($SelfT), ".to_be_bytes();
assert_eq!(bytes, ", $be_bytes, ");
```"),
#[stable(feature = "int_to_from_bytes", since = "1.32.0")]
#[rustc_const_unstable(feature = "const_int_conversion", issue = "53718")]
#[rustc_const_stable(feature = "const_int_conversion", since = "1.43.0")]
#[inline]
pub const fn to_be_bytes(self) -> [u8; mem::size_of::<Self>()] {
self.to_be().to_ne_bytes()
Expand All @@ -4152,7 +4168,7 @@ let bytes = ", $swap_op, stringify!($SelfT), ".to_le_bytes();
assert_eq!(bytes, ", $le_bytes, ");
```"),
#[stable(feature = "int_to_from_bytes", since = "1.32.0")]
#[rustc_const_unstable(feature = "const_int_conversion", issue = "53718")]
#[rustc_const_stable(feature = "const_int_conversion", since = "1.43.0")]
#[inline]
pub const fn to_le_bytes(self) -> [u8; mem::size_of::<Self>()] {
self.to_le().to_ne_bytes()
Expand Down Expand Up @@ -4187,12 +4203,20 @@ assert_eq!(
);
```"),
#[stable(feature = "int_to_from_bytes", since = "1.32.0")]
#[rustc_const_unstable(feature = "const_int_conversion", issue = "53718")]
#[rustc_const_stable(feature = "const_int_conversion", since = "1.43.0")]
// SAFETY: const sound because integers are plain old datatypes so we can always
// transmute them to arrays of bytes
#[allow_internal_unstable(const_fn_union)]
#[inline]
pub const fn to_ne_bytes(self) -> [u8; mem::size_of::<Self>()] {
#[repr(C)]
union Bytes {
val: $SelfT,
bytes: [u8; mem::size_of::<$SelfT>()],
}
// SAFETY: integers are plain old datatypes so we can always transmute them to
// arrays of bytes
unsafe { mem::transmute(self) }
unsafe { Bytes { val: self }.bytes }
}
}

Expand Down Expand Up @@ -4221,7 +4245,7 @@ fn read_be_", stringify!($SelfT), "(input: &mut &[u8]) -> ", stringify!($SelfT),
}
```"),
#[stable(feature = "int_to_from_bytes", since = "1.32.0")]
#[rustc_const_unstable(feature = "const_int_conversion", issue = "53718")]
#[rustc_const_stable(feature = "const_int_conversion", since = "1.43.0")]
#[inline]
pub const fn from_be_bytes(bytes: [u8; mem::size_of::<Self>()]) -> Self {
Self::from_be(Self::from_ne_bytes(bytes))
Expand Down Expand Up @@ -4254,7 +4278,7 @@ fn read_le_", stringify!($SelfT), "(input: &mut &[u8]) -> ", stringify!($SelfT),
}
```"),
#[stable(feature = "int_to_from_bytes", since = "1.32.0")]
#[rustc_const_unstable(feature = "const_int_conversion", issue = "53718")]
#[rustc_const_stable(feature = "const_int_conversion", since = "1.43.0")]
#[inline]
pub const fn from_le_bytes(bytes: [u8; mem::size_of::<Self>()]) -> Self {
Self::from_le(Self::from_ne_bytes(bytes))
Expand Down Expand Up @@ -4297,11 +4321,19 @@ fn read_ne_", stringify!($SelfT), "(input: &mut &[u8]) -> ", stringify!($SelfT),
}
```"),
#[stable(feature = "int_to_from_bytes", since = "1.32.0")]
#[rustc_const_unstable(feature = "const_int_conversion", issue = "53718")]
#[rustc_const_stable(feature = "const_int_conversion", since = "1.43.0")]
// SAFETY: const sound because integers are plain old datatypes so we can always
// transmute to them
#[allow_internal_unstable(const_fn_union)]
#[inline]
pub const fn from_ne_bytes(bytes: [u8; mem::size_of::<Self>()]) -> Self {
#[repr(C)]
union Bytes {
val: $SelfT,
bytes: [u8; mem::size_of::<$SelfT>()],
}
// SAFETY: integers are plain old datatypes so we can always transmute to them
unsafe { mem::transmute(bytes) }
unsafe { Bytes { bytes }.val }
}
}
}
Expand Down
2 changes: 0 additions & 2 deletions src/test/ui/consts/const-int-conversion-rpass.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
// run-pass

#![feature(const_int_conversion)]

const REVERSE: u32 = 0x12345678_u32.reverse_bits();
const FROM_BE_BYTES: i32 = i32::from_be_bytes([0x12, 0x34, 0x56, 0x78]);
const FROM_LE_BYTES: i32 = i32::from_le_bytes([0x12, 0x34, 0x56, 0x78]);
Expand Down