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

[Do not merge] [stdlib] IncompleteRange prototype #3737

Closed
wants to merge 17 commits into from
Closed

[Do not merge] [stdlib] IncompleteRange prototype #3737

wants to merge 17 commits into from

Conversation

beccadax
Copy link
Contributor

What's in this pull request?

This is a prototype of new APIs from the not-yet-numbered Rationalizing Sequence end-operation names proposal. It includes:

  • The RangeExpression protocol and related refactoring of subrange handling
  • The IncompleteRange and IncompleteClosedRange types
  • The operators to construct IncompleteRange and IncompleteClosedRange
  • Cursory tests of some aspects of IncompleteRange and IncompleteClosedRange
  • Documentation comments

It does not include:

  • The removal of prefix(upTo:), prefix(through:), or suffix(from:)
  • Any of the other API renamings in that proposal
  • Thorough tests of RangeExpression, other than the ways it is exercised by existing, source-compatible tests

Known issues:

  • The test at test/Constraints/diagnostics.swift:725 (error message when attempting to use Int indices with a String) is failing because the diagnostics have changed. I have not updated the test because the new diagnostics are less helpful than the old ones.
  • The test at test/IDE/complete_enum_elements.swift:204 is also failing. I honestly don't know what this is testing for or why it would start to fail, so I don't know how to fix it.

Before merging this pull request to apple/swift repository:

  • Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform Comment
All supported platforms @swift-ci Please smoke test
All supported platforms @swift-ci Please smoke test and merge
OS X platform @swift-ci Please smoke test OS X platform
Linux platform @swift-ci Please smoke test Linux platform

A smoke test on macOS does the following:

  1. Builds the compiler incrementally.
  2. Builds the standard library only for macOS. Simulator standard libraries and
    device standard libraries are not built.
  3. lldb is not built.
  4. The test and validation-test targets are run only for macOS. The optimized
    version of these tests are not run.

A smoke test on Linux does the following:

  1. Builds the compiler incrementally.
  2. Builds the standard library incrementally.
  3. lldb is built incrementally.
  4. The swift test and validation-test targets are run. The optimized version of these
    tests are not run.
  5. lldb is tested.

Validation Testing

Platform Comment
All supported platforms @swift-ci Please test
All supported platforms @swift-ci Please test and merge
OS X platform @swift-ci Please test OS X platform
OS X platform @swift-ci Please benchmark
Linux platform @swift-ci Please test Linux platform

Lint Testing

Language Comment
Python @swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

@dabrahams
Copy link
Contributor

@swift-ci Please smoke test Linux platform

@beccadax
Copy link
Contributor Author

The smoke test failed because a ..< slice in swiftpm was too complex for the type checker. This particular case seems fixable with a minor expression redesign, but is this a concern for overloading ..<?

@dabrahams
Copy link
Contributor

@brentdax traditionally this means we file a bug against the typechecker and press forward.

@dabrahams
Copy link
Contributor

@brentdax Hey, I'm still very enthusiastic about this, including the unary range operators. Looks like RangeExpression is not quite enough where protocols are concerned, though; c.f. https://marc.ttias.be/swift-users/2016-10/msg00143.php, which is answered by

protocol CompleteRange {
  associatedtype Bound : Comparable
  var lowerBound : Bound { get }
  var upperBound : Bound { get }
}

extension CountableRange : CompleteRange {}
extension CountableClosedRange : CompleteRange {}

func random<R: CompleteRange>(from r: R) -> Int where R.Bound == Int, R: Collection {
  let rnd = arc4random_uniform(numericCast(r.count))
  return r.lowerBound + Int(rnd)
}

@beccadax
Copy link
Contributor Author

@dabrahams I don't think CompleteRange helps us with, for instance, subscripting on ClosedRange<String.Index>. If it doesn't do that, I don't think it's actually doing its original job.

The basic problem is that CompleteRange.upperBound has no consistent meaning, so it's useless. If someone ever added a FullOpenRange—e.g. 1<.<5 for [2,3,4]—its lowerBound would not work with random(from:) either. So I don't think CompleteRange is a good abstraction.

I suppose you could reconceptualize RangeExpression as being start-plus-count:

