Introduce empty Ranges

[stefandd]

No description provided.

[SeanTAllen]

I see in one of your commits since I first left comments that you added the following:

"It is argued here that based on using the name Range, it is expected to return an iterator producing a sequence of values that lie between the stated bounds. In the cases in question, Pony creates an iterator that produces a sequence of values that lie outside the provided bounds min and max."

I think that statement should be the core of your argument. That's an excellent point that is "entirely about Pony" that would justify a breaking change. Given you didnt find much code using this, you can't make a good "its easy to create bugs" based on empirical evidence But, you should be able to show code that the average reader of the RFC would expect to do one thing and you can show that it doesnt do that.

If you can demonstrate that, and show that there aren't surprising corner cases with the changes you propose, then I think it would be a change I could support wholeheartedly.

[SeanTAllen]

Given that performance is immediately after correctness in the Pony principles, it would be good to note if there are performance implications of the RFC changes.

[stefandd]

This needs some more preparation time. I will reopen when I have a significantly improved version

[stefandd]

@SeanTAllen I agree - having a default ISize for the Range type would be a logical remedy though it may then break more existing code since it is used heavily for indexing of Arrays which usually needs USize.

Yes, I am afraid this would be a very breaking change...

[SeanTAllen]

@stefandd I think that is reasonable break. The current default is, I think, downright awful. Really really bad. I think this is a good breakage.

A giant PITA breakage but it removes a serious issue with the API that you pointed out. I think that not having any default would be a good thing here as well.

[SeanTAllen]

I think the other thing that could be argued from this, if we don't want the breaking change is that Range shouldn't support negative decrementing and that should be the sole domain of an updated Reverse.

You are perhaps unintentionally bringing me around to that point of view @stefandd.

[stefandd]

You are perhaps unintentionally bringing me around to that point of view @stefandd.

Yes, that is why I felt it was important to point out the type assymetry, no matter what the final conclusion for this RFC will be. Also it is a good discussion to have...

[rhagenson]

I am on the side of having a Range that increments and a Reverse that decrements. Changing the default parameter to be ISize to allow Range to be used for both increment and decrement sounds like a change more out of convenience than principle.

[SeanTAllen]

I am on the side of having a Range that increments and a Reverse that decrements. Changing the default parameter to be ISize to allow Range to be used for both increment and decrement sounds like a change more out of convenience than principle.

I dont think it is convenience. I think the combination of API and that default choice is error prone. And "dont create error prone defaults" is a principle of mine.

[SeanTAllen]

Ok, so thinking about this. I think @stefandd / @rhagenson is something like this.

• Remove the footguns. All choices of type to Range or Reverse should give you unsurprising results. To do this we need to make the inc value not be of type A which could lead to weird footguns, but instead inc should always be positive and probably of a fixed type like ISize.

Except... well that doesn't work for floats. I can't use ISize with a Float that I want to increment by a non-integer value.

I dont offhand know how do support for integer types and floating point types without also having the foot guns except by having 2 different Range and Reverse implementations or... by having lots of runtime errors if you set things up poorly.

I'm very open to hearing ideas on how to address and if it is possible to remove the footguns or if the best we can do is have ISize rather than USize as the default.

So after looking at Reverse for a while, I think a way we could address the footguns is to "not allow negative increments" and then have interesting "if you do X" rules of getting some side effect. Something like... what reverse is currently doing:

  If `dec` is 0, produces an infinite series of `max`.

  If `dec` is negative, produces a range with `max` as the only value.

[SeanTAllen]

Another oddity of Reverse is that unlike Range, calling it when has_next is false, doesn't create an error.

[rhagenson]

I am on the side of having a Range that increments and a Reverse that decrements. Changing the default parameter to be ISize to allow Range to be used for both increment and decrement sounds like a change more out of convenience than principle.

I dont think it is convenience. I think the combination of API and that default choice is error prone. And "dont create error prone defaults" is a principle of mine.

I think the error prone default you had in mind is the one Stefan showed where, since the default parameter is USize, a -2 in effect is really adding USize.max_value()-2. Right?

