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

Minor standard library constification #55278

Merged
merged 13 commits into from
Nov 12, 2018
Merged

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Oct 23, 2018

This PR makes some bits of the standard library into const fns.
I've tried to be as aggressive as I possibly could in the constification.
The list is rather small due to how restrictive const fn is at the moment.

r? @oli-obk cc @rust-lang/libs

Stable public APIs affected:

  • Cell::as_ptr
  • UnsafeCell::get
  • char::is_ascii
  • iter::empty
  • ManuallyDrop::{new, into_inner}
  • RangeInclusive::{start, end}
  • NonNull::as_ptr
  • {[T], str}::as_ptr
  • Duration::{as_secs, subsec_millis, subsec_micros, subsec_nanos}
  • CStr::as_ptr
  • Ipv4Addr::is_unspecified
  • Ipv6Addr::new
  • Ipv6Addr::octets

Unstable public APIs affected:

  • Duration::{as_millis, as_micros, as_nanos, as_float_secs}
  • Wrapping::{count_ones, count_zeros, trailing_zeros, rotate_left, rotate_right, swap_bytes, reverse_bits, from_be, from_le, to_be, to_le, leading_zeros, is_positive, is_negative, leading_zeros}
  • core::convert::identity

Removed from list in first pass:

Stable public APIs affected:

  • BTree{Map, Set}::{len, is_empty}
  • VecDeque::is_empty
  • String::{is_empty, len}
  • FromUtf8Error::utf8_error
  • Vec<T>::{is_empty, len}
  • Layout::size
  • DecodeUtf16Error::unpaired_surrogate
  • core::fmt::{fill, width, precision, sign_plus, sign_minus, alternate, sign_aware_zero_pad}
  • panic::Location::{file, line, column}
  • {ChunksExact, RChunksExact}::remainder
  • Utf8Error::valid_up_to
  • VacantEntry::key
  • NulError::nul_position
  • IntoStringError::utf8_error
  • IntoInnerError::error
  • io::Chain::get_ref
  • io::Take::{limit, get_ref}
  • SocketAddrV6::{flowinfo, scope_id}
  • PrefixComponent::{kind, as_os_str}
  • Path::{ancestors, display}
  • WaitTimeoutResult::timed_out
  • Receiver::{iter, try_iter}
  • thread::JoinHandle::thread
  • SystemTimeError::duration

Unstable public APIs affected:

  • core::fmt::Arguments::new_v1
  • core::fmt::Arguments::new_v1_formatted
  • Pin::{get_ref, into_ref}
  • Utf8Lossy::chunks
  • LocalWaker::as_waker
  • panic::PanicInfo::{internal_constructor, message, location}
  • panic::Location::{internal_constructor }

Removed from list in 2nd pass:

Stable public APIs affected:

  • LinkedList::{new, iter, is_empty, len}
  • mem::forget
  • Cursor::{new, get_ref, position}
  • io::{empty, repeat, sink}
  • PoisonError::new
  • thread::Builder::new
  • process::Stdio::{piped, inherit, null}

Unstable public APIs affected:

  • io::Initializer::{zeroing, should_initialize}

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 23, 2018
@sfackler
Copy link
Member

Many of these are literally impossible to use in a const context. Why are we doing this?

@Centril
Copy link
Contributor Author

Centril commented Oct 23, 2018

  • I wanted to see how much we could possible do right now -- I fully expect you'll be wanting to reduce the list (which is what the boxes are for) :)

  • Some of the ones, like vec.is_empty() that you can't use in a const context right now are due to the lack of const fn constructors. Once you get those, these will be usable in const contexts. We could wait until then, but this felt easier to do it at once instead of people asking for one after the other. I.e. asking "has this become useful in const contexts" each time on every function seems too time consuming.

    The question to be asked on each function I think is: "is there some reason reason why we are not comfortable guaranteeing that this is const fn because we might want to introduce non-const behavior later?"

  • const fn is not just about being usable in const contexts; it's also saying "this is referentially transparent".

@rust-highfive

This comment has been minimized.

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

Most of these seem fine in isolation.

I don't think it is good practice to make functions const fn while they have an argument that is not constructable in a const context.

Doing so will incentivise people to use unions or transmute to construct values in const contexts.

src/libcore/fmt/mod.rs Outdated Show resolved Hide resolved
src/libcore/panic.rs Outdated Show resolved Hide resolved
@withoutboats
Copy link
Contributor

I don't think it is good practice to make functions const fn while they have an argument that is not constructable in a const context.

Agreed.

Right now we haven't established a shared vision about what should be const. There's an argument to be made that anything that can be const should be, but that argument has not been made yet.

I think as a first pass, to make this list more manageable, I'd like to see every function removed that can't actually be called in a const context already.

