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.

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

Conversation

dabrahams
Copy link
Collaborator

@dabrahams dabrahams commented Nov 16, 2023

No description provided.

Fails due to

/Users/dave/src/hylo/Sources/IR/Emitter.swift:2321: Fatal error: lvalue lowering for cast expressions #1049
Tests fail with /Users/dave/src/hylo/Tests/LibraryTests/TestCases/RangeTests.hylo:13: precondition failure
@kyouko-taiga kyouko-taiga force-pushed the range-of-non-comparable branch from 99ea407 to d2038b3 Compare November 26, 2023 09:49
@dabrahams dabrahams requested a review from kyouko-taiga March 6, 2024 19:53
@@ -14,21 +14,29 @@ public type Range<Bound: SemiRegular & Comparable> {

/// 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

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

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.

Copy link
Contributor

@kyouko-taiga kyouko-taiga left a comment

Choose a reason for hiding this comment

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

I'm wondering if we're going in the right direction removing the requirement to Comparable (see inline comments) but otherwise the PR looks good.

I also left comments about explicit returns having been added.

Co-authored-by: Dimi Racordon <[email protected]>
@kyouko-taiga
Copy link
Contributor

Last time we discussed we agreed that this PR was going in the right direction but concluded that there were two concepts we should eventually specify. One is a range with the usual mathematical interpretation, that is an interval between two bounds that may or may not be included. The other is a pair of positions in a collection denoting a range of values in that collection, which we tentatively called PositionRange.

The bounds of PositionRange do not have to model Comparable (in fact, it will be common that they don't) but those of a traditional Range do.

Given this approach, I think this PR is actually proposing PropositionRange and if so I would suggest that it is modified accordingly (or closed in favor of another another PR).

@dabrahams
Copy link
Collaborator Author

Agreed. Let's just hold it open as a way to preserve that conclusion until the necessary PR is submitted.

@dabrahams dabrahams changed the title Remove Comparable requirement from Range bound We need a PositionRange type for ranges of collection positions. Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants