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

Avoid that std.range.generate calls fun one time too many #8453

Closed
wants to merge 1 commit into from

Conversation

rtbo
Copy link

@rtbo rtbo commented May 6, 2022

This is somehow a breaking change because if fun has side effects, the side effects won't be the same anymore.
However such side effects are arguably not expected as one would not expect that generate calls fun one time more than the returned range length.

In the proposed implementation, fun is lazily called in front and popFront would only invalidate the front value.

Fixes issue 19587.

avoid that std.range.generate calls
fun one time too many
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @rtbo! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
19587 enhancement std.range.generate's range calls its argument one time too many

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8453"

@rtbo rtbo changed the title fix issue 19587 Avoids that std.range.generate calls fun one time too many May 6, 2022
@rtbo rtbo changed the title Avoids that std.range.generate calls fun one time too many Avoid that std.range.generate calls fun one time too many May 6, 2022
@pbackus
Copy link
Contributor

pbackus commented May 6, 2022

The FreeBSD failure is an issue with the CI scripts. It should be fixed when #8454 is merged.

@dkorpel
Copy link
Contributor

dkorpel commented May 6, 2022

I'm not sure about this. I stumbled on this bug with generate!readln.take(n).array, finding out after debugging that n+1 lines were read, and then stopped using generate after learning about its flaw. On the other hand, I can imagine people working around it or expecting the current behavior somehow, and it will be very annoying for them to debug the issue when the behavior gets changed.

I'd say wait for Phobos v2, except the status of it isn't clear, so maybe it should take the same route as approxEqual -> isClose: Create a fixed version with a synonym, and deprecate the old function.

@rtbo
Copy link
Author

rtbo commented May 6, 2022

On the other hand, I can imagine people working around it or expecting the current behavior somehow, and it will be very annoying for them to debug the issue when the behavior gets changed.

Is it really a big deal to break compatibility with a flawed behavior that no one would expect?
generate is only useful if we supply non-pure function. This bug makes it not useful at all.

@dkorpel
Copy link
Contributor

dkorpel commented May 6, 2022

Is it really a big deal to break compatibility with a flawed behavior that no one would expect?

Not to me, but I hear complaints about D's lack of stability from others, and I can relate to them.

@atilaneves what do you think?

Copy link
Contributor

@pbackus pbackus left a comment

Choose a reason for hiding this comment

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

The documentation needs to be updated to reflect this change. It currently describes the old behavior:

The resulting range will call fun() on construction, and every call to
popFront, and the cached value will be returned when front is called.

It should instead describe the new behavior. For example:

The first time front is called for a given element, it will call fun() and cache the return value. Additional calls to front will return the cached value until popFront is called.

@atilaneves
Copy link
Contributor

I think that changing behaviour like this warrants a new function.

@pbackus
Copy link
Contributor

pbackus commented May 12, 2022

Maybe we can call the new version generator (similar to joiner and splitter).

@rtbo
Copy link
Author

rtbo commented May 12, 2022

Maybe we can call the new version generator (similar to joiner and splitter).

I've made a fixed copy of generate in my own project and called it hatch.

@LightBender LightBender added the Review:Phantom Zone Has value/information for future work, but closed for now label Oct 27, 2024
@LightBender
Copy link
Contributor

LightBender commented Oct 27, 2024

PR closed as stalled due to unimplemented requested changes. This can be resurrected for Phobos 3. @jmdavis this may be of interest to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review:Phantom Zone Has value/information for future work, but closed for now Severity:Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants