phobos icon indicating copy to clipboard operation
phobos copied to clipboard

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

Open rtbo opened this issue 3 years ago • 8 comments

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.

rtbo avatar May 06 '22 17:05 rtbo

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"

dlang-bot avatar May 06 '22 17:05 dlang-bot

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

pbackus avatar May 06 '22 18:05 pbackus

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.

dkorpel avatar May 06 '22 18:05 dkorpel

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.

rtbo avatar May 06 '22 21:05 rtbo

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?

dkorpel avatar May 06 '22 21:05 dkorpel

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

atilaneves avatar May 12 '22 08:05 atilaneves

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

pbackus avatar May 12 '22 13:05 pbackus

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.

rtbo avatar May 12 '22 21:05 rtbo