Add NIOLoopBound(Box) uncheckedValue

[natikgadzhi]

Motivation

• This commit adds unchecked version of init and value get+set of NIOLoopBound and NIOLoopBoundBox.
• Closes NIOLoopBound should get unchecked variants that are truly free in release mode #2506.

Modifications

• Added init and public var uncheckedValue properties to both NIOLoopBound and NIOLoopBoundBox.

Review

• Tests pass locally.
• How should we test that NIOLoopBound.init(uncheckedEventLoop:) calls assertInEventLoop? EmbeddedEventLoop seems to be final — how do I mock the assert function on it elegantly, without copy and pasting all the protocol conformance stuff?

@weissi, sorry, I found a few really nice rabbit holes in swift-syntax and got distracted 👀

[natikgadzhi]

Tomato tomato, but thought there are more initializes in our codebase than initialises.

[natikgadzhi]

A few hours after I pushed this up, I realized that having an unchecked init does not necessarily win the user that much time — you init once. Perhaps if you have to init a lot of objects that makes sense, but uncheckedValue seems more useful, in comparison.

[FranzBusch]

uncheckedValue is basically UnsafeTransfer which just puts the trust into the users hand that they now where things are going and that it is safe to do so.

[Lukasa]

I think this is worth keeping: while you only init once, you typically shouldn't store one of these: instead, you should use it to effect a transfer.

[natikgadzhi]

Nit: Run swiftformat, come on.

[FranzBusch]

I think unchecked isn't quite the right name for this init but I can't come up with a better one right now. It is not completely unchecked but rather just asserts than preconditions.

[Lukasa]

Unchecked is fine for that: it's what Range uses, for example.

[FranzBusch]

uncheckedValue is basically UnsafeTransfer which just puts the trust into the users hand that they now where things are going and that it is safe to do so.

[FranzBusch]

What if we just provide a second type that does asserts or preconditions. Alternatively what about just making the whole precondition/assert behaviour configurable on init?

[Lukasa]

I think this pattern is better: I don't see a good reason to make this a type or a runtime distinction. It's good to be able to see this in the code, as it's fundamentally a safety-breaking optimization.

[FranzBusch]

Fair, if we say unchecked is fine as a spelling then I am happy with this init and property as well.

[dnadoba]

TBH I'm also thinking a type would be better. It's how the rest of Swifts unsafe/unchecked APIs usually work e.g. UncheckedContinuation, Unsafe*Pointer, @unchecked Sendable.

It may also enable us to drop the stored EventLoop property in release modes, once DEBUG is properly propagated in SwiftPM (haven't checked recently), making this actually a zero cost abstraction.

[weissi]

One type is better here: I often want mixed use: First time in a function: checked, follow-ups: unchecked. Different type would be pain and would likely push me to use unchecked everywhere.

[dnadoba]

the number of safe types with unsafe functions vastly dwarfs the number of unsafe types

All the withUnsafe* functions I can think of vend you types that have Unsafe or Unchecked in the name. About which functions do you think?
I'm only aware of unsafeDowncast, unsafeBitCast and [Closed]Range(uncheckedBounds:) which result in a type that doesn't have Unsafe in the name.

[Lukasa]

[Closed]Range is the controlling example. Range is not an unsafe type, but you can construct it in an unsafe way. You should not audit your entire codebase because you want to allow people to opt out of enforcement of an invariant. Imagine for a moment the counterfactual, where we had UnsafeRange. Such a type is borderline useless, because to support people using it you have to litter it all over your codebase. This makes the "grep for Unsafe" strategy very weak, because almost all of the places holding an UnsafeRange are not doing things that are unsafe.

Similarly here: you want to draw attention to the places where the invariants are being broken, not to where they are being observed.

[dnadoba]

Agree that you need to look at where the invariants are broken but you need to look at the creation for that for NIOLoopBound. [Closed]Range(uncheckedBounds:) only has one unsafe/unchecked API and everything else is safe/checked. You only need to audit the use of this initialiser, nothing else. If [Closed]Range(uncheckedBounds:) is safe or not can be determined by just its parameters passed into the initialiser and doesn't have any other dependencies.

You can't say in isolation if loopBound.uncheckedValue is safe or not. You need to go and look at the creation of the NIOLoopBound and trace back on which EventLoop it was created. We are again more like Unsafe*Pointer. The creation and operations of Unsafe*Pointer are more complex and dependent on where it is coming from.

We are proposing a set of duplicated APIs which make all operations on NIOLoopBound unsafe/unchecked.
This is more in line with the UnsafePointer APIs where almost all operation are unsafe.

This design also gives the wrong impression that NIOLoopBound.value is always safe and only uncheckedValue is unsafe. With this PR value becomes unsafe as well as you might have initialised it with NIOLoopBound(_:uncheckedEventLoop).

[dnadoba]

Concretely, I'm proposing this API:

public struct NIOUncheckedLoopBound<Value> {
    public init(_ value: Value, eventLoop: EventLoop) {}
    public init(_ value: NIOLoopBound<Value>) {}
    public init(_ value: NIOLoopBoundBox<Value>) {}
    
    public var value: Value {
        get {}
    }
}

public final class NIOUncheckedLoopBoundBox<Value> {
    public init(_ value: Value, eventLoop: EventLoop) {}
    public init(_ value: NIOLoopBound<Value>) {}
    public init(_ value: NIOLoopBoundBox<Value>) {}
    
    public var value: Value {
        get {}
        set {}
    }
}

If NIOLoopBound.value is to expensive you can still wrap it in NIOUncheckedLoopBound(myLoopBoundValue).value. Its more verbose but that's a good thing.

[natikgadzhi]

Whoa, thank you, @dnadoba, @Lukasa, and @weissi for the discussion! I'm learning quite a lot from you here.

I'm not attached to the current implementation — as I learn more and if we decide to change it, I'm happy to rewrite it.

You can't say in isolation if loopBound.uncheckedValue is safe or not. You need to go and look at the creation of the NIOLoopBound and trace back on which EventLoop it was created.

I agree, and I agree it's different from the Range example.

We are proposing a set of duplicated APIs which make all operations on NIOLoopBound unsafe/unchecked.
This is more in line with the UnsafePointer APIs where almost all operation are unsafe.

This design also gives the wrong impression that NIOLoopBound.value is always safe and only uncheckedValue is unsafe. With this PR value becomes unsafe as well as you might have initialised it with NIOLoopBound(_:uncheckedEventLoop).

If we choose to stick with the current implementation, I think I should at the very least comment and document on that behavior.

Such a type is borderline useless, because to support people using it you have to litter it all over your codebase.

@Lukasa, with the Range example, I'd fully agree. For NIOLoopBound, though — we're not passing LoopBound around in the NIO codebase right now, right? So in theory, sure, we could do two separate structures. The weight of supporting two different structures would then be on the user. Perhaps we could provide a NIOLoopBound as a protocol, and make a safe and unchecked implementations of that protocol, so that users could accept some NIOLoopBound and deal with them that way?

Again, I feel that I'm out of my depth in the API design discussion, but I'm delighted to be part of it and listen in. Feel free to "well actually" me on any points I'm bringing up.