[Docs] Improve docs for UnsafeBoundsCheckedBufferPointer

Sources/WebURL/Util/Pointers.swift

@@ -150,78 +150,175 @@ extension UnsafeBufferPointer {
   }
 }
 
-/// A re-imagined contiguous buffer type, which attempts to protect against buffer over-reads in release builds with a negligible performance hit, and in some
-/// cases even an overall performance _improvement_ over the standard library's `UnsafeBufferPointer`.
+// Note: This documentation is intended to be very thorough, because I assume that anybody who sees a library
+//       which includes its own buffer-pointer type is (rightly) going to be sceptical.
+//       I would be sceptical, too.
+
+/// A re-imagined contiguous buffer type, which introduces bounds-checking to protect against buffer over-reads
+/// even in release builds. The implementation is carefully tuned to allow the compiler to eliminate bounds checks
+/// and other overheads more easily.
+///
+/// The idea behind this type is that we represent a buffer as a base pointer and (unsigned) size.
+/// The indexes are also unsigned integers, although the buffer's size is limited to `Int.max`.
+/// This gives us a couple of benefits:
+///
+/// 1. It is impossible to address locations before the base pointer.
+///    In order to bounds-check a read at some index `i`, we only need to check that `i < endIndex`.
+///
+/// 2. We can easily convert between signed and unsigned integers for arithmetic using `(U)Int(bitPattern:)`.
+///    The bit-pattern of a negative `Int` will be interpreted as a `UInt` whose magnitude is `> Int.max`,
+///    so it will automatically be an invalid index.
+///
+/// The type is also self-slicing (for code size in both respects - to limit the amount of code that needs to be audited
+/// by humans, and the number of generic specialisations that get generated by the compiler).
+///
+/// Slices are really interesting because they are used _a lot_ in complex Collection algorithms,
+/// and could complicate our approach to bounds-checking. There's a whole sub-section about this, but essentially:
+///
+/// 1. Slices keep the same base pointer as the parent.
+///
+/// 2. Since only the upper-bound (the slice's `endIndex`) is used to ensure memory safety,
+///    that's the only part we check in release builds. It must always be `<=` to the parent's `endIndex`,
+///    so the maximum addressable offset from the base pointer can only go down (or stay the same).
+///
+/// 3. This means that we can keep our simplified `i < endIndex` bounds-check, even for slices.
 ///
-/// The idea behind this type is that we start with a buffer, in the form of a base pointer and (unsigned) size, `buffer_size`.
-/// The indices, which are offsets from the pointer, are also unsigned integers; meaning they can never be negative, and bounds-checking can be simplified
-/// to `index < buffer_size`.
-/// The type is self-slicing, and the only rule to check that a slice's bounds are safe is that the maximum addressable offset must never increase over the slice's base.
-/// Since indices are unsigned, all offsets up to the maximum addressable offset are within the bounds of the original buffer, and hence safe to access.
 ///
 /// ## What don't we need to do?
 ///
-/// The thing to remember is that we're not under any obligation to detect misuses of the `Collection` protocol.
-/// If you have a buffer of 10 elements, and you increment its `startIndex` 1000 times, it may trigger a runtime error, or it may well return a garbage result index.
-/// And if you take that garbage result and pass it in to `distance(from: Index, to: Index)`, it may also trigger a runtime error - or it may not;
-/// it's perfectly valid for it to return a garbage number of elements instead. And garbage means garbage - for any reason - it doesn't matter if your index is beyond
-/// the bounds of the collection, or if the calculation overflows; it's all equally garbage and you shouldn't expect a meaningful result.
-/// You may, by some happy accident, land at a valid index, but you also might not, and nobody is under any obligation to trigger a runtime error along the way.
 ///
-/// It isn't just unsafe types that are free from this obligation, either; even `Array` - the paragon of safe `Collection`s in Swift - won't trigger runtime errors
-/// for many misuses of the `Collection` protocol. You have a 10-element `Array`? Want to increment its `startIndex` by 1000? No problem.
-/// How about decrementing it by 1000? Also not a problem. Seriously, try it. I'm not lying:
+/// This type is designed to provide speed _and_ safety, so it's useful to know what we can get away with
+/// without violating memory safety.
+///
+/// The most important thing is that we're not obliged to detect _misuses_ of the `Collection` protocol.
+/// For instance, let's say you have a buffer of 10 elements, and you increment its `startIndex` 1000 times,
+/// what is the result?
+///
+/// Actually, it's not specified: it _may_ trigger a `fatalError` at runtime, or it _may_ return a garbage index,
+/// or it _may_ automatically limit itself to `endIndex`, or wrap around to `startIndex` again.
+/// And if you take that unspecified result and pass it in to, say, `distance(from: Index, to: Index)`,
+/// it's also not specified what that will do, either - it might `fatalError`, or it might not; it might return a garbage
+/// value, or maybe it always returns 0, etc. It's not specified, is the point.
+///
+/// And it's worth pointing out explicitly that "garbage" means garbage for any reason.
+/// It's perfectly fine if `distance(from: Index, to: Index)` overflows with invalid indexes, for instance.
+/// There is no obligation to `fatalError`.
+///
+/// None of this is at odds with the idea of memory safety because, crucially, these operations _do not access memory_.
+///
+/// This isn't just a property of unsafe types, either; even `Array` - the paragon of safe `Collection`s in Swift -
+/// won't trigger runtime errors for many misuses of the `Collection` protocol, and will happily return "garbage"
+/// indexes outside the bounds of the array.
+///
+/// You have a 10-element `Array`? Want to increment its `startIndex` by 1000? No problem.
+/// How about decrementing `startIndex` by 1000? Also fine.
 ///
 /// ```swift
 /// let arr = Array(10..<20)
 /// arr.index(arr.startIndex, offsetBy: 1000)  // Returns 1000. No runtime error.
 /// arr.index(arr.startIndex, offsetBy: -1000) // Returns -1000. No runtime error.
 /// ```
 ///
