Skip to content

Conversation

workingjubilee
Copy link
Member

People keep making fun of this signature for being so gnarly.
Associated type bounds admit a much simpler scribbling.

r? @scottmcm

@rustbot rustbot added 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. labels Jun 11, 2024
@workingjubilee workingjubilee force-pushed the simplify-try-map-signature branch 2 times, most recently from d09382e to 1193cef Compare June 11, 2024 01:07
pub fn try_map<R>(
self,
f: impl FnMut(T) -> R,
) -> <R::Residual as Residual<[R::Output; N]>>::TryType
Copy link
Member Author

@workingjubilee workingjubilee Jun 11, 2024

Choose a reason for hiding this comment

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

This still-pretty-noisy signature cannot reuse ChangeOutputType without falling afoul of either the type_alias_bounds lint or having an even more grody <<T as Try>::Residual as Residual<blahblahblah>>::TryType.

One option that we do have that requires slightly more implementation work (changing the internals of the ones that use NeverShortCircuit, mostly) is this:

    #[unstable(feature = "array_try_map", issue = "79711")]
    pub fn try_map<R>(
        self,
        f: impl FnMut(T) -> R,
    ) -> impl Try<Output = [R::Output; N], Residual = R::Residual>
    where
        R: Try<Residual: Residual<[R::Output; N]>>,

Even further alternates might exist, but I think most of them involve sitting down for a good while and thinking very hard about how the traits relate to each other.

Copy link
Member

Choose a reason for hiding this comment

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

Returning impl Try isn't acceptable, IMHO, because it means that things like a.try_map(…).unwrap_or(…) don't work.

Copy link
Member Author

Choose a reason for hiding this comment

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

basically agreed, which is what makes me annoyed the type_alias_bounds lint fires on changing the type ChangeOutputType alias to match. like, that can't possibly be right, can it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I'm reciting this issue: #125709

pub fn try_map<F, R>(self, f: F) -> ChangeOutputType<R, [R::Output; N]>
pub fn try_map<R>(
self,
f: impl FnMut(T) -> R,
Copy link
Member

@fmease fmease Jun 11, 2024

Choose a reason for hiding this comment

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

Do we have a libs-api policy for using in APITs given the fact that the synthetic type param they get lowered to can't be specified and the fact that the majority of stdlib fns that take Fn*s use an explicit param for historical reasons?

Copy link
Member Author

Choose a reason for hiding this comment

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

The closure can be given type annotations.

Copy link
Member

@fmease fmease Jun 11, 2024

Choose a reason for hiding this comment

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

Right, but if one for whatever reason would like to force the closure to be non-capturing for example (quite an artificial one I know), the user can't do so via try_map::<_, fn(_) -> _> but needs to do (|…| …) as fn(_) -> _. I'm not talking about practical application here, I'm just curious about T-libs-api's stance on the use of APITs vs. explicit type params in the stdlib going forward (a topic I'm pretty sure has been discussed in the past).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ehn. I think that thinking about it in terms of "APITs" instead of "can someone actually write the damn code?" is a very compiler-brained way to look at it, and that any policy established shouldn't apply equally to a hypothetical random trait versus the impl Fn* traits.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's my experience that unnecessary things being in a type signature makes it harder to work with them because now it has to be given as a parameter at every single site, forever and ever, when you actually just wanted to specify the type of R.

Copy link
Member Author

Choose a reason for hiding this comment

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

T-libs-api's stance on the use of APITs vs. explicit type params in the stdlib going forward (a topic I'm pretty sure has been discussed in the past).

anyways, if T-libs-api has a policy on this, it would be news to me, and they should write it down in, er, the policy section of https://std-dev-guide.rust-lang.org

Copy link
Member

@scottmcm scottmcm Jun 11, 2024

Choose a reason for hiding this comment

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

@fmease Part of the reason for stabilizing turbofishing of generics on things with APITs was the flexibility that gives to libs-api. Things like rust-lang/libs-team#362 where not being able to turbofish something -- and then not forcing people to add _s -- gives nice flexibility for things.

TBH I find it extremely rare that there's a reason to turbofish an I: Iterator or F: Fn, and much prefer it being an APIT. I wish that https://doc.rust-lang.org/std/array/fn.from_fn.html had been impl FnMut instead, because from_fn::<u8, N, _>(…) is much worse than if it had been from_fn::<u8, N>(…).

But I don't think there's enough examples to make a libs-api pattern just yet.

@workingjubilee workingjubilee force-pushed the simplify-try-map-signature branch from 1193cef to d695544 Compare June 11, 2024 08:50
People keep making fun of this signature for being so gnarly.
Associated type bounds lend it a much simpler scribbling.
ChangeOutputType can also come along for the ride.
@workingjubilee
Copy link
Member Author

Redistributed some of the diff to ChangeOutputType with the allow discussed.

@scottmcm
Copy link
Member

Cool; I like where this ended up. It does change the signature, but it's an unstable method so any issues with that can be dealt with when (if) it comes up for stabilization.
@bors r+ rollup

Want to do Iterator::try_find too? That's the one I usually reference to make fun of it, not this one...

@bors
Copy link
Collaborator

bors commented Jun 12, 2024

📌 Commit d695544 has been approved by scottmcm

It is now in the queue for this repository.

@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 Jun 12, 2024
@bors bors merged commit 4de77b6 into rust-lang:master Jun 12, 2024
@rustbot rustbot added this to the 1.81.0 milestone Jun 12, 2024
@workingjubilee workingjubilee deleted the simplify-try-map-signature branch June 12, 2024 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

5 participants