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 the "main" constructors of NonZero/Shared/Unique return Option #42959

Merged
merged 13 commits into from
Jul 26, 2017

Conversation

SimonSapin
Copy link
Contributor

Per discussion in #27730 (comment).

This is a breaking change to unstable APIs.

The old behavior is still available under the name new_unchecked. Note that only that one can be const fn, since if is currently not allowed in constant contexts.

In the case of NonZero this requires adding a new is_zero method to the Zeroable trait. I mildly dislike this, but it’s not much worse than having a Zeroable trait in the first place. Zeroable and NonZero are both unstable, this can be reworked later.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @sfackler (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@SimonSapin
Copy link
Contributor Author

CC @aturon @rust-lang/libs

@Mark-Simulacrum
Copy link
Member

cc @rust-lang/libs

@bors
Copy link
Contributor

bors commented Jun 29, 2017

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

@bors
Copy link
Contributor

bors commented Jun 30, 2017

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

@alexcrichton
Copy link
Member

It looks like almost all current instances were changed to new_unchecked, and to me that seems like it's data that this is the wrong default?

@SimonSapin
Copy link
Contributor Author

SimonSapin commented Jun 30, 2017

The were changed to new_unchecked because I made the renames with sed to keep things building, and it was the only constructor available before this PR. Looking at the diff, most users do not do a null/zero check themselves and so would use unwrap if there were changed to use the checked constructor. Whether unwrapping or using an unsafe block is preferable is debatable…

If it is indeed the wrong default, we can drop the two renaming commits and only add new_checked constructors.

@eternaleye
Copy link
Contributor

@SimonSapin: Well, can initializing a NonZero with a zero value lead to UB? If so, I'd strongly favor an unwrap over the possibility of UB in the compiler or stdlib - and the fact that the existing code is not doing checks sounds to me like the current default is the wrong one.

@SimonSapin
Copy link
Contributor Author

@eternaleye Correct, that’s why the unchecked constructor is unsafe fn. There’s lots of instances of using unsafe code in std for things that could be written without unsafe, though. As I said, it’s debatable.

For example in src/liballoc/linked_list.rs we have Shared::new_unchecked(Box::into_raw(node)). In this case it’s very easy to audit that the pointer is known to be non-null since it comes from a Box.

@shepmaster shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 30, 2017
@alexcrichton
Copy link
Member

I don't mind what's what here, but adding a new function that's the "default" and then moving all existing uses to the non-default one seems incoherent to me. I'd feel that we should fix that one way or another, but I don't particularly mind how.

@aidanhs aidanhs 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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 5, 2017
@aidanhs
Copy link
Member

aidanhs commented Jul 5, 2017

Hi @SimonSapin, have you had a chance to have a think about @alexcrichton's comment above?

@SimonSapin
Copy link
Contributor Author

Hi @aidanhs, I’ve had more than a chance. We’ve been having the same discussion in the tracking issue. I’ve listed three options corresponding to commits of this PR: #27730 (comment), Alex suggested "none of the above", and I replied with a lengthy analysis of call sites: #27730 (comment).

The TL;DR is that most calls can be made unnecessary if we add new APIs or modify unstable ones, and most remaining calls could either use the unsafe unchecked constructor or use the checked constructor with .unwrap(). Which is preferable is a subjective decision which couldn’t have been made so far because the safe constructors did not exist yet.

@aidanhs
Copy link
Member

aidanhs commented Jul 13, 2017

Visiting again for PR triage. I assume this PR is on hold until the discussion is resolved in the tracking issue you mention? Looks like it hasn't been updated for about a week now and the rest of the libs team has yet to comment on the fcp.

Just pondering whether it makes sense to close this PR until the questions are resolved over there - thoughts?

@SimonSapin
Copy link
Contributor Author

Yes, this is waiting on that discussion. Closing seems unproductive to me.

@SimonSapin
Copy link
Contributor Author

SimonSapin commented Jul 14, 2017

I’ve replaced some unsafe calls to new_unchecked by adding new safe APIs:

  • Box::into_unique(b: Box<T>) -> Unique<T>, similar to Box::into_raw
  • Conversions (with the From trait) to Shared<T> from: Unique<T>, &mut T, and &T
  • Conversions to Unique<T> from &mut T and &T
  • Conversions to NonZero<*const T> from &mut T and &T
  • Conversion to NonZero<*mut T> from &mut T

These are all #[unstable] for now.

SimonSapin added a commit to SimonSapin/nomicon that referenced this pull request Jul 14, 2017
@SimonSapin
Copy link
Contributor Author

Opened rust-lang/nomicon#32 which makes ./x.py test src/doc pass locally, but I suspect Travis-CI will fail because that commit is not in the upstream nomicon repository, for fetching in the submodule.

@bors
Copy link
Contributor

bors commented Jul 15, 2017

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

@bors
Copy link
Contributor

bors commented Jul 25, 2017

💔 Test failed - status-travis

@aidanhs
Copy link
Member

aidanhs commented Jul 26, 2017

@bors
Copy link
Contributor

bors commented Jul 26, 2017

⌛ Testing commit 0d1864b with merge 83b003f3c985efe1687df88519aefa932556ef5f...

@bors
Copy link
Contributor

bors commented Jul 26, 2017

💔 Test failed - status-appveyor

@SimonSapin
Copy link
Contributor Author

Looks like #43453 again.

@sfackler
Copy link
Member

@bors retry

@bors
Copy link
Contributor

bors commented Jul 26, 2017

⌛ Testing commit 0d1864b with merge 0af7cb5ada568f986c7a43436aa31bc20bba39ee...

@bors
Copy link
Contributor

bors commented Jul 26, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member

kennytm commented Jul 26, 2017

dist-i586-gnu-i686-musl, stage2-std test failed, unknown error.

Those x: remove y stdout comes from collections::hash::map::test_map::test_entry_take_doesnt_corrupt which is absent from the log, probably the cause of the error. Though there isn't much can be done besides retry, I think.

[01:18:59] �[m�[m�[32m�[1m     Running�[m build/x86_64-unknown-linux-gnu/stage2-std/i686-unknown-linux-musl/release/deps/std-fe2724eda844b081
[01:18:59] 
[01:18:59] running 802 tests
[01:18:59] test ascii::tests::inference_works ... ok
[01:18:59] test ascii::tests::test_eq_ignore_ascii_case ... ok
[01:18:59] test ascii::tests::test_is_ascii ... ok
[01:18:59] test ascii::tests::test_is_ascii_alphanumeric ... ok
[01:18:59] test ascii::tests::test_is_ascii_alphabetic ... ok
[01:18:59] test ascii::tests::test_is_ascii_control ... ok
[01:18:59] test ascii::tests::test_is_ascii_digit ... ok
[01:18:59] test ascii::tests::test_is_ascii_lowercase ... ok
[01:18:59] test ascii::tests::test_is_ascii_hexdigit ... ok
[01:18:59] test ascii::tests::test_is_ascii_punctuation ... ok
[01:18:59] test ascii::tests::test_is_ascii_graphic ... ok
[01:18:59] test ascii::tests::test_is_ascii_uppercase ... ok
[01:18:59] test ascii::tests::test_is_ascii_whitespace ... ok
[01:18:59] test ascii::tests::test_make_ascii_lower_case ... ok
[01:18:59] test ascii::tests::test_make_ascii_upper_case ... ok
[01:18:59] test ascii::tests::test_to_ascii_uppercase ... ok
[01:18:59] test ascii::tests::test_to_ascii_lowercase ... ok
[01:18:59] test collections::hash::bench::find_existing ... ok
[01:18:59] test collections::hash::bench::find_nonexisting ... ok
[01:18:59] test collections::hash::bench::get_remove_insert ... ok
[01:18:59] test collections::hash::bench::grow_by_insertion ... ok
[01:18:59] test collections::hash::bench::hashmap_as_queue ... ok
[01:18:59] test collections::hash::bench::new_drop ... ok
[01:18:59] test collections::hash::bench::new_insert_drop ... ok
[01:18:59] test collections::hash::map::test_map::test_adaptive ... ok
[01:18:59] test collections::hash::map::test_map::test_behavior_resize_policy ... ok
[01:18:59] test collections::hash::map::test_map::test_conflict_remove ... ok
[01:18:59] test collections::hash::map::test_map::test_clone ... ok
[01:18:59] test collections::hash::map::test_map::test_create_capacity_zero ... ok
[01:18:59] test collections::hash::map::test_map::test_drops ... ok
[01:18:59] test collections::hash::map::test_map::test_empty_entry ... ok
[01:18:59] test collections::hash::map::test_map::test_capacity_not_less_than_len ... ok
[01:18:59] test collections::hash::map::test_map::test_empty_iter ... ok
[01:18:59] test collections::hash::map::test_map::test_empty_remove ... ok
[01:18:59] test collections::hash::map::test_map::test_entry ... ok
[01:18:59] 0: remove 7
[01:18:59] 1: remove -2
[01:18:59] 2: remove -4
[01:18:59] 3: remove 2
[01:18:59] 6: remove 6
[01:18:59] 12: remove -7
[01:18:59] 15: remove 9
[01:18:59] 16: remove -6
[01:18:59] 18: remove -8
[01:18:59] 19: remove 5
[01:18:59] 24: remove 8
[01:18:59] 25: remove -10
[01:18:59] 26: remove 0
[01:18:59] 29: remove -9
[01:18:59] 46: remove 1
[01:18:59] 68: remove -1
[01:18:59] 79: remove -5
[01:18:59] test collections::hash::map::test_map::test_expand ... ok
[01:18:59] test collections::hash::map::test_map::test_eq ... ok
[01:18:59] �[m�[m�[31m�[1merror:�[m An unknown error occurred
[01:18:59] 
[01:18:59] To learn more, run the command again with --verbose.
[01:18:59] 
[01:18:59] 
[01:18:59] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "-j" "4" "--target" "i686-unknown-linux-musl" "--release" "--locked" "--color" "always" "--features" "panic-unwind jemalloc backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "std:0.0.0" "-p" "compiler_builtins:0.0.0" "-p" "rand:0.0.0" "-p" "panic_abort:0.0.0" "-p" "collections:0.0.0" "-p" "core:0.0.0" "-p" "unwind:0.0.0" "-p" "alloc:0.0.0" "-p" "alloc_system:0.0.0" "-p" "libc:0.0.0" "-p" "std_unicode:0.0.0" "--"
[01:18:59] expected success, got: exit code: 101
[01:18:59] 
[01:18:59] 
[01:18:59] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test --target i686-unknown-linux-musl --target i586-unknown-linux-gnu
[01:18:59] Build completed unsuccessfully in 1:17:07

@Mark-Simulacrum
Copy link
Member

@bors retry - probably #38618 musl segfault

@Mark-Simulacrum
Copy link
Member

@bors rollup

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jul 26, 2017
Make the "main" constructors of NonZero/Shared/Unique return Option

Per discussion in rust-lang#27730 (comment).

This is a breaking change to unstable APIs.

The old behavior is still available under the name `new_unchecked`. Note that only that one can be `const fn`, since `if` is currently not allowed in constant contexts.

In the case of `NonZero` this requires adding a new `is_zero` method to the `Zeroable` trait. I mildly dislike this, but it’s not much worse than having a `Zeroable` trait in the first place. `Zeroable` and `NonZero` are both unstable, this can be reworked later.
bors added a commit that referenced this pull request Jul 26, 2017
Rollup of 10 pull requests

- Successful merges: #42959, #43447, #43455, #43456, #43458, #43462, #43463, #43465, #43471, #43480
- Failed merges:
@bors bors merged commit 0d1864b into rust-lang:master Jul 26, 2017
@SimonSapin SimonSapin deleted the nonzero-checked branch July 26, 2017 23:50
jonhoo added a commit to jonhoo/arccstr that referenced this pull request Jul 27, 2017
hrektts added a commit to hrektts/c-linked-list that referenced this pull request Jul 28, 2017
This is a breaking change.
See a [PR](rust-lang/rust#42959).
Gankra added a commit to rust-lang/nomicon that referenced this pull request Aug 2, 2017
bors added a commit that referenced this pull request Aug 3, 2017
Update nomicon

(This should have been in #42959.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.