-/// This is really important. It means that we can provide the bare minimum bounds-checking needed for memory safety with very few actual checks,
-/// many of which can be relatively simple for the compiler to prove away, providing we express them in the right way.
+/// Again, there's no problem with any of that (from a memory safety perspective),
+/// because these operations _do not access memory_.
+///
 ///
 /// ## Slicing
 ///
-/// Bounds-checking in slices is particularly interesting, because we really want to keep the simplified `index < buffer_size` bounds check.
-/// But while the first buffer starts with indices `0..<buffer_size`, slices (and slices of slices) can have a non-zero offset from the start of the buffer.
-/// So just checking `index < buffer_size` is not enough to say that an index is within the _slice's_ bounds.
 ///
-/// But we can get around this in a bit of cheeky way: by just allowing certain reads outside of the slice's bounds 😅. I know, it sounds wrong, but actually - is it?
-/// Again, we're trying to do the bare minimum to guarantee we never over-read the original buffer - as long as that is true, we don't _really_ care about
-/// the slice's bounds. The slice bounds are mostly only needed for `Collection` semantics, (e.g. that `startIndex` and `count` return correct values),
-/// and reads from other, invalid indexes are not _required_ to always trigger a runtime error. As mentioned above, as long as we always check that slices
-/// (and slices of slices) never increase the maximum addressable offset, we can guarantee not to over-read the buffer just by checking the slice's upper bound.
+/// Bounds-checking in slices is interesting, because we want to keep the simple `i < endIndex` bounds check.
+///
+/// But while the original buffer starts with indexes `0..<size`, slices can have a non-zero offset
+/// from the start of their parent, so just checking `i < size` is not enough to say that an index
+/// is within the slice's bounds.
+///
+/// But we can get around this in a bit of cheeky way: by just allowing certain reads outside of the slice's bounds 😅.
+/// Remember what we said about Collection semantics -- we don't _strictly_ care that you only access indexes
+/// within the slice's bounds, we only care that you only access locations within the original buffer's bounds.
+/// As it turns out, slice bounds are a Collection concept, and enforcing them is not necessary for memory safety.
+///
+/// So what we do is:
 ///
-/// To illustrate: if the slice bounds are `4..<10`, and you want to read from offset `2`, that's not actually a memory-safety issue.
-/// We know that offset `2` is within the buffer; we don't need to fail in that situation. But if you try to read from offset `11` - well, we don't know that the buffer
-/// extends that far, so we do need to fail in order to preserve memory-safety.
+/// 1. We keep the base pointer of the original buffer.
+///
+/// 2. We store the range of the slice -- we still need to _know_ its bounds for it to, y'know, be a slice.
+///
+/// 3. When creating a slice, it turns out that we only need to check the `upperBound` of the slice,
+///    to ensure the maximum addressable offset never grows - only shrinks or stays the same.
+///    This is true even if the range is invalid (i.e. `lowerBound > upperBound`).
+///
+/// 4. This means we can keep our simple `i < endIndex` bounds check in release mode.
+///    We allow reads outside of the slice, but never outside of the buffer it belongs to.
+///
+/// Here's that visually:
+///
+/// ```
+/// ┌────────────────────────────┐
+/// │                            │ - original buffer
+/// └────────────────────────────┘
+/// 0    |                  |    x
+///      |                  |
+///      ┌──────────────────┐
+///      │                  │ - bounds of the slice
+///      └──────────────────┘
+/// ┌───────────────────────┐
+/// │░░░░░░░░░░░░░░░░░░░░░░░│ - will not trap for accesses in this region
+/// └───────────────────────┘
+/// 0                       y
+/// ```
+///
+/// To give a concrete example: let's say the original buffer has a length of `20`, and the slice bounds are `4..<10`,
+/// there will not be a `fatalError` in release builds if you try to access index `2`, because we still know
+/// it is within bounds of the original buffer, so it is not a memory-safety issue.
+///
+/// That doesn't mean it isn't a bug - it is, and we _will_ still trap in debug builds
+/// because you're doing something incorrect, but it's not undefined behaviour.
+///
+/// A related problem is an invalid `Range` - i.e. situations where `range.lowerBound > range.upperBound`.
+/// As long as we always check that successive slices never increase their `upperBound` over their base,
+/// it will always be valid, and so for the most part we don't need to check that the range is properly formed,
+/// or even that `lowerBound` is in-bounds (!).
+///
+/// That's because, again, it doesn't actually access memory. So long as that bounds check on actual accesses holds,
+/// nothing else really matters. It's important for `upperBound` to be valid to ensure that, but that's all.
+///
+/// I said "for the most part", because there are two exceptions:
+///
+/// - `withContiguousStorageIfAvailable`
+/// - `_copyContents(initializing:)`
+///
+/// In both of these cases, we perform "bulk access" to the entire slice or return an unchecked UBP pointer over it,
+/// so we need to check both ends of its bounds.
 ///
