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

Partially stabilize bound_as_ref by stabilizing Bound::as_ref #99736

Conversation

lopopolo
Copy link
Contributor

Stabilizing Bound::as_ref will simplify the implementation for RangeBounds<usize> for custom range types:

impl RangeBounds<usize> for Region {
    fn start_bound(&self) -> Bound<&usize> {
        // TODO: Use `self.start.as_ref()` when upstream `std` stabilizes:
        // https://github.com/rust-lang/rust/issues/80996
        match self.start {
            Bound::Included(ref bound) => Bound::Included(bound),
            Bound::Excluded(ref bound) => Bound::Excluded(bound),
            Bound::Unbounded => Bound::Unbounded,
        }
    }

    fn end_bound(&self) -> Bound<&usize> {
        // TODO: Use `self.end.as_ref()` when upstream `std` stabilizes:
        // https://github.com/rust-lang/rust/issues/80996
        match self.end {
            Bound::Included(ref bound) => Bound::Included(bound),
            Bound::Excluded(ref bound) => Bound::Excluded(bound),
            Bound::Unbounded => Bound::Unbounded,
        }
    }
}

See:

cc @yaahc who suggested partial stabilization.

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jul 25, 2022
@rustbot
Copy link
Collaborator

rustbot commented Jul 25, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive
Copy link
Collaborator

r? @thomcc

(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 Jul 25, 2022
@lopopolo lopopolo changed the title Partially stabilize bound_as_ref by stablizing Bound::as_ref Partially stabilize bound_as_ref by stabilizing Bound::as_ref Jul 25, 2022
@thomcc
Copy link
Member

thomcc commented Jul 25, 2022

Impl looks fine but this needs libs-api FCP.

@thomcc thomcc added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 25, 2022
@lopopolo
Copy link
Contributor Author

lopopolo commented Jul 25, 2022

Impl looks fine but this needs libs-api FCP.

oh whoops, it looks like I missed the magic comment rustbot asked me to make. Thanks @thomcc!

@yaahc
Copy link
Member

yaahc commented Jul 26, 2022

Looks good to me, @lopopolo could you also make a stabilization report1? Once we've got that I'll go ahead and kick off an FCP.

Footnotes

  1. https://std-dev-guide.rust-lang.org/feature-lifecycle/stabilization.html#stabilization-report

@lopopolo
Copy link
Contributor Author

Stabilization Report

This stabilization report is for a partial stabilization of the bound_as_ref feature. Tracking issue: #80996.

Implementation History

API Summary

This stabilization report proposes the following API to be stabilized:

impl<T> Bound<T> {
    pub fn as_ref(&self) -> Bound<&T>;
}

This new API has a motivating usecase to assist in the implementation of the RangeBounds<T> trait for custom range-like data types that store Bound internally.

Experience Report

Code such as this is ready to be converted to this API:

https://github.com/artichoke/artichoke/blob/0a19084bd1c7a8bd65e10e6b1ff99aedeec784ee/artichoke-backend/src/extn/core/matchdata/mod.rs#L54-L74

@dtolnay has remarked that Bound::as_ref is well-motivated given that RangeBounds<T> revolves around returning Bound<&T>: #80996 (comment).

@yaahc
Copy link
Member

yaahc commented Jul 26, 2022

thank you very much @lopopolo!

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Jul 26, 2022

Team member @yaahc 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. labels Jul 26, 2022
@lopopolo
Copy link
Contributor Author

lopopolo commented Aug 13, 2022

@yaahc did I do something wrong that has caused this proposed FCP to hang open? Do I need to update the since attribute since 1.63.0 has gone out?

@rfcbot rfcbot added 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 Aug 20, 2022
@rfcbot
Copy link

rfcbot commented Aug 20, 2022

🔔 This is now entering its final comment period, as per the review above. 🔔

@thomcc thomcc added S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 24, 2022
@est31
Copy link
Member

est31 commented Aug 27, 2022

👋 Hello, I'm writing this comment in this stabilization PR to notify you, the authors of this PR, that #100591 has been merged, which implemented a change in how features are stabilized.

Your PR has been filed before the change, so will likely require modifications in order to comply with the new rules. I recommend you to:

  1. rebase the PR onto latest master, so that uses of the placeholder are possible.
  2. replace the version numbers in the PR with the placeholder CURRENT_RUSTC_VERSION. For language changes, this means the version numbers in accepted.rs (example: 4caedba). For library changes, this means the since fields (example e576a9b).

That's it! The CURRENT_RUSTC_VERSION placeholder will, as part of the release process, be replaced with the version number that the PR merged for. It can be used anywhere in rust-lang/rust, not just accepted.rs and the since fields.

If you have any questions, feel free to drop by the zulip stream, or ping me directly in this PR's thread. Thanks! 👋

@lopopolo lopopolo force-pushed the lopopolo/gh-80996-partial-stabilization-bounds-as-ref branch from d4164f0 to 0820a54 Compare August 27, 2022 20:50
@lopopolo lopopolo force-pushed the lopopolo/gh-80996-partial-stabilization-bounds-as-ref branch from 0820a54 to 773df67 Compare August 27, 2022 20:51
@lopopolo
Copy link
Contributor Author

@est31 I think I got this sorted out. Thanks for making this change! It directly addresses my concern from #99736 (comment).

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Aug 30, 2022
@rfcbot
Copy link

rfcbot commented Aug 30, 2022

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Aug 30, 2022
@lopopolo
Copy link
Contributor Author

lopopolo commented Sep 2, 2022

@yaahc @joshtriplett can this be merged now that the FCP is concluded?

@dtolnay
Copy link
Member

dtolnay commented Sep 2, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Sep 2, 2022

📌 Commit 773df67 has been approved by dtolnay

It is now in the queue for this repository.

@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 2, 2022
@dtolnay dtolnay assigned dtolnay and unassigned thomcc Sep 2, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 3, 2022
Rollup of 7 pull requests

Successful merges:

 - rust-lang#99736 (Partially stabilize `bound_as_ref` by stabilizing `Bound::as_ref`)
 - rust-lang#100928 (Migrate rustc_metadata to SessionDiagnostics)
 - rust-lang#101217 ([drop tracking] Use parent expression for scope, not parent node )
 - rust-lang#101325 (Windows RNG: Use `BCRYPT_RNG_ALG_HANDLE` by default)
 - rust-lang#101330 (Fix `std::collections::HashSet::drain` documentation)
 - rust-lang#101338 (Fix unsupported syntax in .manifest file)
 - rust-lang#101348 (Cleanup css theme)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 2ed716a into rust-lang:master Sep 3, 2022
@rustbot rustbot added this to the 1.65.0 milestone Sep 3, 2022
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Sep 8, 2022
@lopopolo lopopolo deleted the lopopolo/gh-80996-partial-stabilization-bounds-as-ref branch September 25, 2022 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. S-waiting-on-fcp Status: PR is in FCP and is awaiting for FCP to complete. 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.

None yet

10 participants