protocol RangeExpression {
  associatedtype Bound: Comparable
  func lowerBound<C: Collection>(in c: C) -> Bound where C.Index == Bound
  func distanceToUpperBound<C: Collection>(in c: C) -> C.IndexDistance where C.Index == Bound
}
extension Range: RangeExpression {
  func lowerBound<C: Collection>(in c: C) -> Bound where C.Index == Bound {
    return lowerBound
  }
  func distanceToUpperBound<C: Collection>(in c: C) -> C.IndexDistance where C.Index == Bound {
    return c.distance(from: lowerBound, to: upperBound)
  }
}
extension ClosedRange: RangeExpression {
  func lowerBound<C: Collection>(in c: C) -> Bound where C.Index == Bound {
    return lowerBound
  }
  func distanceToUpperBound<C: Collection>(in c: C) -> C.IndexDistance where C.Index == Bound {
    return c.distance(from: lowerBound, to: upperBound) + 1
  }
}
func random<R: RangeExpression, C: Collection>(from r: R, in c: C) -> C.Element where R.Bound == C.Index {
  let rnd = arc4random_uniform(numericCast(r.distanceToUpperBound(in: c))
  return c.index(r.lowerBound(in: c), offsetBy: C.IndexDistance(rnd))
}
func random<R: RangeExpression>(from r: R) -> R.Bound where R: Collection, R.Bound == R.Index {
  return random(from: r, in: r)
}

But that strikes me as a dirtier design than the "normalize to Range" design we've already talked about. It also loses the flexibility to be used for other things, like the relative range designs I've played with (where you can express ranges with endpoints like .startIndex + 1 and it builds up abstract descriptions of indices which are converted to real indices by relative(to:)).

Honestly, however, I think random(from:) is probably best thought of as a Collection operation:

func random<C: Collection>(from c: C) -> C.Element {
  let offset = C.IndexDistance(arc4random_uniform(numericCast(c.count)))
  return c[c.index(c.startIndex, offsetBy: offset)]
}

This can be used with a countable Range of either variety, but it can also be used with any other Collection, including the indices of a Collection, a slice of a Collection, or a slice of the indices of a Collection.

If so, the random(from:) case isn't really something we need to worry about.


I've been experimenting and thinking about this for the last couple months, and I'm actually thinking Range might not be the best return value for relative(to:). Ideally, I think we want to return a Collection so you can loop over the indices in the range. And CountableRange isn't right, because the indices actually used by the collection may not match the result of striding by 1. So I think the right design for RangeExpression might be one that returns a slice of the indices:

// Note: I'm going to assume that the eventual design is that all collections should use 
// DefaultIndices, and the current C.Indices is only there because conditional conformances
// are needed to eliminate Default{Bidirectional,RandomAccess}Indices.
// 
// If this assumption is incorrect, this design may have to become a little more convoluted, 
// particularly as regards the recursion anchoring I mention below.
protocol IndexSlice {
  associatedtype Bound: Comparable
  func makeIndices<C: Collection>(in c: C) -> DefaultIndices<C> where C.Index == Bound
}
extension Range: IndexSlice {
  func makeIndices<C: Collection>(in c: C) -> DefaultIndices<C> where C.Index == Bound {
    // Note: We'd need a `DefaultIndices.subscript(_: Range<Index>)` to anchor the recursion.
    // Perhaps it would have a different name or be an initializer; doesn't really matter.
    return c.indices[lowerBound..<upperBound]
  }
}
extension ClosedRange: IndexSlice {
  func makeIndices<C: Collection>(in c: C) -> DefaultIndices<C> where C.Index == Bound {
    return c.indices[lowerBound..<c.index(after: upperBound)]
  }
}
// etc.

On the Collection side, you'd see this:

protocol Collection {
  ...
  // Implements slicing; you'll use indexSlice.startIndex and .endIndex to figure out what's what.
  subscript(_ indexSlice: DefaultIndices<C>) -> SubSequence { get }
}
extension Collection {
  // Presumably, this would temporarily be implemented by GYB instead of generics.
  subscript<IS: IndexSlice>(_ range: IS) -> SubSequence where IS.Bound == Index {
    return self[range.makeIndices(in: self)]
  }
}
// Similar for `MutableCollection`, `RangeReplaceableCollection`, et.al.

However, I may be overthinking this, and we'd be better off with just a Range; if you want to iterate, you can always use a RangeExpression to subscript indices yourself. Any thoughts?

@dabrahams
Copy link
Contributor

@brentdax I agree with you that c.randomElement() is a much better way to express this and we don't need to worry about random(from:).


With regard to looping over indices in the range, can't you just use the
IncompleteRange to slice indices?

let i = a.index(a.startIndex, offsetBy: 3)
for x in a.indices[i...] {
   ...
}

remember, relative(to:) is just a part of the mechanics of the system;
I don't expect anyone to actually use it directly.

// Note: I'm going to assume that the eventual design is that all collections should
use
// DefaultIndices, and the current C.Indices is only there because conditional
conformances
// are needed to eliminate Default{Bidirectional,RandomAccess}Indices.

Why do you think all collections should use DefaultIndices? It's not
out of the question, and I don't remember why we didn't do something
along these lines, but I am assuming that if it were feasible, all
Indices would be one of the DefaultXXXIndices. Do we want
(3..<11).indices to stop being a CountableRange?

However, I may be overthinking this, and we'd be better off with just a Range; if
you want to iterate, you can always use a RangeExpression to subscript indices
yourself.

That was my first thought.

@beccadax beccadax closed this May 7, 2017
@beccadax beccadax deleted the incomplete-range branch May 7, 2017 04:50
@beccadax
Copy link
Contributor Author

beccadax commented May 9, 2017

This has been obsoleted by SE-0172, which adopted a different design for the same feature.

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