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

Implement Default for Result<(), E> #86065

Closed
wants to merge 2 commits into from
Closed

Implement Default for Result<(), E> #86065

wants to merge 2 commits into from

Conversation

IQBigBang
Copy link

Implement the Default trait for Result<(), E>.
It makes sense for Result<(), E> to implement this as a counterpart of Some(T), because both Ok(()) and None signify the absence of a value.
It also goes hand in hand with the common Rust idiom of calling several functions which return a Result:

fn f() -> Result<(), Error> {
    foo()?;
    bar()?;
    baz();
    Ok(()) // <- Here, Ok(()) arguably is the "default".
}

@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 @scottmcm (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.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 6, 2021
@scottmcm scottmcm added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 6, 2021
@scottmcm
Copy link
Member

scottmcm commented Jun 6, 2021

Thanks for the PR, @IQBigBang! I think, however, that this change ought to have an RFC.

Trait implementations cannot go in as unstable (for technical reasons), so there's no opportunity to experiment with this before stabilization. And this is a controversial area, with many explorations of related features (such as rust-lang/rfcs#2120), so the extra discussion of motivations and tradeoffs would be valuable.

Some things an RFC might discuss:

  • When would this implementation be expected to be used? Is it for helping #[derive(Default)] on structs with a Result<(), _> field? Do you want to write Default::default() instead of Ok(())?
  • How does this interact with potential future features? For example, Esteban is working on an RFC that would allow struct Foo { x: Result<(), MyError> = Ok(()) }, which could solve the deriving case. Does this get way better if the default() function goes into the prelude? Would try{} blocks make it less useful? Or what if you could write const OK<E>: Result<(), E> = Ok(());?
  • Why is this the only reasonable implementation of the trait, as opposed to something which should have a named function or method? Would Err(Default::default()) also be reasonable? Or is the Ok side way better for some reason?

(Official libs folks: of course you should feel free to contradict me here if you disagree.)

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
   Compiling libc v0.2.93
   Compiling std v0.0.0 (/checkout/library/std)
   Compiling compiler_builtins v0.1.45
   Compiling unwind v0.0.0 (/checkout/library/unwind)
error: an `#[unstable]` annotation here has no effect
     |
     |
1382 | #[unstable(feature = "unit_result_default_impl", issue = "none")]
     |
     |
     = note: `#[deny(ineffective_unstable_trait_impl)]` on by default

error: aborting due to previous error

error: could not compile `core`
error: could not compile `core`

To learn more, run the command again with --verbose.
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "x86_64-unknown-linux-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/library/test/Cargo.toml" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
Build completed unsuccessfully in 0:01:09

@scottmcm
Copy link
Member

scottmcm commented Jun 6, 2021

I noticed you have an IRLO thread open at https://internals.rust-lang.org/t/suggestion-pre-rfc-implement-default-for-result/14843?u=scottmcm I'm going to close this for now, pending outcome of discussions that happen there and feedback from libs folks.

One thing to add to my previous post: you should probably get an informal "we'd be interested in considering that" from the libs team before putting in the work to draft an RFC, just to make sure that you don't spend the effort unnecessarily.

@scottmcm scottmcm closed this Jun 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

4 participants