Skip to content

We need a PositionRange type for ranges of collection positions. #1143

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.

Sign up for GitHub

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

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from
Draft

We need a PositionRange type for ranges of collection positions. #1143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
595b197
Remove Comparable requirement from Range bound
dabrahams Nov 16, 2023
d2038b3
Use unsafe operations.
dabrahams Nov 17, 2023
58b62b6
Merge branch 'main' into range-of-non-comparable
dabrahams Mar 6, 2024
45c4bf6
Remove haxx
dabrahams Mar 6, 2024
915ef9f
Correct spello
dabrahams Mar 6, 2024
489ef5e
Flesh out tests.
dabrahams Mar 6, 2024
b859382
Merge remote-tracking branch 'origin/main' into range-of-non-comparable
dabrahams Mar 6, 2024
1b69469
Really try to fix the windows build.
dabrahams Mar 6, 2024
f1e2684
Merge branch 'main' into range-of-non-comparable
dabrahams Mar 6, 2024
ba807a5
Apply changes from code review
dabrahams Mar 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions StandardLibrary/Sources/Core/Range.hylo
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/// A half-open interval from a lower bound up to, but not including, an uppor bound.
public type Range<Bound: SemiRegular & Comparable> {
/// A half-open interval from a lower bound up to, but not including, an upper bound.
public type Range<Bound: SemiRegular> {

// TODO: private(set)
/// The minimum value included in `self`.
Expand All @@ -13,17 +13,24 @@ public type Range<Bound: SemiRegular & Comparable> {
memberwise init

/// Creates a half-open interval [`lower_bound`, `upper_bound`).
///
/// - Requires: `lower_bound <= upper_bound`.
/// - Requires: `lower_bound <= upper_bound` if `Bound` models `Comparable`.
Copy link
Contributor

@kyouko-taiga kyouko-taiga Mar 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I like this change but it prompted me with a question for which I don't have a formed opinion. Does it make sense to have a range of incomparable bounds?

The lower bound must be at least notionally less than the upper bound otherwise the abstraction is just an arbitrary pair. The reason why this requirement isn't enforced by a conformance requirement is that I'd like to avoid requiring Collection.Position to be Comparable and yet be able to write c[p0 ..< p1] for any collection c and positions p0, p1 therein (similar to C++ ranges). Of course, a Collection.Position must be at least notionally comparable to justify p0 ..< p1 making sense. Only the implementation of < may require access to the collection.

The sad part is that most algorithms require Bound to be comparable, as the rest of this incomplete implementation already demonstrates. So maybe a range ought to have comparable bounds and p0 ..< p1 when type(of: c).Position isn't Comparable ought to have a different type.

Outside the scope of a first release, another idea could be that traits could accept an implicit context so that we could say something along these lines:

conformance C.Position : Comparable using whole: C {
  public fun infix< (other: Self) -> Bool { whole.are_in_inreasing_order(self, other) }
}

Copy link
Collaborator Author

@dabrahams dabrahams Mar 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The lower bound must be at least notionally less than the upper bound otherwise the abstraction is just an arbitrary pair.

Sure, but possibly only contextually less. Consider ReverseCollection, which could in principle use the same index type as its Base (this does make me realize that the precondition is wrong—it can't even be upheld in a generic context: given two instances that might or might not be Comparable, you wouldn't know if you could form a Range).

is that I'd like to avoid

is that we'd like to avoid

We have always agreed about that. The cost to tree-structured collections of index comparability is too high.

a Collection.Position must be at least notionally comparable

Really, no. Comparable means “give me any two instances—and no other information—and I can tell you how they are ordered (or equal).” A classic linked list orders its positions, and the order is different from the meanings of those positions when they are treated as bare memory addresses.

All that said, I think there are at least three kinds of range-adjacent things, and using separate types to represent them could make sense:

  1. Ranges whose bound ordering is only relative to some context.
  2. #1 noitionally bundled with an ordering predicate.
  3. #2 that is also a collection (e.g. the Strideable Bound case).

(we also have the partial ranges x..., ...x, and ..<x, the first of which could be in any of the categories; the others are both of type 1)

We only want to use #1 in collection APIs.

If you want to call #2 a Range and find another term for #1, that's fine with me. 1-3 are in some sense a refinement hierarchy (it's possible that Bound comparability is not implied by being a collection, in which case we have a refinement diamond). I was going to say it's hard to imagine there being more than a single useful representation for #1, but a (position, length) pair is a useful range in its own right, and actually comes up pretty often in algorithms. Being forced to go through a (position, position) interface on these algorithms when what you have is a length means incurring an O(length) cost, and it's even common that the algorithm would not have to traverse all of length otherwise.

IMO a comprehensive design that sorts all these things out is an interesting problem, but until we've done that work we should start with this small modification to Swift's design that solves the problem of index comparability.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Separately, with regard to the future suggestion, though I don't know the details of what you have in mind, I can comment on what I'm guessing it means:

Aside from seeming to express something I think we don't want (even the collection shouldn't necessarily be able to act as an O(1) ordering predicate on its indices—and I don't think an O(n) predicate is useful), it looks like it introduces needless complexity. In this world Comparable appears to effectively be a multi-type trait, an idea I've found to be unneeded in practice. Essentailly it looks like you're making [value: C.Position, context: C] conform to a ContextuallyComparable trait, which is something that you can express entirely in terms of a trait bound on C.

But y'know, maybe I just don't understand it yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

100% agree with everything that you said.

public init(from lower_bound: sink Bound, up_to upper_bound: sink Bound) {
// precondition(lower_bound <= upper_bound)
&self.lower_bound = lower_bound
&self.upper_bound = upper_bound
}

/// Returns `true` iff `element` is contained in `self`.
public fun contains(_ element: Bound) -> Bool {
(element >= lower_bound) && (element < upper_bound)
/// Returns true iff `lower_bound == upper_bound`.
public fun is_empty() -> Bool {
lower_bound == upper_bound
}

}

public extension Range where Bound: Comparable {

/// Returns `true` iff `x` is contained in `self`.
public fun contains(_ x: Bound) -> Bool {
(x >= lower_bound) && (x < upper_bound)
}

/// Returns `true` iff `other` is contained in `self`.
Expand Down
15 changes: 14 additions & 1 deletion Tests/LibraryTests/TestCases/RangeTests.hylo
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//- compileAndRun expecting: success

public fun test_conformance_to_iterator() {
private fun test_conformance_to_iterator() {
var r = 1 ..< 3
var x: Int

Expand All @@ -14,6 +14,18 @@ public fun test_conformance_to_iterator() {
precondition(r.lower_bound == r.upper_bound)
}

private fun test_non_comparable() {
struct NonComparable: SemiRegular {
public memberwise init
let x: Int
}

var r0 = NonComparable(0) ..< NonComparable(0)
precondition(r0.is_empty())
var r1 = NonComparable(0) ..< NonComparable(1)
precondition(!r1.is_empty())
}

public fun main() {
let r0 = Range(from: 0, up_to: 10)

Expand All @@ -27,4 +39,5 @@ public fun main() {
precondition(!r0.contains(5 ..< 15))

test_conformance_to_iterator()
test_non_comparable()
}