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

Stabilize some Result methods as const #76136

Merged
merged 5 commits into from
Sep 20, 2020
Merged

Conversation

CDirkx
Copy link
Contributor

@CDirkx CDirkx commented Aug 31, 2020

Stabilize the following methods of Result as const:

  • is_ok
  • is_err
  • as_ref

A test is also included, analogous to the test for const_option.

These methods are currently const under the unstable feature const_result (tracking issue: #67520).
I believe these methods to be eligible for stabilization because of the stabilization of #49146 (Allow if and match in constants) and the trivial implementations, see also: PR#75463 and PR#76135.

Note: these methods are the only methods currently under the const_result feature, thus this PR results in the removal of the feature.

Related: #76225

Stabilize the following methods of `Result` as const:
 - `is_ok`
 - `is_err`
 - `as_ref`

Possible because of stabilization of rust-lang#49146 (Allow if and match in constants).
@rust-highfive
Copy link
Collaborator

r? @cramertj

(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 Aug 31, 2020
@CDirkx
Copy link
Contributor Author

CDirkx commented Aug 31, 2020

r? @ecstatic-morse

@ecstatic-morse
Copy link
Contributor

r? @KodrAus (Needs FCP)

@CDirkx
Copy link
Contributor Author

CDirkx commented Sep 1, 2020

Created an issue for similar stabilizations: #76225

@jyn514 jyn514 added A-const-fn needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels Sep 3, 2020
@KodrAus
Copy link
Contributor

KodrAus commented Sep 4, 2020

For reference, there's also an equivalent push to constify some Option methods at #76135

@rfcbot fcp merge

@KodrAus KodrAus added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 4, 2020
@KodrAus
Copy link
Contributor

KodrAus commented Sep 4, 2020

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Sep 4, 2020

Team member @KodrAus 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 Sep 4, 2020
@rfcbot
Copy link

rfcbot commented Sep 7, 2020

🔔 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 Sep 7, 2020
@josephlr
Copy link
Contributor

josephlr commented Sep 12, 2020

There are some other methods we should also make const (either in this PR or a followup). They all have trivial match implementations and doesn't use any trait bounds:

Result::ok()
Result::err()
Result::iter()
Result::and()
Result::or()
Result::unwrap_or()
Result::transpose()

Result::flatten() also has a trivial impl, but that's unstable anyway (see #70142 (comment))

EDIT: Nevermind, these all require const fn Drop
EDIT2: Actually, we can implement transpose without dropping anything
EDIT3: Nevermind, transpose requires const_precise_live_drops, which isn't stable.

@jonas-schievink jonas-schievink added the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 13, 2020
@rfcbot rfcbot added the finished-final-comment-period The final comment period is finished for this PR / Issue. label Sep 17, 2020
@bors
Copy link
Contributor

bors commented Sep 17, 2020

⌛ Testing commit 787b270 with merge a51855f645eb6b86d1c6eb5656ae3394c9949a05...

@bors
Copy link
Contributor

bors commented Sep 17, 2020

💔 Test failed - checks-actions

@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 Sep 17, 2020
@spastorino spastorino removed the to-announce Announce this issue on triage meeting label Sep 17, 2020
@ecstatic-morse
Copy link
Contributor

@CDirkx Test failures are legit. Clippy has a lint that suggests is_err() instead of if let Err(_), but it's disabled in const fn. With this PR, that exception is no longer necessary and the test output can be updated.

Update the test `redundant_pattern_matching`: check if `is_ok` and `is_err` are suggested within const contexts.
Also removes the `redundant_pattern_matching_const_result` test, as it is no longer needed.
@CDirkx
Copy link
Contributor Author

CDirkx commented Sep 20, 2020

I updated the testcases, when this get's merged I'll update the testcases using Option as well and sync these changes to the Clippy repo.

@RalfJung
Copy link
Member

How can it be enough to update the testcases, doesn't the clippy code emitting these lints also need to be updated?

@ebroto
Copy link
Member

ebroto commented Sep 20, 2020

Clippy is checking if the suggested methods are const fn in const contexts, so it works without changing the logic because they are after this change.

But once the Option methods are stabilized as const fn the function can_suggest and calls to it should be removed as there would be no special cases left.

@CDirkx
Copy link
Contributor Author

CDirkx commented Sep 20, 2020

That's what I thought as well (#76135 (comment)), but the implementation of can_suggest was not specific to is_err etc., so at first I thought it could be used in the future if Clippy were to ever suggest other unstable const methods. However then it should probably be a more general pass and not a specific function that lives in matches.rs, so I'm indeed removing it.

I removed the can_suggest check for is_ok and is_err, but for now can_suggest is still used to not suggest the unstable const is_some and is_none from #76135, I will remove it entirely in that PR.

`is_ok` and `is_err` are stabilized as const and can thus always be suggested.
Co-authored-by: Ralf Jung <post@ralfj.de>
@RalfJung
Copy link
Member

@bors r=dtolnay

@bors
Copy link
Contributor

bors commented Sep 20, 2020

📌 Commit bf70e21 has been approved by dtolnay

@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 Sep 20, 2020
@bors
Copy link
Contributor

bors commented Sep 20, 2020

⌛ Testing commit bf70e21 with merge b873fa6...

@bors
Copy link
Contributor

bors commented Sep 20, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: dtolnay
Pushing b873fa6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 20, 2020
@bors bors merged commit b873fa6 into rust-lang:master Sep 20, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 20, 2020
@CDirkx CDirkx deleted the const-result branch September 20, 2020 16:03
flip1995 pushed a commit to flip1995/rust that referenced this pull request Sep 24, 2020
Stabilize some Result methods as const

Stabilize the following methods of Result as const:
 - `is_ok`
 - `is_err`
 - `as_ref`

A test is also included, analogous to the test for `const_option`.

These methods are currently const under the unstable feature `const_result` (tracking issue: rust-lang#67520).
I believe these methods to be eligible for stabilization because of the stabilization of rust-lang#49146 (Allow if and match in constants) and the trivial implementations, see also: [PR#75463](rust-lang#75463) and [PR#76135](rust-lang#76135).

Note: these methods are the only methods currently under the `const_result` feature, thus this PR results in the removal of the feature.

Related: rust-lang#76225
@dtolnay dtolnay self-assigned this Mar 24, 2024
@RalfJung RalfJung added the A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) label Dec 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: Constant evaluation, covers all const contexts (static, const fn, ...) 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. merged-by-bors This PR was explicitly merged by bors. 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.