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

Stabilize str::len, [T]::len and str::as_bytes as const fn #63770

Merged
merged 1 commit into from
Sep 24, 2019

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Aug 21, 2019

r? @Centril

cc @RalfJung

This also introduces a scheme for making certain feature gates legal in stabilized const fns

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 21, 2019
@Centril Centril added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. T-lang Relevant to the language team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Aug 21, 2019
@Centril Centril added this to the 1.39 milestone Aug 21, 2019
src/libcore/lib.rs Outdated Show resolved Hide resolved
src/libcore/lib.rs Outdated Show resolved Hide resolved
src/libcore/slice/mod.rs Outdated Show resolved Hide resolved
src/libcore/str/mod.rs Outdated Show resolved Hide resolved
src/libcore/slice/mod.rs Show resolved Hide resolved
src/test/ui/consts/const-eval/strlen.rs Show resolved Hide resolved
src/librustc_mir/transform/qualify_min_const_fn.rs Outdated Show resolved Hide resolved
src/librustc_mir/transform/qualify_min_const_fn.rs Outdated Show resolved Hide resolved
@Centril
Copy link
Contributor

Centril commented Aug 21, 2019

cc also @eddyb

@Centril

This comment has been minimized.

@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 21, 2019

all review points have been addressed

@Centril Centril removed the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Aug 22, 2019
@Centril
Copy link
Contributor

Centril commented Aug 22, 2019

Dear libs and language teams and the community at large...

...I propose that we stabilize the following functions as const fn as it seems clear (to me) that committing to these being const fn is a safe commitment:

  • str::len, str::is_empty, and str::as_bytes
  • [T]::len and [T]::is_empty.

The language team element in this FCP consists of accepting the internal use of #[allow_internal_unstable(const_fn_union)] as seen throughout this PR.

We are not stabilizing const_fn_union at this time because it can be used to transmute between types. This in turn would allow you to encode the operation *[const|mut] T -> usize which can be used to observe addresses during execution. However, a const fn must retain deterministic execution even at run-time as laid out by @RalfJung: - "CTFE is correct if, when it loops forever, completes with a result, or panics, that behavior matches the run-time behavior of the same code.". As such, a const fn which behaves non-deterministically at run-time has undefined behavior. At this point in time we lack sufficient educational material to stabilize this for everyone but can still make internal use of it.

Our policy-to-be re. allow_internal_unstable with regards to const fn is documented here. Additionally, we would appreciate that you always ping @oli-obk, @RalfJung, and @Centril if you are adding more allow_internal_unstable attributes to any const fn.

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Aug 22, 2019

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

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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 Aug 22, 2019
src/libcore/slice/mod.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

However, a const fn must retain deterministic execution even at run-time

To elaborate on that, here's a "bad" const fn:

const fn mk_int() -> usize {
  let v = 42;
  &v as *const _ as usize
}

If we allow raw-to-int casts (or union accesses or transmute, both of which let users implement said casts), such a const fn can be written by stable (unsafe) code.

What's bad about that function is that it would be desirable for unsafe code, and maybe even for the compiler, to rely on the fact that a function with signature const fn() -> usize will always return the same thing, even when called at run-time.

@Centril
Copy link
Contributor

Centril commented Aug 24, 2019

To elaborate on that, here's a "bad" const fn:

@oli-obk might be a good idea to add to the rustc guide PR also.

@@ -435,6 +435,30 @@ pub fn find_by_name(attrs: &[Attribute], name: Symbol) -> Option<&Attribute> {
attrs.iter().find(|attr| attr.check_name(name))
}

pub fn allow_internal_unstable<'a>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
pub fn allow_internal_unstable<'a>(
pub fn allow_internal_unstable_list<'a>(

(suggests an action performed as currently named)

@Centril Centril added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 1, 2019
@Centril
Copy link
Contributor

Centril commented Sep 1, 2019

r=me with comment above addressed and #63770 (comment) incorporated into the rustc guide once FCP has completed.

@oli-obk
Copy link
Contributor Author

oli-obk commented Sep 1, 2019

Ping @Kimundi @joshtriplett @nikomatsakis @pnkfelix @withoutboats the FCP is waiting on an action by you

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 24, 2019
@bors
Copy link
Contributor

