rfcs
rfcs copied to clipboard
ponylang
Introduce empty Ranges
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.
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.
This needs some more preparation time. I will reopen when I have a significantly improved version
The draft was completely reworked, incorporating as much as possible the comments that were previously given.
@stefandd I have added myself as a reviewer to remind myself to look at it this evening. I think Sean has been busy lately so I want to ensure you have eyes on it in case he is still busy.
I am very busy at work and have had limited time. I apologize for the slowness with which any review might come.
@stefandd I have added some comments to continue the conversation. Biggest update for my thinking is, after thinking on it more, convincing myself a parameter rearrangment approach is a bad fit for solving the same underlying problem that empty ranges intends to address (i.e., preventing unsuspecting infinite ranges).
I haven't had a chance to review this yet. Sorry, very busy with non-pony things. Please do not take that as a lack of interest.
@stefandd Excellent to see the rewrite! I will review this when I am home from work today. I really like/support the changes you are putting forward so I hope the rewrite clarifies points where I was previously confused.
I haven't had time to look at this at all. Apologies, long drawn out work project. I might have time in November, certainly not before. I will try to give this the time in deserves soon.
Actually, during sync, we covered this slightly. Joe gave me an overview, said he was in favor, and based on the overview, and his support, I am hereby giving my "I'm fine with this going through as others have said it is good".
Side note, Joe read the Drawbacks section out loud and the call and kudos on the drawbacks section. It was very very well written.
@rhagenson FYI, I would be wary of assuming that any docstrings are correct rather than someone (a volunteer) attempting to explain what they are seeing. I think consistency is good, if that happens to match the docstring, great! If not, I think we can also reasonably decide the doc string is wrong.
Sean, I presume your comment was due to my talking about the docstring for Reverse. I took the docstring to mean "Reverse is meant to be Range in reverse" I was not so much assuming it to be correct as deciding I agreed with the docstring as written that it should be that way.
If we go with the changes here as written without any change to Reverse then Range and Reverse would no longer be related in the same way and the docstring should change so as to not suggest a relationship that we know does not apply any longer.
We discussed this during sync and Joe and I both believe that if we agree with the statement that "reverse should be the opposite of range" then changes to Reverse should be part of this RFC.
I agree with Ryan's statement that Reverse should be the opposite of Range. Therefore, I think changes to reverse should be added to this RFC. That said, I think that it might turn out when trying to make Reverse the opposite of Range that the statement that it should be opposite is in fact, non-sensical.
@jemc noticed during sync that in terms of inclusivity, Range and Reverse are not opposites of each other. In general, it appears that Reverse isn't at all the opposite of Range. Across many factors
@kulibali raises the interesting point, "if Range can take a negative step to go backwards, why do we need Reverse"?
I think it would be reasonable to consider removing reverse as part of this RFC.
After additional conversation during sync, Joe, Gordan, and I have arrived on the following:
This RFC should be updated to remove Reverse.
If we find that using Range instead of Range turns out to be problematic, then an RFC specifically for adding a new Reverse. As part of that RFC, we can decide what characteristics we want from Reverse.
I am also in favor of retiring reverse as it is almost the same, e.g. Reverse(max, min, 2) is similar to Range[ISize](max, min-1, -2). For the sake of a best-outcome discussion, I still want to offer one argument for why someone could possibly want it to remain. Unlike Range it plays more nicely with the default unsigned integer types for descending series. For example, if you did:
use "collections"
actor Main
new create(env: Env) =>
for e in Reverse(10, 2, 2) do
env.out.print(e.string())
end
env.out.print("\n")
for e in Range(10, 1, -2) do
env.out.print(e.string())
end
it outputs
10
8
6
4
2
10
8
6
4
2
0
18446744073709551614
18446744073709551612
The reason is of course that -2 gets silently converted to USize. Still, I'd be in favor of a single Range class, especially since the current one and the one proposed here can deal with ascending and descending Ranges.
Note however that under the current proposal and because of this silent conversion to the default USize, the Range in the code example above would be empty, whereas currently Range(10, 1, -2) is one of these nasty indefinitely iterating Ranges I try to get rid of herewith. The above example nearly hung the Playground runtime...
There's no silent type conversions in Pony @stefandd. What do you mean by the term?
Ah, looking more I think you mean that Range is generic and it defaults to USize which seems somewhat problematic given that it is unsigned and we are going to have people using negative values. I think as part of this RFC, it would make sense to chanğe the default from USize to ISize.
Or perhaps, removing the default entirely. My first thought though is that ISize as a default is good.
@jemc thoughts?
I think there is a decent argument in favor of reverse being useful because it can represent a wider range of numbers to reverse through than is allowed by Range.
There's no silent type conversions in Pony @stefandd. What do you mean by the term?
I mean that a negative step parameter like -2 above is cast to a very large USize since USize is the default type of the generic Range type parameter [A: (Real[A] val & Number) = USize]. You are right that it isn't really silent. But because that cast happens and the way that Range adds the step whereas Reverse substracts it, one cannot get the same descending series as type USize. Instead, one would have to write Range[ISize](10, 1, -2) whereas Reverse(10, 2, 2) actually produces USize values.
However, I don't argue against the removal -- I wanted to merely point out that subtle difference!
There's no casting there @stefandd. It is actually a USize from the get-go, but it wraps around. As I detailed in my updated comment, I think that is quite problematic and that either there should be no default or it should be an ISize as the default. I agree that USize SHOULDNT be the default type for range and would not argue against there being no default.
@stefandd Even if we keep range, I think it is very important as part of this PR to either
- Not have a default type for Range so that you have to specify the type in question or...
- Set it to ISize, because USize is a horrible choice given the existence of negative step values as an option.
@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...
@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.
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.
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...