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

Implement empty ranges RFC #4280

Merged
merged 10 commits into from
Jan 2, 2023
Merged

Implement empty ranges RFC #4280

merged 10 commits into from
Jan 2, 2023

Conversation

rhagenson
Copy link
Member

@rhagenson rhagenson commented Dec 20, 2022

Implement RFC-76: "Introduce Empty Ranges"

This commit additionally adds tests for known footguns. The additional tests are not detailed in the RFC.

Full RFC text is available at https://github.com/ponylang/rfcs/blob/main/text/0076-introduce%20empty%20ranges.md

Closes #4255

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Dec 20, 2022
@SeanTAllen SeanTAllen added do not merge This PR should not be merged at this time and removed discuss during sync Should be discussed during an upcoming sync labels Dec 20, 2022
@rhagenson
Copy link
Member Author

I found an instance of "footgunnery" in the "Examples of Affected Ranges" from the RFC

Range(0, 10, -1), .is_empty() == true, .is_infinite() == false

In actual Pony, this would be Range[A](0, 10, -1) where A is the type parameter. Well if A = USize then Range[USize](0, 10, -1) which wraps around to be Range[USize](0, 10, 18446744073709551615) which is not empty, nor infinite, but rather produces a single element of [0] -- this applies to all unsigned integers. Of course, then A is a float or signed, then the above does apply. I added additional tests to cover this.

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Dec 21, 2022
@SeanTAllen
Copy link
Member

@rhagenson that footgun was discussed on the RFC and was decided that it wouldn't be address at this time. That was left to a further RFC to sort out.

@rhagenson
Copy link
Member Author

I do not doubt it was discussed. Only instance from the list of affected ranges that I have found so far as being not quite the same based on the type parameter used so making the entry in the list misleading. I added tests and will need to note it in the docs that it is a possible tripping point.

@rhagenson rhagenson added the changelog - changed Automatically add "Changed" CHANGELOG entry on merge label Dec 22, 2022
@ponylang-main
Copy link
Contributor

Hi @rhagenson,

The changelog - changed label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do.

Release notes are added by creating a uniquely named file in the .release-notes directory. We suggest you call the file 4280.md to match the number of this pull request.

The basic format of the release notes (using markdown) should be:

## Title

End user description of changes, why it's important,
problems it solves etc.

If a breaking change, make sure to include 1 or more
examples what code would look like prior to this change
and how to update it to work after this change.

Thanks.

@rhagenson rhagenson marked this pull request as ready for review December 22, 2022 02:51
@rhagenson
Copy link
Member Author

I believe I have covered everything from the RFC. I added to the original scope only in additional tests and one sentence in the proposed docstring to note the wraparound when using an unsigned type and negative literal. I organized tests by the three classifications (finite, infinite, and empty) as well as removed an unused testing function (_count_range).

@@ -0,0 +1,5 @@
## Introduction of empty Ranges

Previously, a `Range` was considered infinite if either 1) the step is 0, or any of min, max, or step were NaN, +Inf or -Inf, or 2) if no progress could be made from min to max due to the sign of the step. This classification was a possible source of bugs including "hanging actors" when a `Range` would otherwise be reasonably expected to be **empty** -- producing no values. Through the RFC process the larger Pony community decided of the agreed-upon new mutually exclusive classification for what constitutes finite, infinite, and empty ranges.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it is important that this came through the RFC process at least as part of the release notes. If you can bring more information about what changed into the release notes, that would be good with perhaps an "additional information available here" type link to the text of the RFC.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this paragraph will mean anything to someone who hasn't read the RFC.

I'd like to see a large portion of the motivation section from the RFC written to the past tense and included as the bulk of the release notes to explain why a change was made and then the additional expansion of examples as noted below.

Copy link
Member Author

Choose a reason for hiding this comment

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

I took a crack at rewriting to keep it short, but increase the amount of "why we made the change" included in the release notes. Take a look when you can and tell me what you think. I was struggling with how to not simply repeat the entire motivation section with all its additional details that would, in my opinion, lead to as much old/outdated code as new/working code without very clear delineation.

@SeanTAllen
Copy link
Member

@rhagenson can you update the first comment to be the commit comment that should be used when squashing and merging this PR?

@SeanTAllen SeanTAllen merged commit d23fa2d into main Jan 2, 2023
@SeanTAllen SeanTAllen deleted the empty-ranges branch January 2, 2023 15:09
@ponylang-main ponylang-main removed the discuss during sync Should be discussed during an upcoming sync label Jan 2, 2023
github-actions bot pushed a commit that referenced this pull request Jan 2, 2023
github-actions bot pushed a commit that referenced this pull request Jan 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - changed Automatically add "Changed" CHANGELOG entry on merge do not merge This PR should not be merged at this time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: Introduction of Empty Ranges
4 participants