If so, that was not what I was thinking about for the major error we were intending to solve. I was working from the idea of the type parameter determines the range of values that are possible and the method determines the direction. So if I want to have a range that includes signed integers I need an ISize, but if I only want positives then I want USize. I then assume the latter is more common based on how I have seen Range being used.

[SeanTAllen]

I am on the side of having a Range that increments and a Reverse that decrements. Changing the default parameter to be ISize to allow Range to be used for both increment and decrement sounds like a change more out of convenience than principle.

I dont think it is convenience. I think the combination of API and that default choice is error prone. And "dont create error prone defaults" is a principle of mine.

I think the error prone default you had in mind is the one Stefan showed where, since the default parameter is USize, a -2 in effect is really adding USize.max_value()-2. Right?

If so, that was not what I was thinking about for the major error we were intending to solve. I was working from the idea of the type parameter determines the range of values that are possible and the method determines the direction. So if I want to have a range that includes signed integers I need an ISize, but if I only want positives then I want USize. I then assume the latter is more common based on how I have seen Range being used.

Right but having documentation that says "USE NEGATIVE VALUES, its cool" and a default parameter that makes that negative value actually an integer underflow is just an awful confluence of items. We can't in good conscience drop Reverse if that is the API we are giving folks.

[rhagenson]

Absolutely.

I will have to sit down later today and collect my thoughts on the API that is "best" in my opinion. Not to get too far away from the original point of this RFC any change must include the empty ranges as that was fixing one possible footgun in the existing API.

[SeanTAllen]

Absolutely.

I will have to sit down later today and collect my thoughts on the API that is "best" in my opinion. Not to get too far away from the original point of this RFC any change must include the empty ranges as that was fixing one possible footgun in the existing API.

I think we should entertain getting away from the RFC. If this RFC is impetus for something different than it is right now, I think that is ok.

I also think that we could say, we are ok with this RFC "just as it is" and we approve it and then ask @stefandd or something else to tackle the "larger issues" we are addressing now.

[pmetras]

What about, like initially suggested by @stefandd, having the following signature:

Range[A: (Real[A] val & Number) = USize] is Iterator[A]

  new create(from: A, to: A, inc: A = 1, backward: Bool = false)

Advantages

• Compatible with existing code. Can still be used to index arrays and collections that use default USize.
• Can still be used with floats.
• Reverse becomes useless. Less code to maintain.
• No questions about increasing or decreasing ==> look at the backward value.

There's still a question though about the interval of the ranges' values: is it closed or open? If it is open, counting down from 10 to 1 with Range[USize](10, 0, 1, true) does not give the value 0. How would you write a Range[USize] to include the end value? The meaning of Range(10, USize.max_value(), 1, true) is not obvious.

[rhagenson]

@ponylang/committer @stefandd @pmetras

I do not see a clear consensus on a direction for unifying Range/Reverse functionality. Is anyone opposed to accepting this RFC as-is and having a discussion about further changes to Range/Reverse elsewhere (Zulip or open office hours both seem sensible to me) so we do not delay the closing of one source of error (unintended infinite ranges) which this RFC addresses?

[pmetras]

I agree that the RFP as it is solves a first problem. We can open another one later to decide about Reverse status.

[jemc]

My suggestion for resolving the Reverse asymmetry (and the fact that you can't currently go backward with an unsigned integer type in Range) would be:

• Remove the Reverse type
• Add a Range.reverse(...) constructor, which is symmetrical to the main constructor. This is how you'd go backward when using an unsigned integer type for the increment amount.

[jemc]

On the sync call we discussed that we want to leave the Reverse topic for another RFC. If anyone objects to this, feel free to comment within the next week. We think this is a reasonable iterative improvement without introducing any changes to Reverse.

We will vote on this during next week's sync call, unless the RFC author wants to make any further changes between now and then, or delay the vote based on feedback.

[pmetras]

I assume that acceptance of this RFP will invalidate this one: added range_redesign rfc.

[SeanTAllen]

This was accepted at the sync meeting. We still would like to see another RFC to address Reverse and various other issues.

[SeanTAllen]

RE: additional RFC for Reverse and Range, I've opened an "official" issue for that: #204