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

Make Layout::new const #66254

Merged
merged 2 commits into from
Jan 9, 2020
Merged

Make Layout::new const #66254

merged 2 commits into from
Jan 9, 2020

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented Nov 9, 2019

This seems like a reasonable change to make. If we don't provide Layout::new::<T> as const, then users can just instead do the more error prone Layout::from_size_align_unchecked(mem::size_of::<T>(), mem::align_of::<T>()) for the same effect and an extra unsafe { } incantation.

@rust-highfive
Copy link
Collaborator

r? @KodrAus

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 9, 2019
@jonas-schievink jonas-schievink added relnotes Marks issues that should be documented in the release notes of the next release. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Nov 9, 2019
@jonas-schievink jonas-schievink added this to the 1.41 milestone Nov 9, 2019
@Centril Centril added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Nov 12, 2019
@CAD97
Copy link
Contributor Author

CAD97 commented Nov 25, 2019

cc @rust-lang/libs for [needs-fcp]

Copy link
Contributor

@KodrAus KodrAus left a comment

Choose a reason for hiding this comment

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

This looks good to me!

@rfcbot fcp merge

@sfackler
Copy link
Member

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Nov 30, 2019

Team member @sfackler 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. final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Nov 30, 2019
@rfcbot
Copy link

rfcbot commented Dec 5, 2019

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

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Dec 5, 2019
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Dec 15, 2019
@rfcbot
Copy link

rfcbot commented Dec 15, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

@KodrAus
Copy link
Contributor

KodrAus commented Dec 15, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Dec 15, 2019

📌 Commit e8c53a6 has been approved by KodrAus

@bors
Copy link
Contributor

bors commented Dec 15, 2019

🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened

@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 Dec 15, 2019
@bors
Copy link
Contributor

bors commented Dec 16, 2019

⌛ Testing commit e8c53a6 with merge ca7e02a1ee5dc9691042789d473f1b574af59be8...

@rust-highfive
Copy link
Collaborator

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-12-16T07:16:18.9965567Z TMP=/tmp
2019-12-16T07:16:18.9965656Z TOOLSTATE_ISSUES_API_URL=https://api.github.com/repos/rust-lang/rust/issues
2019-12-16T07:16:18.9965731Z TOOLSTATE_PUBLISH=1
2019-12-16T07:16:18.9965822Z TOOLSTATE_REPO=https://github.com/rust-lang-nursery/rust-toolstate
2019-12-16T07:16:18.9965971Z This seems like a reasonable change to make. If we don't provide `Layout::new::<T>` as `const`, then users can just instead do the more error prone `Layout::from_size_align_unchecked(mem::size_of::<T>(), mem::align_of::<T>())` for the same effect and an extra `unsafe { }` incantation.
2019-12-16T07:16:18.9966203Z USERDOMAIN=fv-az433
2019-12-16T07:16:18.9966770Z USERDOMAIN_ROAMINGPROFILE=fv-az433
2019-12-16T07:16:18.9966903Z USERNAME=VssAdministrator
2019-12-16T07:16:18.9966977Z USERPROFILE=C:\Users\VssAdministrator
---
2019-12-16T07:16:31.6801285Z Chocolatey v0.10.15
2019-12-16T07:17:23.0516529Z Installing the following packages:
2019-12-16T07:17:23.0520022Z msys2
2019-12-16T07:17:23.0526557Z By installing you accept licenses for the packages.
2019-12-16T07:19:03.8815759Z Error retrieving packages from source 'https://chocolatey.org/api/v2/':
2019-12-16T07:19:03.8816645Z  Could not connect to the feed specified at 'https://chocolatey.org/api/v2/'. Please verify that the package source (located in the Package Manager Settings) is valid and ensure your network connectivity.
2019-12-16T07:19:03.8824465Z msys2 not installed. The package was not found with the source(s) listed.
2019-12-16T07:19:03.8824971Z  Source(s): 'https://chocolatey.org/api/v2/'
2019-12-16T07:19:03.8825314Z  NOTE: When you specify explicit sources, it overrides default sources.
2019-12-16T07:19:03.8825563Z If the package version is a prerelease and you didn't specify `--pre`,
2019-12-16T07:19:03.8826087Z Please see https://chocolatey.org/docs/troubleshooting for more 
2019-12-16T07:19:03.8826287Z  assistance.
2019-12-16T07:19:03.8941795Z 
2019-12-16T07:19:03.8941928Z Chocolatey installed 0/1 packages. 1 packages failed.
2019-12-16T07:19:03.8941928Z Chocolatey installed 0/1 packages. 1 packages failed.
2019-12-16T07:19:03.8942033Z  See the log for details (C:\ProgramData\chocolatey\logs\chocolatey.log).
2019-12-16T07:19:03.8946317Z 
2019-12-16T07:19:03.8950367Z Failures
2019-12-16T07:19:03.8956556Z  - msys2 - msys2 not installed. The package was not found with the source(s) listed.
2019-12-16T07:19:03.8956724Z  Source(s): 'https://chocolatey.org/api/v2/'
2019-12-16T07:19:03.8956824Z  NOTE: When you specify explicit sources, it overrides default sources.
2019-12-16T07:19:03.8957135Z If the package version is a prerelease and you didn't specify `--pre`,
2019-12-16T07:19:03.8957290Z Please see https://chocolatey.org/docs/troubleshooting for more 
2019-12-16T07:19:03.8957373Z  assistance.
2019-12-16T07:19:04.2795267Z 
2019-12-16T07:19:04.2795267Z 
2019-12-16T07:19:04.2877650Z ##[error]Bash exited with code '1'.
2019-12-16T07:19:04.3001392Z ##[section]Starting: Checkout
2019-12-16T07:19:04.3106352Z ==============================================================================
2019-12-16T07:19:04.3106450Z Task         : Get sources
2019-12-16T07:19:04.3106568Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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 Dec 16, 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 Dec 16, 2019
@bors
Copy link
Contributor

