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 the ordering of invocations in Defer#fix #4252

Merged
merged 1 commit into from
Jun 24, 2022

Conversation

TimWSpence
Copy link
Member

@TimWSpence TimWSpence commented Jun 24, 2022

What we want is to evaluate fn and pass the recursive value lazily in case fn doesn't need to evaluate it ie fn(defer(res)).

defer(fn(res)) also works but it suspends the initial fn invocation, which is pointless as we must always evaluate fn at least once to produce a result

Don't unnecessarily defer the first function invocation
@armanbilge armanbilge changed the title Change the ordering of invocations in Defer#fix Change the ordering of invocations in Defer#fix Jun 24, 2022
@armanbilge
Copy link
Member

After being befuddled, here's an attempt to put it in my own words: the idea is we want to pass the lazily-evaluated defer(res) to the fn, rather than res itself.

A trivial example of where this obviously makes a difference:

Defer[F].fix[Unit] { _ => // discard it!
  F.unit
}

Under the old implementation, res would be strictly evaluated, before being passed to fn (which is discarding it anyway). Under the new implementation, instead defer(res) is passed to fn. Since defer(res) is lazy and is discarded, res is not evaluated. This eliminates a level of recursion.

Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Heh, forgot I can approve stuff here 😜

@johnynek
Copy link
Contributor

This arose from cats-parse where I had written a function this way (but not, strangely, the fix override).

The motivation there was that for parser you would always need to call the function. So if you know you must call it, lift it out and don't bury it in a level of recursion.

I'm not 100% sure there couldn't be F instances where this is worse but it seems like a better default to me, and if some F likes the other way better I think they can override.

@TimWSpence
Copy link
Member Author

This arose from cats-parse where I had written a function this way (but not, strangely, the fix override).

The motivation there was that for parser you would always need to call the function. So if you know you must call it, lift it out and don't bury it in a level of recursion.

I'm not 100% sure there couldn't be F instances where this is worse but it seems like a better default to me, and if some F likes the other way better I think they can override.

Sorry I meant to include this context in the description

@johnynek johnynek merged commit c6ed0e2 into typelevel:main Jun 24, 2022
@TimWSpence TimWSpence deleted the defer-fix branch June 24, 2022 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants