API issue: Unit (e.g. bytes/characters/code units) omitted when configuring limits/sizes

[weissi]

Describe the bug

Some APIs like .string(...), .bytes(...) and size parameters take integers of units unknown to the programmer.

For example:

        output: .string(limit: 12345, encoding: UTF16.self)

What's the limit here? 12345 bytes, 12345 UTF16 code units, 12345 String.Characters? This is unnecessarily unclear. I would argue that the limit should be explicit. Compare this for example to NIOFileSystem's API:

        let plan = try await ByteBuffer(
            contentsOf: "/Users/hal9000/demise-of-dave.txt",
            maximumSizeAllowed: .mebibytes(1)
        )

which is much clearer than maximumSizeAllowed: 1024 * 1024 or similar.

[iCharlesHu]

This one is more like a feature request.

[weissi]

This one is more like a feature request.

You can treat it as such but it should be fixed before stabilising the API. You could also just add Bytes to clarify, maybe maximumSizeBytes: 1235.

[broken-circle]

I think this would be worth doing. Both a new ByteCount type and the parameter rename (limit to byteLimit) are source breaking, but the parameter rename doesn't address the unintuitiveness and high chance of errors in writing 1024 * 1024. Since we'd be breaking compatibility anyway, I think ByteCount is the way to go.

The shape would be close to swift-nio's implementation, with some Subprocess-specific changes: Int-backed to match internal call sites, a static var unlimited to replace the current .max checks, and CustomStringConvertible conformance for error rendering.

public struct ByteCount: Sendable, Hashable {
    public let bytes: Int

    public static func bytes(_ count: Int) -> ByteCount
    public static func kilobytes(_ count: Int) -> ByteCount 
    public static func megabytes(_ count: Int) -> ByteCount
    public static func gigabytes(_ count: Int) -> ByteCount
    public static func kibibytes(_ count: Int) -> ByteCount  
    public static func mebibytes(_ count: Int) -> ByteCount
    public static func gibibytes(_ count: Int) -> ByteCount 

    public static var unlimited: ByteCount
}

extension ByteCount: 
  AdditiveArithmetic, Comparable, 
  CustomStringConvertible, CustomDebugStringConvertible { }

BufferingPolicy.maxLineLength(Int) uses Encoding.CodeUnit, so the underlying byte count changes across encoding types. A labeled argument like case maxLineLength(codeUnits: Int) would work here without needing a dedicated type.

I can take this on either way, but wanted to get your thoughts first since the approaches are so different.

@jakepetroules, curious for your take too.

[jakepetroules]

Following NIO's lead seems reasonable.

I'm not sure it's worth adding the complexity of specifying length in terms of code units or anything else, bytes seems sufficient.

And I don't think "unlimited" should be part of the API, after all you can't allocate unlimited memory and it encourages introducing vulnerabilities into client code.

[broken-circle]

Great, I'll start drafting the ByteCount change. Quick clarification on the code units point: BufferingPolicy.maxLineLength(Int) is currently using code units (StringSequence.AsyncIterator checks buffer.count where buffer: [Encoding.CodeUnit]). Did you mean to switch that to a byte-denominated check? It'd be a behavioral change, so just wanted to confirm before going down that path.

[iCharlesHu]

We decided to wait until there is a canonical type from standard library or swift-system to adopt this (definitely post 1.0). In the meantime, we will use Int64 instead.

[weissi]

We decided to wait until there is a canonical type from standard library or swift-system to adopt this (definitely post 1.0). In the meantime, we will use Int64 instead.

@iCharlesHu This is very surprising. Introducing ByteCount now will facilitate moving to a hypothetical stdlib type. The recipe is very straightforward: Shadow all APIs with the new stdlib type and mark the then-old ByteCount ones as deprecated&disfavouredOverload. This is much more challenging with ints because you lose readability of the code until this is done. Something like .mebiBytes(5) just reads much better than 5_242_880 or 5 * 1024 * 1024 or 5 * (1 << 20) or whatever people think they want to write.

Therefore, I think it's a grave mistake to not to the best thing now but instead defer to Int64 until a hypothetical future type gets introduced. Let's do the right thing, the extra type doesn't cost us anything, @broken-circle has even done the work already