bors commented Dec 23, 2019

☔ The latest upstream changes (presumably #67540) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 23, 2019
@lukaslueg
Copy link
Contributor

Unbeknown I did the same thing (and more) in #67494 ; should I wait for this to be rebased and resubmit?

@oli-obk
Copy link
Contributor

oli-obk commented Dec 26, 2019

well this stabilizes it, while your PR just makes it const on nightly. I guess it would be best to wait for this PR and then stabilize the rest. If you want you can already change your PR to just not touch fn new

1 similar comment
@oli-obk
Copy link
Contributor

oli-obk commented Dec 26, 2019

well this stabilizes it, while your PR just makes it const on nightly. I guess it would be best to wait for this PR and then stabilize the rest. If you want you can already change your PR to just not touch fn new

@lukaslueg lukaslueg mentioned this pull request Dec 28, 2019
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Dec 31, 2019
Constify Result

r? @oli-obk

This is just the `Result`-part of rust-lang#67494 which I'll resubmit once rust-lang#66254 has landed.
Centril added a commit to Centril/rust that referenced this pull request Dec 31, 2019
Constify Result

r? @oli-obk

This is just the `Result`-part of rust-lang#67494 which I'll resubmit once rust-lang#66254 has landed.
@CAD97
Copy link
Contributor Author

CAD97 commented Jan 9, 2020

Rebased again.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 9, 2020

@bors r=KodrAus

@bors
Copy link
Contributor

bors commented Jan 9, 2020

📌 Commit d860def has been approved by KodrAus

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 9, 2020
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jan 9, 2020
Make Layout::new const

This seems like a reasonable change to make. If we don't provide `Layout::new::<T>` as `const`, then users can just instead do the more error prone `Layout::from_size_align_unchecked(mem::size_of::<T>(), mem::align_of::<T>())` for the same effect and an extra `unsafe { }` incantation.
bors added a commit that referenced this pull request Jan 9, 2020
Rollup of 10 pull requests

Successful merges:

 - #66254 (Make Layout::new const)
 - #67122 (Do not deduplicate diagnostics in UI tests)
 - #67358 (Add HashSet::get_or_insert_owned)
 - #67725 (Simplify into_key_slice_mut)
 - #67935 (Relax the Sized bounds on Pin::map_unchecked(_mut))
 - #67967 (Delay bug to prevent ICE in MIR borrowck)
 - #67975 (Export public scalar statics in wasm)
 - #68006 (Recognise riscv64 in compiletest)
 - #68040 (Cleanup)
 - #68054 (doc: add Null-unchecked version section to mut pointer as_mut method)

Failed merges:

 - #67258 (Introduce `X..`, `..X`, and `..=X` range patterns)

r? @ghost
@bors bors merged commit d860def into rust-lang:master Jan 9, 2020
lukaslueg added a commit to lukaslueg/rust that referenced this pull request Jan 11, 2020
let (size, align) = size_align::<T>();
// Note that the align is guaranteed by rustc to be a power of two and
// the size+align combo is guaranteed to fit in our address space. As a
// result use the unchecked constructor here to avoid inserting code
// that panics if it isn't optimized well enough.
debug_assert!(Layout::from_size_align(size, align).is_ok());
Copy link
Member

Choose a reason for hiding this comment

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

Now that we have conditionals in const, can we re-add the debug_assert!?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that the stdlib is always compiled in release mode, it's not too useful to re-add.

But yes, given feature(const_fn, const_if_match, and const_panic), this debug assert can be made to compile. (Ideally it'd also have const fn Result::is_ok to avoid using matches! directly.)

Copy link
Member

Choose a reason for hiding this comment

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

We have some CI runners that have debug assertions enabled; it is not uncommon for rustc devs to enable debug assertions locally; Miri will hopefully enable libstd debug assertions eventually. So, there is some use to these (and the libstd is using them in plenty of places).

Copy link
Contributor

Choose a reason for hiding this comment

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

is_ok was recently made const in #67685

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RalfJung For some reason, while I can get debug_assert! in const_fn on the playground, any attempt to add it in libcore, even #[cfg(not(bootstrap))] is giving me error[E0723]: loops and conditional expressions are not stable in const fn.

diff --git a/src/libcore/alloc.rs b/src/libcore/alloc.rs
index a04e75bc7ce..a679fe160a8 100644
--- a/src/libcore/alloc.rs
+++ b/src/libcore/alloc.rs
@@ -131,6 +131,8 @@ impl Layout {
         // the size+align combo is guaranteed to fit in our address space. As a
         // result use the unchecked constructor here to avoid inserting code
         // that panics if it isn't optimized well enough.
+        #[cfg(not(bootstrap))]
+        debug_assert!(Layout::from_size_align(size, align).is_ok());
         unsafe { Layout::from_size_align_unchecked(size, align) }
     }
 
diff --git a/src/libcore/lib.rs b/src/libcore/lib.rs
index bca96b77812..00ddbb923e2 100644
--- a/src/libcore/lib.rs
+++ b/src/libcore/lib.rs
@@ -82,6 +82,7 @@
 #![feature(const_int_pow)]
 #![feature(constctlz)]
 #![feature(const_panic)]
+#![feature(const_fn)]
 #![feature(const_fn_union)]
 #![feature(const_generics)]
 #![feature(const_ptr_offset_from)]
error[E0723]: loops and conditional expressions are not stable in const fn                                                                    
   --> src\libcore\macros\mod.rs:184:23
    |
183 | / macro_rules! debug_assert {
184 | |     ($($arg:tt)*) => (if $crate::cfg!(debug_assertions) { $crate::assert!($($arg)*); })
    | |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
185 | | }
    | |_- in this expansion of `debug_assert!`
    | 
   ::: src\libcore\alloc.rs:135:9
    |
135 |           debug_assert!(Layout::from_size_align(size, align).is_ok());
    |           ------------------------------------------------------------ in this macro invocation
    |
    = note: see issue #57563 <https://github.com/rust-lang/rust/issues/57563> for more information
    = help: add `#![feature(const_fn)]` to the crate attributes to enable

Copy link
Member

@RalfJung RalfJung Feb 21, 2020

Choose a reason for hiding this comment

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

@CAD97 Stable const fn cannot use unstable features. There's allow_internal_unstable to work around this, but at this point we might not yet want to stabilize methods that use these features internally. OTOH this is just a debug assertion so it seems fine to me.

Also @oli-obk is the better person for such questions. :) Or just ping the entire team: @rust-lang/wg-const-eval

Copy link
Member

Choose a reason for hiding this comment

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

Actually looks like we already have stable const fn that opt into using conditionals, so just adding #[allow_internal_unstable(const_if_match)] should be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not fine. The stable const fns that use conditionals are just refactorings of hacks that were written without conditionals before, but they do not use more expressive power than what is available on stable, which, as far as I can tell, this would.

Copy link
Member

Choose a reason for hiding this comment

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

This would just use it for a debug assertion though, which we can remove again any time if we have to.

@CAD97 CAD97 deleted the patch-1 branch February 21, 2020 01:42
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Mar 13, 2020
Version 1.42.0 (2020-03-12)
==========================

Language
--------
- [You can now use the slice pattern syntax with subslices.][67712] e.g.
  ```rust
  fn foo(words: &[&str]) {
      match words {
          ["Hello", "World", "!", ..] => println!("Hello World!"),
          ["Foo", "Bar", ..] => println!("Baz"),
          rest => println!("{:?}", rest),
      }
  }
  ```
- [You can now use `#[repr(transparent)]` on univariant `enum`s.][68122] Meaning
  that you can create an enum that has the exact layout and ABI of the type
  it contains.
- [There are some *syntax-only* changes:][67131]
   - `default` is syntactically allowed before items in `trait` definitions.
   - Items in `impl`s (i.e. `const`s, `type`s, and `fn`s) may syntactically
     leave out their bodies in favor of `;`.
   - Bounds on associated types in `impl`s are now syntactically allowed
     (e.g. `type Foo: Ord;`).
   - `...` (the C-variadic type) may occur syntactically directly as the type of
      any function parameter.

  These are still rejected *semantically*, so you will likely receive an error
  but these changes can be seen and parsed by procedural macros and
  conditional compilation.

Compiler
--------
- [Added tier 2* support for `armv7a-none-eabi`.][68253]
- [Added tier 2 support for `riscv64gc-unknown-linux-gnu`.][68339]
- [`Option::{expect,unwrap}` and
   `Result::{expect, expect_err, unwrap, unwrap_err}` now produce panic messages
   pointing to the location where they were called, rather than
   `core`'s internals. ][67887]

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

Libraries
---------
- [`iter::Empty<T>` now implements `Send` and `Sync` for any `T`.][68348]
- [`Pin::{map_unchecked, map_unchecked_mut}` no longer require the return type
   to implement `Sized`.][67935]
- [`io::Cursor` now derives `PartialEq` and `Eq`.][67233]
- [`Layout::new` is now `const`.][66254]
- [Added Standard Library support for `riscv64gc-unknown-linux-gnu`.][66899]


Stabilized APIs
---------------
- [`CondVar::wait_while`]
- [`CondVar::wait_timeout_while`]
- [`DebugMap::key`]
- [`DebugMap::value`]
- [`ManuallyDrop::take`]
- [`matches!`]
- [`ptr::slice_from_raw_parts_mut`]
- [`ptr::slice_from_raw_parts`]

Cargo
-----
- [You no longer need to include `extern crate proc_macro;` to be able to
  `use proc_macro;` in the `2018` edition.][cargo/7700]

Compatibility Notes
-------------------
- [`Error::description` has been deprecated, and its use will now produce a
  warning.][66919] It's recommended to use `Display`/`to_string` instead.
- [`use $crate;` inside macros is now a hard error.][37390] The compiler
  emitted forward compatibility warnings since Rust 1.14.0.
- [As previously announced, this release reduces the level of support for
  32-bit Apple targets to tier 3.][apple-32bit-drop]. This means that the
  source code is still available to build, but the targets are no longer tested
  and no release binary is distributed by the Rust project. Please refer to the
  linked blog post for more information.

[37390]: rust-lang/rust#37390
[68253]: rust-lang/rust#68253
[68348]: rust-lang/rust#68348
[67935]: rust-lang/rust#67935
[68339]: rust-lang/rust#68339
[68122]: rust-lang/rust#68122
[67712]: rust-lang/rust#67712
[67887]: rust-lang/rust#67887
[67131]: rust-lang/rust#67131
[67233]: rust-lang/rust#67233
[66899]: rust-lang/rust#66899
[66919]: rust-lang/rust#66919
[66254]: rust-lang/rust#66254
[cargo/7700]: rust-lang/cargo#7700
[`DebugMap::key`]: https://doc.rust-lang.org/stable/std/fmt/struct.DebugMap.html#method.key
[`DebugMap::value`]: https://doc.rust-lang.org/stable/std/fmt/struct.DebugMap.html#method.value
[`ManuallyDrop::take`]: https://doc.rust-lang.org/stable/std/mem/struct.ManuallyDrop.html#method.take
[`matches!`]: https://doc.rust-lang.org/stable/std/macro.matches.html
[`ptr::slice_from_raw_parts_mut`]: https://doc.rust-lang.org/stable/std/ptr/fn.slice_from_raw_parts_mut.html
[`ptr::slice_from_raw_parts`]: https://doc.rust-lang.org/stable/std/ptr/fn.slice_from_raw_parts.html
[`CondVar::wait_while`]: https://doc.rust-lang.org/stable/std/sync/struct.Condvar.html#method.wait_while
[`CondVar::wait_timeout_while`]: https://doc.rust-lang.org/stable/std/sync/struct.Condvar.html#method.wait_timeout_while
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. 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. 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.