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

Add Result::{ok, err, and, or, unwrap_or} as const #92385

Merged
merged 1 commit into from
Mar 8, 2022

Conversation

clarfonthey
Copy link
Contributor

@clarfonthey clarfonthey commented Dec 29, 2021

Already opened tracking issue #92384.

I don't think that this should actually cause any issues as long as the constness is unstable, but we may want to double-check that this doesn't get interpreted as a weird Drop bound even for non-const usages.

@rust-highfive
Copy link
Collaborator

r? @scottmcm

(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 Dec 29, 2021
@rust-log-analyzer

This comment has been minimized.

@clarfonthey
Copy link
Contributor Author

Aaaaaand now I find out why these are not marked as const. Okay.

@@ -643,7 +643,8 @@ impl<T, E> Result<T, E> {
/// ```
#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
pub fn ok(self) -> Option<T> {
#[rustc_const_unstable(feature = "const_result_option", issue = "92384")]
pub const fn ok(self) -> Option<T> {
Copy link
Contributor

@PatchMixolydic PatchMixolydic Dec 29, 2021

Choose a reason for hiding this comment

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

I'm not too familiar with the feature, but would adding a ~const Drop bound work here?

Suggested change
pub const fn ok(self) -> Option<T> {
pub const fn ok(self) -> Option<T>
where
E: ~const Drop,
{

Copy link
Contributor

@PatchMixolydic PatchMixolydic Dec 29, 2021

Choose a reason for hiding this comment

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

See also #91928, which constified most of Option's methods.

@clarfonthey
Copy link
Contributor Author

You're right; there is a way to make this work. Reopening.

@clarfonthey clarfonthey reopened this Dec 29, 2021
@rust-log-analyzer

This comment has been minimized.

@clarfonthey
Copy link
Contributor Author

Okay, so, a couple things.

  1. I marked const fn drop under the feature const_drop. Haven't opened a tracking issue yet because I'm not sure if the lang/libs folks want to commit to that.
  2. Right now, the destructor check is a bit overzealous; I have to manually drop values in order for it to only require the T: ~const Drop and E: ~const Drop bounds instead of the (larger) Self: ~const Drop bounds.

In general there should probably be just a tracking issue in general for what the semantics of ~const Drop would even be, since it seems like it shouldn't just be lumped in #67792 with all of the ~const stuff.

@rust-log-analyzer

This comment has been minimized.

@slightlyoutofphase
Copy link
Contributor

Have you tried writing the start of the None arms neither with x or _, but rather _x? Here's what I mean, specifically:

pub const fn ok(self) -> Option<T>
where
    E: ~const Drop,
{
    match self {
        Ok(x) => Some(x),
        Err(_x) => None,
    }
}

pub const fn err(self) -> Option<E>
where
    T: ~const Drop,
{
    match self {
        Ok(_x) => None,
        Err(x) => Some(x),
    }
}

I kind of feel like that might make it work without the drop() function needing to be directly involved at all.

@slightlyoutofphase
Copy link
Contributor

slightlyoutofphase commented Dec 29, 2021

I kind of feel like that might make it work without the drop() function needing to be directly involved at all.

Yeah, I was on to something here. The following works exactly as written, for example:

#![feature(const_fn_trait_bound, const_precise_live_drops, const_trait_impl)]

pub const fn ok<T, E>(res: Result<T, E>) -> Option<T>
where
    E: ~const Drop,
{
    match res {
        Ok(x) => Some(x),
        // Actually works without the leading `_` also,
        // but then you get an "unused variable" lint.
        Err(_x) => None,
    }
}

pub const fn err<T, E>(res: Result<T, E>) -> Option<E>
where
    T: ~const Drop,
{
    match res {
        // Actually works without the leading `_` also,
        // but then you get an "unused variable" lint.
        Ok(_x) => None,
        Err(x) => Some(x),
    }
}

const A: Result<u32, &str> = Ok(2);
const B: Result<u32, &str> = Err("Nothing here");
const C: Option<u32> = ok(A);
const D: Option<u32> = ok(B);
const E: Option<&str> = err(A);
const F: Option<&str> = err(B);

fn main() {
    assert_eq!(C, Some(2));
    assert_eq!(D, None);
    assert_eq!(E, None);
    assert_eq!(F, Some("Nothing here"));
}

Playground link for the above.

@scottmcm
Copy link
Member

I'm going to re-roll this to someone who will hopefully have more ~const Drop context.

r? rust-lang/libs

@clarfonthey clarfonthey force-pushed the const_option branch 3 times, most recently from f503949 to 15433ce Compare January 29, 2022 22:46
@clarfonthey clarfonthey changed the title Add Result::ok and Result::err as const Add Result::{ok, err, and, or, unwrap_or} as const Jan 29, 2022
@clarfonthey
Copy link
Contributor Author

clarfonthey commented Jan 29, 2022

Since this hasn't been CRed yet and I found that there were a few other methods like these, I decided to add them to the PR with the appropriate ~const Drop bounds. I don't think this should be an issue, but if you'd prefer to limit it to just ok and err, let me know.

cc @joshtriplett since you seem to have won the review lottery.

@rust-log-analyzer

This comment has been minimized.

@camelid camelid added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 3, 2022
@bors
Copy link
Contributor

bors commented Mar 6, 2022

📌 Commit 19645ac has been approved by fee1-dead

@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 Mar 6, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 6, 2022
Add Result::{ok, err, and, or, unwrap_or} as const

Already opened tracking issue rust-lang#92384.

I don't think that this should actually cause any issues as long as the constness is unstable, but we may want to double-check that this doesn't get interpreted as a weird `Drop` bound even for non-const usages.
@fee1-dead
Copy link
Member

@bors r-

Blocked on #93412

@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 Mar 6, 2022
@clarfonthey
Copy link
Contributor Author

clarfonthey commented Mar 7, 2022

Now that that's merged, I think this is ready to merge again? Let me know if there are any changes in the code and I can fix them, @fee1-dead.

also r? @fee1-dead since I guess you're taking this over now just kidding, only org members can do this

@fee1-dead
Copy link
Member

cc @oli-obk for potential blockers (otherwise r=me)

@oli-obk
Copy link
Contributor

oli-obk commented Mar 7, 2022

@bors r=fee1-dead

@bors
Copy link
Contributor

bors commented Mar 7, 2022

📌 Commit 19645ac has been approved by fee1-dead

@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 Mar 7, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 8, 2022
Add Result::{ok, err, and, or, unwrap_or} as const

Already opened tracking issue rust-lang#92384.

I don't think that this should actually cause any issues as long as the constness is unstable, but we may want to double-check that this doesn't get interpreted as a weird `Drop` bound even for non-const usages.
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Mar 8, 2022
Add Result::{ok, err, and, or, unwrap_or} as const

Already opened tracking issue rust-lang#92384.

I don't think that this should actually cause any issues as long as the constness is unstable, but we may want to double-check that this doesn't get interpreted as a weird `Drop` bound even for non-const usages.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 8, 2022
…askrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#91993 (Tweak output for non-exhaustive `match` expression)
 - rust-lang#92385 (Add Result::{ok, err, and, or, unwrap_or} as const)
 - rust-lang#94559 (Remove argument from closure in thread::Scope::spawn.)
 - rust-lang#94580 (Emit `unused_attributes` if a level attr only has a reason)
 - rust-lang#94586 (Generalize `get_nullable_type` to allow types where null is all-ones.)
 - rust-lang#94708 (diagnostics: only talk about `Cargo.toml` if running under Cargo)
 - rust-lang#94712 (promot debug_assert to assert)
 - rust-lang#94726 (:arrow_up: rust-analyzer)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e22331c into rust-lang:master Mar 8, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 8, 2022
@clarfonthey clarfonthey deleted the const_option branch March 8, 2022 16:01
yvt added a commit to r3-os/r3 that referenced this pull request Mar 10, 2022
This unstable library feature, which covers the `const fn` version of
`Result::{ok, error, and, or, unwrap_or}`, was added by
[rust-lang/rust#92385][1].

[1]: rust-lang/rust#92385
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. 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.