const fn is not just about being usable in const contexts; it's also saying "this is referentially transparent".

const fn isn't intended to mean that the function is referentially transparent. In particular, there's not a technical reason we could not allow mutable references as arguments to const fn, which would violate any referential transparency. const fn is not pure fn.

@Centril
Copy link
Contributor Author

Centril commented Oct 23, 2018

Right now we haven't established a shared vision about what should be const. There's an argument to be made that anything that can be const should be, but that argument has not been made yet.

That's my vision at least; I guess it might need to be RFCed to become shared.

I think as a first pass, to make this list more manageable, I'd like to see every function removed that can't actually be called in a const context already.

Will do. :)

const fn isn't intended to mean that the function is referentially transparent. In particular, there's not a technical reason we could not allow mutable references as arguments to const fn, which would violate any referential transparency. const fn is not pure fn.

Mutable references would not really violate referential transparency in my view and I have no problem allowing them in const fn. With a function such as const fn foo(y: A, x: &mut B) -> C {...} you are in effect saying the same thing as A -> B -> (B, C), which is equivalent to A -> State B C. Yet, state monads do not cause global side effects in Haskell. To me, const fn is pure fn in all but name.

@Centril
Copy link
Contributor Author

Centril commented Oct 23, 2018

I think as a first pass, to make this list more manageable, I'd like to see every function removed that can't actually be called in a const context already.

Will do. :)

@withoutboats Done. I've segregated the list; the ones that are removed are in their separate list in the OP.

src/liballoc/collections/binary_heap.rs Outdated Show resolved Hide resolved
src/liballoc/collections/btree/node.rs Outdated Show resolved Hide resolved
src/liballoc/collections/vec_deque.rs Outdated Show resolved Hide resolved
@Centril
Copy link
Contributor Author

Centril commented Oct 23, 2018

Removed changes to internal APIs per review.

@oli-obk
Copy link
Contributor

oli-obk commented Oct 24, 2018

I am for this change. But we do have a ban on creating new stable public const fns in the libstd, and we haven't officially lifted that ban.

cc @nikomatsakis I am reasonably sure that we have fixed everything that can go wrong with new public const fns. Reasons we didn't stabilize any new ones before:

  • the const_fn feature gate is too powerful, we weren't really sure what we were exposing
    • resolved via min_const_fn
  • const fns were immediately promotable
    • no libstd/libcore function is promotable unless marked so with the rustc_const_promotable attribute
    • no user defined const fns are promotable unless the const_fn feature gate is enabled

I propose we lift the ban on adding new const fns to the libstd

@Centril
Copy link
Contributor Author

Centril commented Oct 24, 2018

But we do have a ban on creating new stable public const fns in the libstd, and we haven't officially lifted that ban.

I believe the ban was originally imposed by the language team for the reasons @oli-obk mentioned.
I think from the language team's perspective they should be considered lifted given the stabilization of min_const_fn and the changes mentioned. Indeed, stabilized specifically in such a way to facilitate stabilization of new functions in the standard library as const fn.

I propose we lift the ban on adding new const fns to the libstd

Yes, let's :)

@alexcrichton
Copy link
Member

The PR title currently says it's a "Minor standard library constification" but then the PR text says:

I've tried to be as aggressive as I possibly could in the constification.

It looks like this has been scaled back since the first round but we need to be very careful to not over-promise on implementations here. Landing this PR will require an FCP signoff from the @rust-lang/libs team.

I personally still think that the list is too ambitious and would like to see it cut back further. We can always add const whenever we want, it's impossible to remove it once stabilized. I'd rather we take our time and move deliberately rather than simply codify what today's implementation happens to be.

For example I don't think we should stabilize the const-ness of the following APIs:

  • LinkedList::iter - the iterator returned can't be used?
  • char::is_ascii - is anyone asking for this?
  • mem::forget - is anyone asking for this?
  • Cursor - I don't think this is ever used in a const context
  • io::* - is this needed yet really?
  • Ipv4Addr::is_unspecified - this seems... random? If new isn't even const why would this be?
  • PoisonError::new - I kinda doubt anyone wants this
  • thread::Builder::new - I really doubt anyone wants this
  • process::Stdio::{piped, inherit, null} - I think this is overpromising and not getting much benefit, I think it should be removed from this list

@Centril
Copy link
Contributor Author

Centril commented Oct 24, 2018

@alexcrichton

The PR title currently says it's a "Minor standard library constification" but then the PR text says:

I've tried to be as aggressive as I possibly could in the constification.

As aggressive as possible given what stable const fn can do; which isn't much ;)
Once we can do control flow (match and friends), let bindings, loops, and panics in const fn then I think we should be many many times more aggressive than this.

I personally still think that the list is too ambitious and would like to see it cut back further. We can always add const whenever we want, it's impossible to remove it once stabilized. I'd rather we take our time and move deliberately rather than simply codify what today's implementation happens to be.

Fair enough; I personally think we should ask the question "is there any reason we would introduce non-const behavior in this function making it not a const fn?" rather than "is anyone asking for this". As I noted before, once const fn gets more powerful, which it should in the coming months I think, the number of things that can be const fn in the standard library will increase dramatically; In such a future, being too deliberate on each function will take a long time to go through. When the power increases, I would at least recommend the libs team to take stock of everything in the standard library.

* `char::is_ascii` - is anyone asking for this?

In the sense that someone has explicitly asked for it? No.
The proposed list came about by looking at every single standard library function and then seeing if it could be constified.

The definition is fairly simple, so you should be able to copy it readily.
However, I think if we ask "is anyone asking for this?" for each of these kinds of functions then constification will take considerable time. So I'd personally be more eager in constification than doing it lazily when people ask for it.

* `Ipv4Addr::is_unspecified` - this seems... random? If `new` isn't even const why would this be?

new is const on nightly at least, https://doc.rust-lang.org/nightly/std/net/struct.Ipv4Addr.html#method.new so it can be used.

@RalfJung
Copy link
Member

@withoutboats wrote

const fn is not pure fn.

And I agree. I appreciate that is your vision, @Centril, but until we have adapted this vision as our shared vision, please do not use it to justify changes that we cannot take back later.


I personally think we should ask the question "is there any reason we would introduce non-const behavior in this function making it not a const fn?" rather than "is anyone asking for this".

I think the question should be "is this useful?" -- as in, can you as the advocate of these features come up with an example where using this function as a const fn actually makes sense, an example that seems somewhat realistic and that compiles with your PR but does not without?

Things related to concurrency (thread, PoisonError) and IO (io, process) immediately fail this criterion. Cursor is also useless because its point is the Seek implementation, and we don't get that in const context.

LinkedList::new makes sense, we usually have const fn container constructors (and they have a clear purpose). Less so for the other methods -- we don't have len and is_empty be const fn for other containers either, do we? I think we should do this more consistently, in particular given that you cannot actually fill a container in const fn and hence these getters and iterators don't make that much sense. I don't like stabilizing a different random subset of the API that LinkedList, Vec, VecDeque etc all share to be const fn -- these APIs being so uniform is extremely useful, we shouldn't give up that uniformity without a good reason.

mem::forget isn't useful yet as we cannot have Drop types in const, can we? But I do not mind very strongly.

is_ascii and is_unspecified are fine, I think, as are all the remaining things not on @alexcrichton's list.

@Centril
Copy link
Contributor Author

Centril commented Oct 25, 2018

I've trimmed the list along the lines per @alexcrichton's list; I kept is_ascii and is_unspecified per @RalfJung's notes. I hope that's OK.

@nikomatsakis
Copy link
Contributor

My take:

  • @oli-obk convinced me we can lift the ban
  • I agree with @RalfJung's criteria basically 100%

The one that I am torn about is mem::forget, because it feels like a "primitive capability" that we will eventually want to expose -- but it can also wait, and perhaps it's better not to send misleading signals (i.e., you can't actually use this productively in a const anyway...).

@rust-highfive

This comment has been minimized.

src/libcore/fmt/mod.rs Outdated Show resolved Hide resolved
@bors

This comment has been minimized.

@alexcrichton alexcrichton added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Oct 29, 2018
@alexcrichton
Copy link
Member

Ok I think the list looks good now, but I'd like to get libs team signoff as well:

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Oct 29, 2018

Team member @alexcrichton has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Oct 29, 2018
@rfcbot
Copy link

rfcbot commented Oct 30, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

@Centril Centril changed the title [WIP] Minor standard library constification Minor standard library constification Nov 10, 2018
@Centril Centril added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 10, 2018
@Centril
Copy link
Contributor Author

Centril commented Nov 10, 2018

@alexcrichton rebased + minor other fixes. :)

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Nov 12, 2018

📌 Commit ac1c6b0 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 12, 2018
@bors
Copy link
Contributor

bors commented Nov 12, 2018

⌛ Testing commit ac1c6b0 with merge 65204a9...

bors added a commit that referenced this pull request Nov 12, 2018
Minor standard library constification

This PR makes some bits of the standard library into `const fn`s.
I've tried to be as aggressive as I possibly could in the constification.
The list is rather small due to how restrictive `const fn` is at the moment.

r? @oli-obk cc @rust-lang/libs