bors commented Sep 24, 2019

⌛ Testing commit 6324d165fbaa0c2d4f4c36143f7fd27a655187da with merge 27c7b4f41f73f366159bb0fa8f571c3c7f962876...

@rust-highfive
Copy link
Collaborator

The job dist-i686-freebsd of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2019-09-24T10:48:04.4327844Z    Compiling std v0.0.0 (/checkout/src/libstd)
2019-09-24T10:48:04.7436576Z error[E0133]: access to union field is unsafe and requires unsafe function or block
2019-09-24T10:48:04.7437069Z   --> src/libcore/slice/mod.rs:70:13
2019-09-24T10:48:04.7437752Z    |
2019-09-24T10:48:04.7438101Z 70 |             crate::ptr::Repr { rust: self }.raw.len
2019-09-24T10:48:04.7438712Z    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ access to union field
2019-09-24T10:48:04.7439000Z    |
2019-09-24T10:48:04.7439347Z    = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior
2019-09-24T10:48:04.7471984Z error[E0133]: access to union field is unsafe and requires unsafe function or block
2019-09-24T10:48:04.7472326Z     --> src/libcore/str/mod.rs:2180:18
2019-09-24T10:48:04.7472545Z      |
2019-09-24T10:48:04.7472545Z      |
2019-09-24T10:48:04.7472880Z 2180 |         unsafe { Slices { str: self }.slice }
2019-09-24T10:48:04.7473252Z      |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^ access to union field
2019-09-24T10:48:04.7473490Z      |
2019-09-24T10:48:04.7474038Z      = note: the field may not be properly initialized: using uninitialized data will cause undefined behavior
2019-09-24T10:48:04.7519607Z error: aborting due to 2 previous errors
2019-09-24T10:48:04.7519742Z 
2019-09-24T10:48:04.7528249Z For more information about this error, try `rustc --explain E0133`.
2019-09-24T10:48:04.8476969Z [RUSTC-TIMING] build_script_build test:false 0.410
---
2019-09-24T10:48:05.3772048Z == clock drift check ==
2019-09-24T10:48:05.3784430Z   local time: Tue Sep 24 10:48:05 UTC 2019
2019-09-24T10:48:05.5310826Z   network time: Tue, 24 Sep 2019 10:48:05 GMT
2019-09-24T10:48:05.5317046Z == end clock drift check ==
2019-09-24T10:48:09.6491482Z ##[error]Bash exited with code '1'.
2019-09-24T10:48:09.6527647Z ##[section]Starting: Upload CPU usage statistics
2019-09-24T10:48:09.6531121Z ==============================================================================
2019-09-24T10:48:09.6531198Z Task         : Bash
2019-09-24T10:48:09.6531281Z Description  : Run a Bash script on macOS, Linux, or Windows

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@bors
Copy link
Contributor

bors commented Sep 24, 2019

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 24, 2019
@oli-obk oli-obk force-pushed the allow_internal_unstable branch from 6324d16 to 7767e7f Compare September 24, 2019 10:56
@Centril
Copy link
Contributor

Centril commented Sep 24, 2019

@bors p=20 r+

@bors
Copy link
Contributor

bors commented Sep 24, 2019

📌 Commit 7767e7f has been approved by Centril

@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 Sep 24, 2019
@bors
Copy link
Contributor

bors commented Sep 24, 2019

⌛ Testing commit 7767e7f with merge 6ef275e...

bors added a commit that referenced this pull request Sep 24, 2019
Stabilize `str::len`, `[T]::len` and `str::as_bytes` as const fn

r? @Centril

cc @RalfJung

This also introduces a scheme for making certain feature gates legal in stabilized const fns
@bors
Copy link
Contributor

bors commented Sep 24, 2019

☀️ Test successful - checks-azure
Approved by: Centril
Pushing 6ef275e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 24, 2019
@bors bors merged commit 7767e7f into rust-lang:master Sep 24, 2019
@nvzqz nvzqz mentioned this pull request Nov 3, 2019
25 tasks
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Nov 11, 2019
Pkgsrc changes:
 * Remove patch which no longer applies (but what about RPATH?)
 * Adapt a few patches to changed files upstream.

Upstream changes:

Version 1.39.0 (2019-11-07)
===========================

Language
--------
- [You can now create `async` functions and blocks with `async fn`,
  `async move {}`, and `async {}` respectively, and you can now call
  `.await` on async expressions.][63209]
- [You can now use certain attributes on function, closure, and function
  pointer parameters.][64010] These attributes include `cfg`, `cfg_attr`,
  `allow`, `warn`, `deny`, `forbid` as well as inert helper attributes used
  by procedural macro attributes applied to items. e.g.
  ```rust
  fn len(
      #[cfg(windows)] slice: &[u16],
      #[cfg(not(windows))] slice: &[u8],
  ) -> usize {
      slice.len()
  }
  ```
- [You can now take shared references to bind-by-move patterns in the
  `if` guards of `match` arms.][63118] e.g.
  ```rust
  fn main() {
      let array: Box<[u8; 4]> = Box::new([1, 2, 3, 4]);

      match array {
          nums
  //      ---- `nums` is bound by move.
              if nums.iter().sum::<u8>() == 10
  //                 ^------ `.iter()` implicitly takes a reference to `nums`.
          => {
              drop(nums);
  //          ----------- Legal as `nums` was bound by move and so we have ownership.
          }
          _ => unreachable!(),
      }
  }
  ```

Compiler
--------
- [Added tier 3\* support for the `i686-unknown-uefi` target.][64334]
- [Added tier 3 support for the `sparc64-unknown-openbsd` target.][63595]
- [rustc will now trim code snippets in diagnostics to fit in your terminal.]
  [63402] **Note** Cargo currently doesn't use this feature. Refer to
  [cargo#7315][cargo/7315] to track this feature's progress.
- [You can now pass `--show-output` argument to test binaries to print the
  output of successful tests.][62600]

\* Refer to Rust's [platform support page][forge-platform-support] for more
information on Rust's tiered platform support.

Libraries
---------
- [`Vec::new` and `String::new` are now `const` functions.][64028]
- [`LinkedList::new` is now a `const` function.][63684]
- [`str::len`, `[T]::len` and `str::as_bytes` are now `const` functions.][63770]
- [The `abs`, `wrapping_abs`, and `overflowing_abs` numeric functions are
  now `const`.][63786]

Stabilized APIs
---------------
- [`Pin::into_inner`]
- [`Instant::checked_duration_since`]
- [`Instant::saturating_duration_since`]

Cargo
-----
- [You can now publish git dependencies if supplied with a `version`.]
  [cargo/7237]
- [The `--all` flag has been renamed to `--workspace`.][cargo/7241] Using
  `--all` is now deprecated.

Misc
----
- [You can now pass `-Clinker` to rustdoc to control the linker used
  for compiling doctests.][63834]

Compatibility Notes
-------------------
- [Code that was previously accepted by the old borrow checker, but rejected by
  the NLL borrow checker is now a hard error in Rust 2018.][63565] This was
  previously a warning, and will also become a hard error in the Rust 2015
  edition in the 1.40.0 release.
- [`rustdoc` now requires `rustc` to be installed and in the same directory to
  run tests.][63827] This should improve performance when running a large
  amount of doctests.
- [The `try!` macro will now issue a deprecation warning.][62672] It is
  recommended to use the `?` operator instead.
- [`asinh(-0.0)` now correctly returns `-0.0`.][63698] Previously this
  returned `0.0`.

[62600]: rust-lang/rust#62600
[62672]: rust-lang/rust#62672
[63118]: rust-lang/rust#63118
[63209]: rust-lang/rust#63209
[63402]: rust-lang/rust#63402
[63565]: rust-lang/rust#63565
[63595]: rust-lang/rust#63595
[63684]: rust-lang/rust#63684
[63698]: rust-lang/rust#63698
[63770]: rust-lang/rust#63770
[63786]: rust-lang/rust#63786
[63827]: rust-lang/rust#63827
[63834]: rust-lang/rust#63834
[63927]: rust-lang/rust#63927
[63933]: rust-lang/rust#63933
[63934]: rust-lang/rust#63934
[63938]: rust-lang/rust#63938
[63940]: rust-lang/rust#63940
[63941]: rust-lang/rust#63941
[63945]: rust-lang/rust#63945
[64010]: rust-lang/rust#64010
[64028]: rust-lang/rust#64028
[64334]: rust-lang/rust#64334
[cargo/7237]: rust-lang/cargo#7237
[cargo/7241]: rust-lang/cargo#7241
[cargo/7315]: rust-lang/cargo#7315
[`Pin::into_inner`]: https://doc.rust-lang.org/std/pin/struct.Pin.html#method.into_inner
[`Instant::checked_duration_since`]: https://doc.rust-lang.org/std/time/struct.Instant.html#method.checked_duration_since
[`Instant::saturating_duration_since`]: https://doc.rust-lang.org/std/time/struct.Instant.html#method.saturating_duration_since
Centril added a commit to Centril/rust that referenced this pull request Jan 11, 2020
…r=Centril

Don't require `allow_internal_unstable` unless `staged_api` is enabled.

rust-lang#63770 changed `qualify_min_const_fn` to require `allow_internal_unstable` for *all* crates that used an unstable feature, regardless of whether `staged_api` was enabled or the `fn` that used that feature was stably const. In practice, this meant that every crate in the ecosystem that wanted to use nightly features added `#![feature(const_fn)]`, which skips `qualify_min_const_fn` entirely.

After this PR, crates that do not have `#![feature(staged_api)]` will only need to enable the feature they are interested in. For example, `#![feature(const_if_match)]` will be enough to enable `if` and `match` in constants. Crates with `staged_api` (e.g., `libstd`) require `#[allow_internal_unstable]` to be added to a function if it uses nightly features unless that function is also marked `#[rustc_const_unstable]`. This prevents proliferation of `#[allow_internal_unstable]` into functions that are not callable in a `const` context on stable.

r? @oli-obk (author of rust-lang#63770)
cc @Centril
Centril added a commit to Centril/rust that referenced this pull request Jan 11, 2020
…r=Centril

Don't require `allow_internal_unstable` unless `staged_api` is enabled.

rust-lang#63770 changed `qualify_min_const_fn` to require `allow_internal_unstable` for *all* crates that used an unstable feature, regardless of whether `staged_api` was enabled or the `fn` that used that feature was stably const. In practice, this meant that every crate in the ecosystem that wanted to use nightly features added `#![feature(const_fn)]`, which skips `qualify_min_const_fn` entirely.

After this PR, crates that do not have `#![feature(staged_api)]` will only need to enable the feature they are interested in. For example, `#![feature(const_if_match)]` will be enough to enable `if` and `match` in constants. Crates with `staged_api` (e.g., `libstd`) require `#[allow_internal_unstable]` to be added to a function if it uses nightly features unless that function is also marked `#[rustc_const_unstable]`. This prevents proliferation of `#[allow_internal_unstable]` into functions that are not callable in a `const` context on stable.

r? @oli-obk (author of rust-lang#63770)
cc @Centril
Centril added a commit to Centril/rust that referenced this pull request Jan 11, 2020
…r=Centril

Don't require `allow_internal_unstable` unless `staged_api` is enabled.

rust-lang#63770 changed `qualify_min_const_fn` to require `allow_internal_unstable` for *all* crates that used an unstable feature, regardless of whether `staged_api` was enabled or the `fn` that used that feature was stably const. In practice, this meant that every crate in the ecosystem that wanted to use nightly features added `#![feature(const_fn)]`, which skips `qualify_min_const_fn` entirely.

After this PR, crates that do not have `#![feature(staged_api)]` will only need to enable the feature they are interested in. For example, `#![feature(const_if_match)]` will be enough to enable `if` and `match` in constants. Crates with `staged_api` (e.g., `libstd`) require `#[allow_internal_unstable]` to be added to a function if it uses nightly features unless that function is also marked `#[rustc_const_unstable]`. This prevents proliferation of `#[allow_internal_unstable]` into functions that are not callable in a `const` context on stable.

r? @oli-obk (author of rust-lang#63770)
cc @Centril
@oli-obk oli-obk deleted the allow_internal_unstable branch March 16, 2021 12:13
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. merged-by-bors This PR was explicitly merged by bors. 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-lang Relevant to the language team, which will review and decide on the PR/issue. 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.

9 participants