explain some force tries/unwraps (#619)

Motivation:

It's generally good style to explain why something needs to be force
unwrapped/tried if not obvious.

Modifications:

Add explanations to a bunch of places.

Result:

Code easier to understand.

Sources/NIO/BaseSocket.swift

@@ -24,13 +24,13 @@ protocol SockAddrProtocol {
 }
 
 /// Returns a description for the given address.
-internal func descriptionForAddress(family: CInt, bytes: UnsafeRawPointer, length byteCount: Int) -> String {
+internal func descriptionForAddress(family: CInt, bytes: UnsafeRawPointer, length byteCount: Int) throws -> String {
     var addressBytes: [Int8] = Array(repeating: 0, count: byteCount)
-    return addressBytes.withUnsafeMutableBufferPointer { (addressBytesPtr: inout UnsafeMutableBufferPointer<Int8>) -> String in
-        try! Posix.inet_ntop(addressFamily: family,
-                             addressBytes: bytes,
-                             addressDescription: addressBytesPtr.baseAddress!,
-                             addressDescriptionLength: socklen_t(byteCount))
+    return try addressBytes.withUnsafeMutableBufferPointer { (addressBytesPtr: inout UnsafeMutableBufferPointer<Int8>) -> String in
+        try Posix.inet_ntop(addressFamily: family,
+                            addressBytes: bytes,
+                            addressDescription: addressBytesPtr.baseAddress!,
+                            addressDescriptionLength: socklen_t(byteCount))
         return addressBytesPtr.baseAddress!.withMemoryRebound(to: UInt8.self, capacity: byteCount) { addressBytesPtr -> String in
             String(cString: addressBytesPtr)
         }
@@ -78,7 +78,8 @@ extension sockaddr_in: SockAddrProtocol {
     /// Returns a description of the `sockaddr_in`.
     mutating func addressDescription() -> String {
         return withUnsafePointer(to: &self.sin_addr) { addrPtr in
-            descriptionForAddress(family: AF_INET, bytes: addrPtr, length: Int(INET_ADDRSTRLEN))
+            // this uses inet_ntop which is documented to only fail if family is not AF_INET or AF_INET6 (or ENOSPC)
+            try! descriptionForAddress(family: AF_INET, bytes: addrPtr, length: Int(INET_ADDRSTRLEN))
         }
     }
 }
@@ -101,7 +102,8 @@ extension sockaddr_in6: SockAddrProtocol {
     /// Returns a description of the `sockaddr_in6`.
     mutating func addressDescription() -> String {
         return withUnsafePointer(to: &self.sin6_addr) { addrPtr in
-            descriptionForAddress(family: AF_INET6, bytes: addrPtr, length: Int(INET6_ADDRSTRLEN))
+            // this uses inet_ntop which is documented to only fail if family is not AF_INET or AF_INET6 (or ENOSPC)
+            try! descriptionForAddress(family: AF_INET6, bytes: addrPtr, length: Int(INET6_ADDRSTRLEN))
         }
     }
 }

Sources/NIO/BaseSocketChannel.swift

@@ -805,6 +805,9 @@ class BaseSocketChannel<T: BaseSocket>: SelectableChannel, ChannelCore {
         }
 
         if self.lifecycleManager.isPreRegistered {
+            // we expect kqueue/epoll registration to always succeed which is basically true, except for errors that
+            // should be fatal (EBADF, EFAULT, ESRCH, ENOMEM) and a two 'table full' (EMFILE, ENFILE) error kinds which
+            // we don't handle yet but might do in the future (#469).
             try! becomeFullyRegistered0()
             if self.lifecycleManager.isRegisteredFully {
                 self.becomeActive0(promise: promise)
@@ -966,6 +969,9 @@ class BaseSocketChannel<T: BaseSocket>: SelectableChannel, ChannelCore {
             if let channelErr = err as? ChannelError, channelErr == ChannelError.eof {
                 readStreamState = .eof
                 // Directly call getOption0 as we are already on the EventLoop and so not need to create an extra future.
+
+                // getOption0 can only fail if the channel is not active anymore but we assert further up that it is. If
+                // that's not the case this is a precondition failure and we would like to know.
                 if self.lifecycleManager.isActive, try! getOption0(option: ChannelOptions.allowRemoteHalfClosure) {
                     // If we want to allow half closure we will just mark the input side of the Channel
                     // as closed.

Sources/NIO/ByteBuffer-aux.swift

@@ -277,7 +277,7 @@ extension ByteBuffer {
     ///
     /// - returns: A `ByteBuffer` sharing storage containing the readable bytes only.
     public func slice() -> ByteBuffer {
-        return getSlice(at: self.readerIndex, length: self.readableBytes)!
+        return getSlice(at: self.readerIndex, length: self.readableBytes)! // must work, bytes definitely in the buffer
     }
 
     /// Slice `length` bytes off this `ByteBuffer` and move the reader index forward by `length`.

Sources/NIO/ByteBuffer-views.swift

@@ -53,7 +53,7 @@ public struct ByteBufferView: ContiguousCollection, RandomAccessCollection {
         guard position >= self.range.lowerBound && position < self.range.upperBound else {
             preconditionFailure("index \(position) out of range")
         }
-        return self.buffer.getInteger(at: position)!
+        return self.buffer.getInteger(at: position)! // range check above
     }
 
     public subscript(range: Range<Index>) -> ByteBufferView {

Sources/NIO/Heap.swift

@@ -185,7 +185,7 @@ extension Heap: CustomDebugStringConvertible {
             return "<empty heap>"
         }
         let descriptions = self.storage.map { String(describing: $0) }
-        let maxLen: Int = descriptions.map { $0.count }.max()!
+        let maxLen: Int = descriptions.map { $0.count }.max()! // storage checked non-empty above
         let paddedDescs = descriptions.map { (desc: String) -> String in
             var desc = desc
             while desc.count < maxLen {

Sources/NIO/Selector.swift

@@ -331,6 +331,8 @@ final class Selector<R: Registration> {
          is likely to cause performance problems. By abusing ARC, we get the guarantee that there won't be any future
          wakeup calls as there are no references to this selector left. 💁
          */
+
+        // we try! this because `close` only fails in cases that should never happen (EBADF).
 #if os(Linux)
         try! Posix.close(descriptor: self.eventfd)
 #else

Sources/NIO/SocketAddresses.swift

@@ -84,14 +84,16 @@ public enum SocketAddress: CustomStringConvertible {
             host = addr.host.isEmpty ? nil : addr.host
             type = "IPv4"
             var mutAddr = addr.address.sin_addr
-            addressString = descriptionForAddress(family: AF_INET, bytes: &mutAddr, length: Int(INET_ADDRSTRLEN))
+            // this uses inet_ntop which is documented to only fail if family is not AF_INET or AF_INET6 (or ENOSPC)
+            addressString = try! descriptionForAddress(family: AF_INET, bytes: &mutAddr, length: Int(INET_ADDRSTRLEN))
 
             port = "\(self.port!)"
         case .v6(let addr):
             host = addr.host.isEmpty ? nil : addr.host
             type = "IPv6"
             var mutAddr = addr.address.sin6_addr
-            addressString = descriptionForAddress(family: AF_INET6, bytes: &mutAddr, length: Int(INET6_ADDRSTRLEN))
+            // this uses inet_ntop which is documented to only fail if family is not AF_INET or AF_INET6 (or ENOSPC)
+            addressString = try! descriptionForAddress(family: AF_INET6, bytes: &mutAddr, length: Int(INET6_ADDRSTRLEN))
     
             port = "\(self.port!)"
         case .unixDomainSocket(let addr):

Sources/NIO/System.swift

@@ -78,6 +78,7 @@ private func isBlacklistedErrno(_ code: Int32) -> Bool {
 }
 
 private func assertIsNotBlacklistedErrno(err: CInt, where function: StaticString) -> Void {
+    // strerror is documented to return "Unknown error: ..." for illegal value so it won't ever fail
     assert(!isBlacklistedErrno(err), "blacklisted errno \(err) \(String(cString: strerror(err)!)) in \(function))")
 }
 

Sources/NIOHTTP1/HTTPTypes.swift

@@ -246,7 +246,7 @@ private extension String {
         case .string(let string):
             self = string
         case .byteBuffer(let buffer):
-            self = buffer.getString(at: buffer.readerIndex, length: buffer.readableBytes)!
+            self = buffer.getString(at: buffer.readerIndex, length: buffer.readableBytes)! // bytes definitely in buffer
         }
     }
 }

Sources/NIOPriorityQueue/Heap.swift

@@ -189,7 +189,7 @@ extension Heap: CustomDebugStringConvertible {
             return "<empty heap>"
         }
         let descriptions = self.storage.map { String(describing: $0) }
-        let maxLen: Int = descriptions.map { $0.count }.max()!
+        let maxLen: Int = descriptions.map { $0.count }.max()! // storage is guarded to be non-empty
         let paddedDescs = descriptions.map { (desc: String) -> String in
             var desc = desc
             while desc.count < maxLen {

Sources/NIOTLS/SNIHandler.swift

@@ -171,7 +171,7 @@ public class SniHandler: ByteToMessageDecoder {
         //
         // From this point onwards if we don't have enough data to satisfy a read, this is an error and
         // we will fall back to let the upper layers handle it.
-        tempBuffer = tempBuffer.getSlice(at: tempBuffer.readerIndex, length: Int(contentLength))!
+        tempBuffer = tempBuffer.getSlice(at: tempBuffer.readerIndex, length: Int(contentLength))! // length check above
 
         // Now parse the handshake header. If the length of the handshake message is not exactly the
         // length of this record, something has gone wrong and we should give up.
@@ -204,7 +204,7 @@ public class SniHandler: ByteToMessageDecoder {
         }
 
         // Check the content type.
-        let contentType: UInt8 = buffer.readInteger()!
+        let contentType: UInt8 = buffer.readInteger()! // length check above
         guard contentType == tlsContentTypeHandshake else {
             // Whatever this is, it's not a handshake message, so something has gone
             // wrong. We're going to fall back to the default handler here and let
@@ -213,7 +213,7 @@ public class SniHandler: ByteToMessageDecoder {
         }
 
         // Now, check the major version.
-        let majorVersion: UInt8 = buffer.readInteger()!
+        let majorVersion: UInt8 = buffer.readInteger()! // length check above
         guard majorVersion == 3 else {
             // A major version of 3 is the major version used for SSLv3 and all subsequent versions
             // of the protocol. If that's not what this is, we don't know what's happening here.
@@ -223,7 +223,7 @@ public class SniHandler: ByteToMessageDecoder {
 
         // Skip the minor version byte, then grab the content length.
         buffer.moveReaderIndex(forwardBy: 1)
-        let contentLength: UInt16 = buffer.readInteger()!
+        let contentLength: UInt16 = buffer.readInteger()! // length check above
         return Int(contentLength)
     }
 

Sources/NIOWebSocket/WebSocketFrame.swift

@@ -66,7 +66,8 @@ extension WebSocketMaskingKey: ExpressibleByArrayLiteral {
     public typealias ArrayLiteralElement = UInt8
 
     public init(arrayLiteral elements: UInt8...) {
-        self.init(elements)!
+        precondition(elements.count == 4, "WebSocketMaskingKeys must be exactly 4 bytes long")
+        self.init(elements)! // length precondition above
     }
 }
 
@@ -155,9 +156,12 @@ public struct WebSocketFrame {
     /// The opcode for this frame.
     public var opcode: WebSocketOpcode {
         get {
+            // this is a public initialiser which only fails if the opcode is invalid. But all opcodes in 0...0xF
+            // space are valid so this can never fail.
             return WebSocketOpcode(encodedWebSocketOpcode: firstByte & 0x0F)!
         }
         set {
+            // this ! isn't actually safe (won't cause problems in reality though). Filed as #617 to be fixed in NIO 2.0
             self.firstByte = (self.firstByte & 0xF0) + UInt8(webSocketOpcode: newValue)!
         }
     }