Option::flatten by eopb · Pull Request #60256 · rust-lang/rust

eopb · 2019-04-25T11:08:53Z

This PR makes this possible.

assert_eq!(Some(6),Some(Some(6)).flatten());assert_eq!(Some(6),Some(Some(6)).into());

rust-highfive · 2019-04-25T11:08:56Z

r? @rkruppe

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

Centril · 2019-04-25T11:24:56Z

I think it would be preferable to add this as a method .flatten() since that is clearer wrt. intent and is better in terms of type inference. Moreover, we already have the other monadic operations and .flatten() already exists for Iterator so there's precedent there.

Assigning over to random t-libs member;

r? @alexcrichton

eopb · 2019-04-25T11:27:28Z

@Centril I agree, a flatten method would be a good idea. Do you think I should add both a flatten method and an Into implementation?

Centril · 2019-04-25T11:41:59Z

and an Into implementation?

Don't really have an opinion :)

Keruspe · 2019-04-25T12:29:35Z

You probably could use unwrap_or_default instead of matching, couldn't you?

Centril · 2019-04-25T12:35:10Z

You probably could use unwrap_or_default instead of matching, couldn't you?

That imposes a constraint T: Default.

What you can do instead is self.and_then(core::convert::identity),
since it holds that (>>= id) = join.

Centril · 2019-04-25T13:31:34Z

@ethanboxx once you're done with the changes, can you please squash them into a single commit?

rust-highfive · 2019-04-25T14:04:51Z

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (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.

travis_time:end:09cbb92c:start=1556199268327592591,finish=1556199270489011385,duration=2161418794
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout
Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
[00:30:00] Compiling chalk-macros v0.1.0
[00:30:04] Compiling humantime v1.2.0
[00:30:04] Compiling backtrace-sys v0.1.27
[00:30:04] Compiling miniz-sys v0.1.11
[00:30:04] error[E0283]: type annotations required: cannot resolve `std::option::Option<_>: std::convert::Into<std::option::Option<&str>>`
[00:30:04] |
[00:30:04] |
[00:30:04] 12 | .define("MINIZ_NO_STDIO", None)
[00:30:04] [00:30:04] error: aborting due to previous error
[00:30:04] [00:30:04] For more information about this error, try `rustc --explain E0283`.
---
travis_time:end:024dc3af:start=1556201087937006377,finish=1556201087942299209,duration=5292832
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0c329b53
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:0d75f2e1
$ cat ./obj/build/x86_64-unknown-linux-gnu/native

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)

eopb · 2019-04-25T14:06:50Z

@Centril I dont understand this error. Can you explain why this is failing?

Centril · 2019-04-25T14:26:30Z

@ethanboxx It seems to me you are getting a type inference error because you are giving the compiler too many options to pick from. Try removing the implementation and that should fix it.