-/// A related problem is invalid `Range`s; i.e. situations where `range.lowerBound > range.upperBound`. It turns out that, again, as long as we always
-/// check that successive slices never increase the `upperBound` over their base (only decrease the addressable buffer or stay the same), we don't need
-/// to check that the range is properly formed, or even that `startIndex` is in-bounds (until we try to access from it, of course).
 ///
 /// ## A delicate balance
 ///
-/// This is a delicate balance, for sure. It definitely does **not** protect against logic bugs which can happen by invoking behaviour unspecified by `Collection`
-/// (e.g. reading from an invalid index), but it does provide effective protection against reading outside the bounds of a buffer of memory,
-/// and hence provides meaningful safety guarantees beyond what `UnsafeBufferPointer` provides (which is nothing; no meaningful checks whatsoever).
 ///
-/// Moreover (and perhaps most importantly), it does so with a negligible impact on performance. In the "AverageURLs" benchmark, I've seen the performance
-/// difference when switching from UBP to this type range from about -5% (~ 32.2ms to 33.8ms) to about +8% (~32.2ms vs 29.7ms). My guess is that the use of
-/// unsigned integers helps the compiler avoid some runtime checks when constructing `Range` literals, which are frequently used in generic `Collection` code.
+/// This is a delicate balance, for sure. It's important to remember the goal of this type -
+/// to do the utter bare minimum required for memory safety (in release builds).
+///
+/// It does **not** protect against logic bugs which can happen if you misuse the `Collection` protocol
+/// (e.g. invalid indexing operations, and sometimes even reading from an invalid location),
+/// so if you get a problem in a release build, it may be more difficult to debug.
+/// But those kinds of bugs should at least be deterministic.
+///
+/// In debug builds, more thorough checks are enabled, so you can more easily spot where the unexpected behaviour
+/// happened.
 ///
-/// But overall it's kind of margin-of-error-ish stuff, which is a great result considering the added memory-safety. By comparison, a type which adds
-/// naive bounds-checking (including being strict about slice lower bounds) on an unsafe buffer with `Int` indexes, comes with a 20-25% performance hit.
 ///
 /// ## Great! So, why is this still called 'Unsafe'?
 ///
+///
 /// Bounds-checking alone isn't enough for memory safety. In particular, this type:
 ///
-/// 1. Doesn't guarantee the lifetime of the underlying memory. It's still a pointer underneath, after all.
-/// 2. Doesn't guarantee that the buffer has been fully initialized. You're supposed to do that before you create this view over the buffer.
+/// 1. Doesn't guarantee the lifetime of the underlying memory.
+///    It's still a pointer underneath, after all.
+///
+/// 2. Doesn't guarantee that the buffer has been fully initialized.
+///    You're supposed to do that before you create this view over the buffer.
 ///
-/// So it's still unsafe, but the situations where unsafe, undefined behaviour could be invoked are easier to spot, even in complex `Collection` algorithms.
+/// So it's still unsafe, but the situations where unsafe, undefined behaviour could be invoked are easier to spot,
+/// even in complex `Collection` algorithms.
 ///
 @usableFromInline
 internal struct UnsafeBoundsCheckedBufferPointer<Element> {
@@ -308,9 +405,8 @@ extension UnsafeBoundsCheckedBufferPointer: RandomAccessCollection {
 
   @inlinable
   internal var isEmpty: Bool {
-    // We don't expect startIndex to regularly be greater than endIndex, but by writing it this way
-    // (rather than startIndex != endIndex), we communicate that !isEmpty is a synonym for startIndex < endIndex.
-    // This can help eliminate some bounds checks.
+    // We don't actually expect startIndex to be greater than endIndex, but by writing it this way
+    // rather than `startIndex != endIndex`, we communicate that `!isEmpty` means `startIndex < endIndex`.
     startIndex >= endIndex
   }
 
@@ -386,7 +482,7 @@ extension UnsafeBoundsCheckedBufferPointer: RandomAccessCollection {
     _ body: (UnsafeBufferPointer<Element>) throws -> R
   ) rethrows -> R? {
     // Watch out!
-    // - 'guard startIndex < endIndex' is effectively a bounds-check for 'startIndex'.
+    // - 'startIndex < endIndex' is effectively a bounds-check for 'startIndex'.
     //   We otherwise don't check the collection's lower bound.
     // - This must be a single statement, otherwise the compiler may not recognize that this
     //   never returns 'nil', and might not eliminate the non-contiguous branch in generic algorithms.