Feature request: make `ParserSpan` `~Copyable`.

[CrystDragon]

Currently, ParserSpan is Copyable, it is OK because the documentation says it's

A non-owning, non-escaping view for parsing binary data.

However, that means client code can use it this way:

extension QOI.Header {
  init(parsing input: inout ParserSpan) throws {
    var sharedCopy = input
    
    self.channels = try Channels(parsing: &input)
    self.colorspace = try ColorSpace(parsing: &sharedCopy)
  }
}

In this snippet, both input and sharedCopy are referencing the same underline bytes, however they are distinct. The user might think they can be used interchangeably, which is not the case.

I think changing ParserSpan into representing exclusive references can help reduce such confusion.

[natecook1000]

@CrystDragon, thanks for this suggestion! I think it makes a lot of sense to mark ParserSpan as ~Copyable, and would help users understand the intended usage of the type. With that change in place, I didn't need to modify any of the included example parsers at all, which is a good sign.

I'd like to get a little more feedback before making the change, but you can see my branch here: https://github.com/apple/swift-binary-parsing/compare/noncopyable

[Lancelotbronner]

Would that prevent backtracked parsing or would we just need to call an explicit .clone() or something?

For example: I might need to parse a complex type, validate it, re-parse the same area as a different type on validation failure. The obvious way to write it would be the following.

let anchor = input
let result: WhyDidTheyDoThis
do {
    result = try WhyDidTheyDoThis.version1(parsing: &input)
} catch {
    input = anchor
    result = try WhyDidTheyDoThis.version2(parsing: &input)
}

[Lancelotbronner]

I just noticed atomically on ParserSpan, so my example above would be

let result: WhyDidTheyDoThis
do {
    result = try input.atomically {
        try WhyDidTheyDoThis.version1(parsing: &$0)
    }
} catch {
    result = try WhyDidTheyDoThis.version2(parsing: &input)
}

[atrick]

Caveat: I haven't taken the time to work with this library or research typical usage. Just some general thoughts...

I think the decision to make ParserSpan non-Copyable should be based on whether it is really core to its design that ParerSpan is a unique view into memory.

Avoiding possible confusion by itself doesn't seem like a strong justificaiton. If a Swift programmer is confused by this:

    var sharedCopy = input
    
    self.channels = try Channels(parsing: &input)
    self.colorspace = try ColorSpace(parsing: &sharedCopy)

then it seems they would be confused by all var/inout variables in Swift.

Since Swift was designed around copyability, there's a usability price to pay for ~Copyable. Programmers will be burdened with thinking about ownership whenever passing and argument and won't be able to cache the value in a variable or property. My bigger concern is that programmers will learn idiomatic techniques for working with Span. It could be a problem for other Span-like APIs to behave differently.

[CrystDragon]

@atrick I agree with all of what you said.

ParserSpan seems to sit in the middle ground between Span and MutableSpan: it is mutable (unlike Span), but the mutation is on its privately controlled properties (unlike MutableSpan). So it does make sense, no matter whether we choose to make ParserSpan copyable or not.

However, I believe on the client's side, binary parsing should be a top-down approach in most cases, backtracking situations should be rare. And treating ParserSpan as representing the whole data is much simpler than treating it as a "view" of some data. That's why I think a ParserSpan is more closer to MutableSpan and should be used exclusively.

[atrick]

Thanks for the explanation. It seems like it could be valuable to prevent casual copying.

[apple-fcloutier]

When parsing over shared memory, it's a serious security liability that you might accidentally parse the same data twice because it can change from under your feet. On the other hand, there are formats that it is incredibly wasteful to parse entirely ahead of time and you need to be able to store ranges (and accept that you shouldn't use these with memory that can change from under your feet). I think it makes sense to have an explicit copy operation.

[natecook1000]

While there isn't an explicit copy operation, there are two groups of methods that return copies:

• parserSpan.sliceSpan(...) consumes the requested size chunk from the current ParserSpan and returns it as a copy, for immediate parsing as a separate block of memory. This is useful for parsing a "section" of data where the size is fixed, since it helps prevent overruns even in the middle of a block of memory.
• parserSpan.seeking(toRange:) (etc) returns a copy of the span with bounds that are shifted according to the parameters. These are paired with mutating versions (seek(toRange:)) and are useful when working with ranges or offsets that have been previously saved, like @apple-fcloutier mentioned.