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

Change array_from_fn signatures #90505

Closed
wants to merge 1 commit into from

Conversation

c410-f3r
Copy link
Contributor

@c410-f3r c410-f3r commented Nov 2, 2021

Superseded by #91286

Implementation

These functions are using the same inner code of other array methods and such feature already ensures a reliable execution.

Signature

Originally, indices returned by the callback were introduced as auxiliary parameters since arrays are hereditary indexed and known at compile-time.

But due to #75644 (comment) and #75644 (comment), no index will be returned to avoid further concerns or discussions.

@rust-highfive
Copy link
Collaborator

r? @dtolnay

(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 Nov 2, 2021
@c410-f3r
Copy link
Contributor Author

c410-f3r commented Nov 2, 2021

@rustbot label +T-libs
r? @yaahc

@rust-highfive rust-highfive assigned yaahc and unassigned dtolnay Nov 2, 2021
@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Nov 2, 2021
@crlf0710 crlf0710 added the needs-fcp This change is insta-stable, so needs a completed FCP to proceed. label Nov 2, 2021
library/core/src/array/mod.rs Outdated Show resolved Hide resolved
@CryZe
Copy link
Contributor

CryZe commented Nov 2, 2021

I've seen that nowadays try_ functions return R: core::ops::Try<Output = T> instead. I think try_from_fn should probably follow that. E.g. Iterator::try_for_each

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Nov 9, 2021

After a week without comments, I am assuming that using the Try trait is indeed the way to go.

@c410-f3r c410-f3r force-pushed the here-we-go-again branch 3 times, most recently from 397d610 to 5167fd6 Compare November 9, 2021 13:00
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I am not prepared to stabilize this instantly with the signature change. This needs to go into nightly with the new signature so that we can get a picture of whether this gives rise to inconvenient inference issues in practical use, or anything else that may come up.

@c410-f3r
Copy link
Contributor Author

I am not prepared to stabilize this instantly with the signature change. This needs to go into nightly with the new signature so that we can get a picture of whether this gives rise to inconvenient inference issues in practical use, or anything else that may come up.

Sigh...

Done

@c410-f3r c410-f3r changed the title Stabilize array_from_fn feature Change array_try_from signatures Nov 16, 2021
@c410-f3r c410-f3r changed the title Change array_try_from signatures Change array_from_fn signatures Nov 16, 2021
@scottmcm
Copy link
Member

whether this gives rise to inconvenient inference issues in practical use

The usual problem is that an inferred return type is not compatible with ?.

I copied over the function signature here into the playground https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=0cdced05401eea1621827f86537ccd18, and sure enough, inference failures:

error[E0284]: type annotations needed
  --> src/lib.rs:17:9
   |
17 |         try_from_fn(|| -> Result<i32, TryFromIntError> { i32::try_from(i) })?,
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot infer type
   |
   = note: cannot satisfy `<_ as Try>::Residual == _`

And, importantly, that's happening despite the explicitly-specified closure type and a fully available return type (no try blocks or IIFEs or similar to confuse it).

That ? usage, however, compiles fine with the current nightly try_from_fn: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=b42ef33453354dce78f3299a1a40809a

It also compiles fine with the approach in #91286: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=3bc991d03ff52c972fc4c3b6dd02c46e

(Sorry for not seeing this PR sooner.)

@c410-f3r
Copy link
Contributor Author

The suddenly and surprising depreciation of this PR over #91286 was unexpected but I don't think anyone could do better than the very person who elaborated and implemented the new try infrastructure

Superseded by #91286

@c410-f3r c410-f3r closed this Nov 27, 2021
@scottmcm
Copy link
Member

Thanks for continuing to push these forward, @c410-f3r -- they're critical building blocks and I'd love to see them in stable.

@dtolnay dtolnay assigned dtolnay and unassigned yaahc Mar 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants