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 array_from_fn #94119

Merged
merged 1 commit into from
May 22, 2022
Merged

Conversation

c410-f3r
Copy link
Contributor

@c410-f3r c410-f3r commented Feb 18, 2022

Overall

Stabilizes core::array::from_fn and core::array::try_from_fn to allow the creation of custom infallible and fallible arrays.

Signature proposed for stabilization here, tweaked as requested in the meeting:

// in core::array

pub fn from_fn<T, const N: usize, F>(_: F) -> [T; N];

Examples in https://doc.rust-lang.org/nightly/std/array/fn.from_fn.html

History

  • On 2020-08-17, implementation was proposed.
  • On 2021-09-29, tracking issue was created.
  • On 2021-10-09, the proposed implementation was merged.
  • On 2021-12-03, the return type of try_from_fn was changed.

Considerations

  • It is being assumed that indices are useful and shouldn't be removed from the callbacks
  • The fact that try_from_fn returns an unstable type R: Try does not prevent stabilization. Although I'm honestly not sure about it.
  • The addition or not of repeat-like variants is orthogonal to this PR.

These considerations are not ways of saying what is better or what is worse. In reality, they are an attempt to move things forward, anything really.

cc #89379

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(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 Feb 18, 2022
/// let array = core::array::from_fn(|i| i);
/// assert_eq!(array, [0, 1, 2, 3, 4]);
/// ```
#[inline]
#[unstable(feature = "array_from_fn", issue = "89379")]
#[stable(feature = "array_from_fn", since = "1.61.0")]
pub fn from_fn<F, T, const N: usize>(mut cb: F) -> [T; N]
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we should change the generic arguments to

Suggested change
pub fn from_fn<F, T, const N: usize>(mut cb: F) -> [T; N]
pub fn from_fn<T, const N: usize, F>(mut cb: F) -> [T; N]

this makes it a lot more similar to other such functions with the function being last.

Copy link
Member

Choose a reason for hiding this comment

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

We might even consider going to

Suggested change
pub fn from_fn<F, T, const N: usize>(mut cb: F) -> [T; N]
pub fn from_fn<T, const N: usize>(mut cb: impl FnMut(usize) -> T) -> [T; N]

Because if the rules change to "you can turbofish the explicit generic parameters but not the APITs", then that would be nicer -- from_fn::<i32, N>(...) instead of ::<i32, N, _>.

That would, of course, mean that they'd need to be provided by context in the mean time, however. But TBH I prefer that anyway, as I find from_fn::<_, 4, _> distasteful. (The closure almost always provides the T.)

@@ -80,7 +76,7 @@ where
/// assert_eq!(array, None);
/// ```
#[inline]
#[unstable(feature = "array_from_fn", issue = "89379")]
#[stable(feature = "array_from_fn", since = "1.61.0")]
pub fn try_from_fn<F, R, const N: usize>(cb: F) -> ChangeOutputType<R, [R::Output; N]>
Copy link
Member

Choose a reason for hiding this comment

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

Note for libs-api: This would be the first stable usage of the https://doc.rust-lang.org/nightly/std/ops/trait.Residual.html mechanism (the other things like Iterator::try_find are still unstable) so it probably deserves extra careful consideration.

@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 Feb 26, 2022
@scottmcm
Copy link
Member

It is being assumed that indices are useful and shouldn't be removed from the callbacks

To hopefully help team discussion, I'll link my rationale in favour of usize -> T: #89379 (comment)

array::repeat_with might also be good (#89379 (comment)), but that doesn't need to keep us from stabilizing the takes-usize version. We can always have two constructor functions later if we want -- after all, we also have iter::from_fn and iter::repeat_with even though iter::repeat_with(|| f()) can always be written as iter::from_fn(|| Some(f())).

@scottmcm scottmcm added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Mar 18, 2022
@joshtriplett joshtriplett removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Apr 6, 2022
@joshtriplett
Copy link
Member

joshtriplett commented Apr 6, 2022

We discussed this in today's libs-api meeting. Out of an abundance of caution, since this needs the Try trait and the residual mechanism, we decided to not stabilize try_from_fn for now; we'd like to just stabilize from_fn first and defer try_from_fn until try is stabilized.

Separately from that, we'd like to see the arguments reordered to put the function type last, which will allow us to in the future omit that argument (with further language enhancements).

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Apr 6, 2022

The fact that R implements Try shouldn't be a problem because Iterator::try_for_each, Iterator::try_fold, and DoubleEndedIterator::try_rfold already return R: Try. Here also goes a list with a fraction of unstable things used in stable stuff:

Result<[T; N], E>, which was the original return, could also de used instead of R: Try. FWICT, Try is a superset of Result so I guess it is probably possible to replace an initial Result<[T; N], E> with a future R: Try.

With all that said, I ask the responsible party to reconsider the stabilization of try_from_fn with any desired return type. As previously stated, there are no ways to create fallible arrays without recurring to unsafe, boilerplate or external dependencies.

@joshtriplett
Copy link
Member

The fact that R implements Try shouldn't be a problem because Iterator::try_for_each, Iterator::try_fold, and DoubleEndedIterator::try_rfold already return R: Try

That's a compelling argument, to me.

@joshtriplett joshtriplett added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Apr 13, 2022
@cuviper
Copy link
Member

cuviper commented Apr 13, 2022

The fact that R implements Try shouldn't be a problem because Iterator::try_for_each, Iterator::try_fold, and DoubleEndedIterator::try_rfold already return R: Try.

Those stable examples don't use the Residual mapping though, which gives a sort of type-constructor conversion from R: Try to a related TryType. It's more like Iterator::try_collect, try_find, and try_reduce, which are all still unstable. Once we stabilize array::try_from_fn or any of those others, we're locked into needing that mechanism in Try.

(This was noted before in #94119 (comment))

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Apr 13, 2022

The fact that R implements Try shouldn't be a problem because Iterator::try_for_each, Iterator::try_fold, and DoubleEndedIterator::try_rfold already return R: Try.

Those stable examples don't use the Residual mapping though, which gives a sort of type-constructor conversion from R: Try to a related TryType. It's more like Iterator::try_collect, try_find, and try_reduce, which are all still unstable. Once we stabilize array::try_from_fn or any of those others, we're locked into needing that mechanism in Try.

(This was noted before in https://github.com/rust-lang/rust/pull/94119/files#r815334585)

The fact that Try is a nightly feature shouldn't lock any possible compromise but it seems that such acceptance could imply more future pseudo-"breakage".

Regardless, you indeed got a point so I withdraw my request for reconsidering try_from_fn. Thank you for all the participants.

@joshtriplett The PR will be updated according to #94119 (comment)

@joshtriplett joshtriplett removed the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Apr 13, 2022
@joshtriplett
Copy link
Member

@cuviper Thanks for catching that distinction.

@c410-f3r
Copy link
Contributor Author

The order of the type parameters was modified and the feature of try_from_fn was renamed to array_try_from_fn

@joshtriplett
Copy link
Member

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Apr 23, 2022

Team member @joshtriplett 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 Apr 23, 2022
@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 May 11, 2022
@rfcbot
Copy link

rfcbot commented May 11, 2022

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

@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 May 21, 2022
@rfcbot
Copy link

rfcbot commented May 21, 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 May 21, 2022
@c410-f3r
Copy link
Contributor Author

Someone has to r+

@scottmcm
Copy link
Member

Thanks! The updated generic order update is in, and the version matches the current one (after the bump last week).

@bors r+

@bors
Copy link
Contributor

bors commented May 22, 2022

📌 Commit d917112 has been approved by scottmcm

@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 May 22, 2022
@scottmcm scottmcm assigned scottmcm and unassigned joshtriplett May 22, 2022
@c410-f3r
Copy link
Contributor Author

Thanks! The updated generic order update is in, and the version matches the current one (after the bump last week).

@bors r+

Thanks @scottmcm

@bors
Copy link
Contributor

bors commented May 22, 2022

⌛ Testing commit d917112 with merge 09ea213...

@bors
Copy link
Contributor

bors commented May 22, 2022

☀️ Test successful - checks-actions
Approved by: scottmcm
Pushing 09ea213 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 22, 2022
@bors bors merged commit 09ea213 into rust-lang:master May 22, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 22, 2022
@bors bors mentioned this pull request May 22, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (09ea213): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results
  • Primary benchmarks: 😿 relevant regression found
  • Secondary benchmarks: mixed results
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 1 1 0 2 1
mean2 4.0% 1.4% N/A -2.2% 4.0%
max 4.0% 1.4% N/A -2.6% 4.0%

Cycles

Results
  • Primary benchmarks: mixed results
  • Secondary benchmarks: 😿 relevant regressions found
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 1 2 1 0 2
mean2 2.2% 3.1% -2.4% N/A -0.1%
max 2.2% 3.5% -2.4% N/A -2.4%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. number of relevant changes 2

  2. the arithmetic mean of the percent change 2

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. merged-by-bors This PR was explicitly merged by bors. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. 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.