rust-highfive · 2019-04-25T16:42:26Z

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (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.

travis_time:end:17acb560:start=1556204635745974475,finish=1556204637832352408,duration=2086377933
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout
Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
$ export GCP_CACHE_BUCKET=rust-lang-ci-cache
$ export AWS_ACCESS_KEY_ID=AKIA46X5W6CZEJZ6XT55
---
travis_time:start:test_assembly
Check compiletest suite=assembly mode=assembly (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:20:28] [01:20:28] running 9 tests
[01:20:28] iiiiiiiii
[01:20:28] [01:20:28] finished in 0.146
[01:20:28] travis_fold:end:test_assembly
---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-both (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[01:20:44] [01:20:44] running 121 tests
[01:21:09] .iiiii...i.....i..i...i..i.i.i..i.ii...i.....i..i....i..........iiii..........i...ii...i.......ii.i. 100/121
[01:21:14] i.i......iii.i.....ii
[01:21:14] [01:21:14] finished in 30.498
[01:21:14] travis_fold:end:test_debuginfo
---
[01:36:27] .................................................................................................... 1600/2309
[01:36:39] .................................................................................................... 1700/2309
[01:36:53] .................................................................................................... 1800/2309
[01:37:07] .................................................................................................... 1900/2309
[01:37:20] .......................F............................................................................ 2000/2309
[01:37:53] .................................................................................................... 2200/2309
[01:38:10] ................i................................................................................... 2300/2309
[01:38:11] .........
[01:38:11] failures:
[01:38:11] failures:
[01:38:11] [01:38:11] ---- option.rs - option::Option<Option<T>>::flatten (line 1424) stdout ----
[01:38:11] error[E0658]: use of unstable library feature 'option_flattening'
[01:38:11] --> option.rs:1426:23
[01:38:11] |
[01:38:11] 5 | assert_eq!(Some(6), x.flatten());
[01:38:11] |
[01:38:11] = note: for more information, see https://github.com/rust-lang/rust/issues/60258
[01:38:11] = note: for more information, see https://github.com/rust-lang/rust/issues/60258
[01:38:11] = help: add #![feature(option_flattening)] to the crate attributes to enable
[01:38:11] error[E0658]: use of unstable library feature 'option_flattening'
[01:38:11] --> option.rs:1429:20
[01:38:11] |
[01:38:11] |
[01:38:11] 8 | assert_eq!(None, x.flatten());
[01:38:11] |
[01:38:11] = note: for more information, see https://github.com/rust-lang/rust/issues/60258
[01:38:11] = note: for more information, see https://github.com/rust-lang/rust/issues/60258
[01:38:11] = help: add #![feature(option_flattening)] to the crate attributes to enable
[01:38:11] error[E0658]: use of unstable library feature 'option_flattening'
[01:38:11] --> option.rs:1432:20
[01:38:11] |
[01:38:11] |
[01:38:11] 11 | assert_eq!(None, x.flatten());
[01:38:11] |
[01:38:11] = note: for more information, see https://github.com/rust-lang/rust/issues/60258
[01:38:11] = note: for more information, see https://github.com/rust-lang/rust/issues/60258
[01:38:11] = help: add #![feature(option_flattening)] to the crate attributes to enable
[01:38:11] error: aborting due to 3 previous errors
[01:38:11] [01:38:11] For more information about this error, try `rustc --explain E0658`.
[01:38:11] For more information about this error, try `rustc --explain E0658`.
[01:38:11] thread 'option.rs - option::Option<Option<T>>::flatten (line 1424)' panicked at 'couldn't compile the test', src/librustdoc/test.rs:310:13
[01:38:11] [01:38:11] [01:38:11] failures:
[01:38:11] option.rs - option::Option<Option<T>>::flatten (line 1424)
[01:38:11] option.rs - option::Option<Option<T>>::flatten (line 1424)
[01:38:11] [01:38:11] test result: FAILED. 2297 passed; 1 failed; 11 ignored; 0 measured; 0 filtered out
[01:38:11] [01:38:11] error: test failed, to rerun pass '--doc'
[01:38:11] [01:38:11] [01:38:11] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "test" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "-p" "core" "--" "--quiet"
[01:38:11] [01:38:11] [01:38:11] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[01:38:11] Build completed unsuccessfully in 0:29:16
[01:38:11] Build completed unsuccessfully in 0:29:16
[01:38:11] make: *** [check] Error 1
[01:38:11] Makefile:48: recipe for target 'check' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0028eda4
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Thu Apr 25 16:42:20 UTC 2019
---
travis_time:end:0857dbe6:start=1556210542191347728,finish=1556210542245854899,duration=54507171
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:059e1a58
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:079e6d16
$ dmesg | grep -i kill

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)

eopb · 2019-04-25T18:11:31Z

@Centril Whats the best way to solve this new issue.

ghost · 2019-04-25T19:19:53Z

TL;DR: Add #![feature(option_flattening)] at the start of the doc test, and move the attributes so they'll be below the doc comment (right above the method).
The first change fixes the issue, while the second one is to conform to the established style.

Documentation tests are external to the crate they're testing, so the x.flatten() in your doc test works the same way as writing x.flatten() in your own crate.
However, Option::flatten is unstable, so you'd have to opt into it with the feature gate, so adding #[feature(option_flattening)] at the start is what you need to solve the issue.
Vec has some unstable methods as well, and you can use them as a reference to make sure you're doing everything right.

In other places in the standard library, the #[inline] and #[unstable] attributes are below the documentation comment, just above the method itself. @Centril, is the "doc comments above attributes" style official and documented somewhere? I'm asking you because you've commented here before (and are generally helpful 😄), but if you're not the right person to ask just redirect the question to whoever is.

eopb · 2019-04-26T11:30:46Z

@Centril @cogmover @Keruspe Thanks for all the invaluable help. Looks like everything is working now.

alexcrichton · 2019-04-26T14:37:26Z

Thanks for the PR @ethanboxx! I'm personally a bit wary on adding this method though since I feel like what it morally is striving to do is collapse any amount of inner options, but it doesn't handle, for example, Option<Option<Option<T>>> -> T. This definitely works for two layers deep, but I could see how ergonomically it's intended (and the name sort of implies) that it folds everything on the inside.

The Option type is also very core to Rust and additions to its API shouldn't be taken lightly, since now Rust programmers will have to recognize what .flatten() is doing when they see it in the wild (another method to keep in mind with Option). Now that of course doesn't mean we can't extend its API, just that we need to be careful in doing so!

Centril · 2019-04-26T14:44:35Z

@alexcrichton

but it doesn't handle, for example, Option<Option<Option<T>>> -> T. This definitely works for two layers deep, but I could see how ergonomically it's intended (and the name sort of implies) that it folds everything on the inside.

Note that this is the same for iter.flatten(). That's just how monadic join works. Is there a particular difference wrt. Option? Note that unnesting was also discussed in the tracking issue for Iterator::flatten: #48213.

eopb · 2019-04-26T15:54:38Z

@alexcrichton You are correct. My original intention was for it flatten all nested layers, however, it quickly became apparent that it was not possible. I have added extra documentation to clarify this in a similar way iter.flatten() is documented.

squashed commit

alexcrichton · 2019-04-29T15:14:44Z

Ok, seems reasonable to me!

@bors: r+

bors · 2019-04-29T15:14:45Z

📌 Commit fa7ba66 has been approved by alexcrichton

Option::flatten This PR makes this possible. ```rust assert_eq!(Some(6), Some(Some(6)).flatten()); assert_eq!(Some(6), Some(Some(6)).into()); ```

Centril · 2019-04-29T20:23:19Z

@bors rollup

@ghost

Rollup of 9 pull requests Successful merges: - #59946 (Fix equivalent string in escape_default docs) - #60256 (Option::flatten) - #60305 (hir: remove LoweredNodeId) - #60334 (Stabilized vectored IO) - #60353 (Add test not to forget resolved ICE) - #60356 (Stabilize str::as_mut_ptr) - #60358 (Clarify the short explanation of E0207) - #60359 (resolve: Consider erroneous imports used to avoid duplicate diagnostics) - #60360 (Add test case for labeled break in const assignment) Failed merges: r? @ghost

@elahn

Stabilize `Option::flatten` - PR: rust-lang#60256 - Tracking issue: rust-lang#60258 @elahn > I was trying to `flat_map()` and found `map().flatten()` does the trick. This has been on nightly for 4 months, can we stabilise it? @ethanboxx > @Centril Helped me get this merged. What is the stabilization process? @Centril > @ethanboxx I'd just file a PR to stabilize it and we'll ask T-libs to FCP. So here I am. I am was unsure what number to put in `since = "-"` so I copied what someone had done in a recent PR.

@elahn

Stabilize `Option::flatten` - PR: rust-lang#60256 - Tracking issue: rust-lang#60258 @elahn > I was trying to `flat_map()` and found `map().flatten()` does the trick. This has been on nightly for 4 months, can we stabilise it? @ethanboxx > @Centril Helped me get this merged. What is the stabilization process? @Centril > @ethanboxx I'd just file a PR to stabilize it and we'll ask T-libs to FCP. So here I am. I am was unsure what number to put in `since = "-"` so I copied what someone had done in a recent PR.

Add Result<Result<T, E>, E>::flatten -> Result<T, E> This PR makes this possible (modulo type inference): ```rust assert_eq!(Ok(6), Ok(Ok(6)).flatten()); ``` Tracking issue: rust-lang#70142 <sub>largely cribbed directly from <https://github.com/rust-lang/rust/pull/60256></sub>

rust-highfive assigned hanna-kruppe Apr 25, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 25, 2019

This comment has been minimized.
Sign in to view

Centril added needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. T-libs-api [DEPRECATED; DO NOT USE] relnotes Marks issues that should be documented in the release notes of the next release. labels Apr 25, 2019

Centril added this to the 1.36 milestone Apr 25, 2019

rust-highfive assigned alexcrichton and unassigned hanna-kruppe Apr 25, 2019

eopb mentioned this pull request Apr 25, 2019
Tracking issue for Option::flatten#60258
Closed

This comment has been minimized.
Sign in to view

eopb changed the title ~~Add flatten option to Option<Option<T>> with Into trait.~~Option::flattenApr 25, 2019

This comment has been minimized.
Sign in to view

Centril removed needs-fcp This change is insta-stable, or significant enough to need a team FCP to proceed. relnotes Marks issues that should be documented in the release notes of the next release. labels Apr 26, 2019

Centril removed this from the 1.36 milestone Apr 26, 2019

Add flatten option for Option<Option<T>>
fa7ba66
squashed commit

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 Apr 29, 2019

Centril mentioned this pull request Apr 29, 2019
Rollup of 9 pull requests #60377
Merged

bors merged commit fa7ba66 into rust-lang:masterApr 29, 2019

eopb mentioned this pull request Sep 24, 2019
Stabilize Option::flatten#64747
Merged

Nemo157 mentioned this pull request Mar 19, 2020
Add Result<Result<T, E>, E>::flatten -> Result<T, E> #70140
Merged

Uh oh!

Uh oh!

Conversation

eopb commented Apr 25, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Apr 25, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

Centril commented Apr 25, 2019

Uh oh!

eopb commented Apr 25, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Centril commented Apr 25, 2019

Uh oh!

This comment has been minimized.

Keruspe commented Apr 25, 2019

Uh oh!

This comment has been minimized.

Centril commented Apr 25, 2019

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Centril commented Apr 25, 2019

Uh oh!

rust-highfive commented Apr 25, 2019

Uh oh!

eopb commented Apr 25, 2019

Uh oh!

Centril commented Apr 25, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Apr 25, 2019

Uh oh!

eopb commented Apr 25, 2019

Uh oh!

ghost commented Apr 25, 2019

Uh oh!

eopb commented Apr 26, 2019

Uh oh!

alexcrichton commented Apr 26, 2019

Uh oh!

Centril commented Apr 26, 2019

Uh oh!

eopb commented Apr 26, 2019

Uh oh!

alexcrichton commented Apr 29, 2019

Uh oh!

bors commented Apr 29, 2019

Uh oh!

Centril commented Apr 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

eopb commented Apr 25, 2019•
edited
Loading

eopb commented Apr 25, 2019•
edited
Loading

Centril commented Apr 25, 2019•
edited
Loading