Stable public APIs affected:
+ [x] `Cell::as_ptr`
+ [x] `UnsafeCell::get`
+ [x] `char::is_ascii`
+ [x] `iter::empty`
+ [x] `ManuallyDrop::{new, into_inner}`
+ [x] `RangeInclusive::{start, end}`
+ [x] `NonNull::as_ptr`
+ [x] `{[T], str}::as_ptr`
+ [x] `Duration::{as_secs, subsec_millis, subsec_micros, subsec_nanos}`
+ [x] `CStr::as_ptr`
+ [x] `Ipv4Addr::is_unspecified`
+ [x] `Ipv6Addr::new`
+ [x] `Ipv6Addr::octets`

Unstable public APIs affected:
+ [x] `Duration::{as_millis, as_micros, as_nanos, as_float_secs}`
+ [x] `Wrapping::{count_ones, count_zeros, trailing_zeros, rotate_left, rotate_right, swap_bytes, reverse_bits, from_be, from_le, to_be, to_le, leading_zeros, is_positive, is_negative, leading_zeros}`
+ [x] `core::convert::identity`

--------------------------

## Removed from list in first pass:

Stable public APIs affected:
+ [ ] `BTree{Map, Set}::{len, is_empty}`
+ [ ] `VecDeque::is_empty`
+ [ ] `String::{is_empty, len}`
+ [ ] `FromUtf8Error::utf8_error`
+ [ ] `Vec<T>::{is_empty, len}`
+ [ ] `Layout::size`
+ [ ] `DecodeUtf16Error::unpaired_surrogate`
+ [ ] `core::fmt::{fill, width, precision, sign_plus, sign_minus, alternate, sign_aware_zero_pad}`
+ [ ] `panic::Location::{file, line, column}`
+ [ ] `{ChunksExact, RChunksExact}::remainder`
+ [ ] `Utf8Error::valid_up_to`
+ [ ] `VacantEntry::key`
+ [ ] `NulError::nul_position`
+ [ ] `IntoStringError::utf8_error`
+ [ ] `IntoInnerError::error`
+ [ ] `io::Chain::get_ref`
+ [ ] `io::Take::{limit, get_ref}`
+ [ ] `SocketAddrV6::{flowinfo, scope_id}`
+ [ ] `PrefixComponent::{kind, as_os_str}`
+ [ ] `Path::{ancestors, display}`
+ [ ] `WaitTimeoutResult::timed_out`
+ [ ] `Receiver::{iter, try_iter}`
+ [ ] `thread::JoinHandle::thread`
+ [ ] `SystemTimeError::duration`

Unstable public APIs affected:
+ [ ] `core::fmt::Arguments::new_v1`
+ [ ] `core::fmt::Arguments::new_v1_formatted`
+ [ ] `Pin::{get_ref, into_ref}`
+ [ ] `Utf8Lossy::chunks`
+ [ ] `LocalWaker::as_waker`
+ [ ] `panic::PanicInfo::{internal_constructor, message, location}`
+ [ ] `panic::Location::{internal_constructor }`

## Removed from list in 2nd pass:

Stable public APIs affected:
+ [ ] `LinkedList::{new, iter, is_empty, len}`
+ [ ] `mem::forget`
+ [ ] `Cursor::{new, get_ref, position}`
+ [ ] `io::{empty, repeat, sink}`
+ [ ] `PoisonError::new`
+ [ ] `thread::Builder::new`
+ [ ] `process::Stdio::{piped, inherit, null}`

Unstable public APIs affected:
+ [ ] `io::Initializer::{zeroing, should_initialize}`
@Mark-Simulacrum
Copy link
Member

@Centril Is the top comment up-to-date? Mostly asking to help us down the line when making release notes.

@bors
Copy link
Contributor

bors commented Nov 12, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 65204a9 to master...

@bors bors merged commit ac1c6b0 into rust-lang:master Nov 12, 2018
@Centril
Copy link
Contributor Author

Centril commented Nov 12, 2018

@Mark-Simulacrum I can't say it is with confidence but it probably is; I'd search for pub const fn to be sure.

@Centril Centril deleted the constification-1 branch November 12, 2018 23:52
@memoryruins
Copy link
Contributor

The top comment appears to be up-to-date.

If needed for release notes, "affected" includes two types of changes -- prepending const to fn and removing rustc_const_unstable feature attributes. The following lists where attributes were removed from preexisting const fns:

Stable public APIs affected:
ManuallyDrop::new
{[T], str}::as_ptr
Duration::{as_secs, subsec_millis, subsec_micros, subsec_nanos}
Ipv6Addr::new

Unstable public APIs affected:
core::convert::identity

The rest of the top comment prepends const to fn.

@Centril Centril added this to the 1.32 milestone Apr 27, 2019
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 22, 2020
…=sfackler

Make `u8::is_ascii` a stable `const fn`

`char::is_ascii` was already stabilized as `const fn` in rust-lang#55278, so there is no reason for `u8::is_ascii` to go through an unstable period.

cc @rust-lang/libs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.