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 Option methods as const #76135

Merged
merged 4 commits into from
Sep 21, 2020
Merged

Conversation

CDirkx
Copy link
Contributor

@CDirkx CDirkx commented Aug 31, 2020

Stabilize the following methods of Option as const:

  • is_some
  • is_none
  • as_ref

These methods are currently const under the unstable feature const_option (tracking issue: #67441).
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.

Related: #76225

@rust-highfive
Copy link
Collaborator

r? @KodrAus

(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

@scottmcm scottmcm added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Sep 2, 2020
@jyn514 jyn514 added the A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. label Sep 3, 2020
@KodrAus
Copy link
Contributor

KodrAus commented Sep 4, 2020

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

@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. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. 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. 🔔

@josephlr
Copy link
Contributor

It seems like Option::iter() Option::flatten() and Option::transpose() could also be stabilized without needing to Drop anything.

@ecstatic-morse
Copy link
Contributor

Option::iter has been previously discussed. It's not very useful, since you can't do anything with it in a const context without const trait methods, which are unstable.

I believe (someone should verify this) that the other two would depend on the unstable const_precise_live_drops feature gate. They can (and should) be made unstably const, but cannot be stabilized at the moment.

@CDirkx
Copy link
Contributor Author

CDirkx commented Sep 13, 2020

I checked, even though nothing actually gets dropped inside flatten and transpose, const_precise_live_drops is required for the compiler to see this. You get error[E0493]: destructors cannot be evaluated at compile-time otherwise.

@josephlr
Copy link
Contributor

I believe (someone should verify this) that the other two would depend on the unstable const_precise_live_drops feature gate. They can (and should) be made unstably const, but cannot be stabilized at the moment.

This makes sense (example playground), and also resolves my comment here: #76136 (comment).

@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
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Sep 19, 2020
RalfJung added a commit to RalfJung/rust that referenced this pull request Sep 19, 2020
Stabilize some Option methods as const

Stabilize the following methods of `Option` as const:
 - `is_some`
 - `is_none`
 - `as_ref`

These methods are currently const under the unstable feature `const_option` (tracking issue: rust-lang#67441).
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).

Related: rust-lang#76225
@RalfJung
Copy link
Member

@bors rollup

@RalfJung
Copy link
Member

I think this caused the clippy test failure in #76907 (comment). The affected test, redundant_pattern_matching_const_result, mentions const option methods.

@bors r-

@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 Sep 19, 2020
@CDirkx
Copy link
Contributor Author

CDirkx commented Sep 19, 2020

Yeah I'm looking into it, Clippy has a special case to not suggest is_some, is_none, is_ok and is_err inside a const context (clippy#5724).

Now that this stabilization for Option and the related one for Result (#76136) were accepted, would it be okay if I opened a new PR that merges them and also fixes Clippy for the both of them by removing the special casing machinery?

Edit: see #76136 (comment), after #76136 gets merged the Clippy check can_suggest will be removed entirely in this PR.

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 20, 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
CDirkx and others added 2 commits September 20, 2020 22:42
Stabilize the following methods of `Option` as const:
 - `is_some`
 - `is_none`
 - `as_ref`

Possible because of stabilization of rust-lang#49146 (Allow if and match in constants).
Update the test `redundant_pattern_matching`: check if `is_some` and `is_none` are suggested within const contexts.
Removes `can_suggest` from as it is no longer used.
Reverts rust-clippy#5724.
@CDirkx
Copy link
Contributor Author

CDirkx commented Sep 20, 2020

With #76136 merged I updated the testcases and now completely removed can_suggest from Clippy, essentially just reverting the changes made in rust-clippy#5724.

@RalfJung
Copy link
Member

LGTM. @rust-lang/clippy could someone take a look at the clippy changes here?

@oli-obk
Copy link
Contributor

oli-obk commented Sep 21, 2020

@bors r=dtolnay,oli-obk

@bors
Copy link
Contributor

bors commented Sep 21, 2020

📌 Commit 43cba34 has been approved by dtolnay,oli-obk

@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 Sep 21, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 21, 2020
Rollup of 13 pull requests

Successful merges:

 - rust-lang#76135 (Stabilize some Option methods as const)
 - rust-lang#76628 (Add sample defaults for config.toml )
 - rust-lang#76846 (Avoiding unnecesary allocations at rustc_errors)
 - rust-lang#76867 (Use intra-doc links in core/src/iter when possible)
 - rust-lang#76868 (Finish moving to intra doc links for std::sync)
 - rust-lang#76872 (Remove DeclareMethods)
 - rust-lang#76936 (Add non-`unsafe` `.get_mut()` for `Unsafecell`)
 - rust-lang#76958 (Replace manual as_nanos and as_secs_f64 reimplementations)
 - rust-lang#76959 (Replace write_fmt with write!)
 - rust-lang#76961 (Add test for issue rust-lang#34634)
 - rust-lang#76962 (Use const_cstr macro in consts.rs)
 - rust-lang#76963 (Remove unused static_assert macro)
 - rust-lang#77000 (update Miri)

Failed merges:

r? `@ghost`
@bors bors merged commit c3abb82 into rust-lang:master Sep 21, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 21, 2020
ebroto pushed a commit to ebroto/rust-clippy that referenced this pull request Sep 21, 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: #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](rust-lang/rust#75463) and [PR#76135](rust-lang/rust#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
@CDirkx CDirkx deleted the const-option branch September 23, 2020 19:23
@dtolnay dtolnay self-assigned this Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-fn Area: const fn foo(..) {..}. Pure functions which can be applied at compile time. 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. 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.