Deprecate and rename ByteBuffer.get*/set*

[weissi]

ByteBuffer's get*/set* methods have a very niche use-case. In almost all cases, the users should use read*/write* or something like readableBytesView.

The key issues are stuff like buffer.getData(at: 0, length: 16). This will often work (because many ByteBuffers happen to have readerIndex == 0) but it won't always work.

Unfortunately the get* APIs are very prominent and new users are pretty much invited to use them because the read* APIs require a mutable ByteBuffer. So if you (which is common!) have an immutable one (say a function argument), then Xcode won't even complete the read* versions. So of course users use get* and then write wrong code.

I'm proposing a set of changes:

• deprecate get*/set*
• introduce new, scary sounding (long names) methods that do the same thing as the existing get*/set* ones
• introduce a new family of APIs (maybe peek*?) that work like read* (ie. from readerIndex) but do not move the readerIndex

That would mean. All existing uses of buffer.get*(at: 0, length: ...) can without other changes be made buffer.peek*(length: ...). And the few uses of buffer.get*(at: nonZeroArgument) can use the specialised, scary sounding new methods.

[vlm]

+1

[Lukasa]

While I think these changes are reasonable, I'm not sure if they merit the churn of a deprecate/replace cycle. In particular, the fact that we cannot unilaterally recommend a single transformation for the get* methods means that each call site needs to be audited by hand for the appropriate replacement.

A quick search across the core NIO repositories (swift-nio, swift-nio-ssl, swift-nio-http2, swift-nio-extras, swift-nio-transport-services, swift-nio-ssh) reveals 530 call sites for these methods. Each of these call sites will need to be manually audited and rewritten to use the appropriate invocation.

A related note is that it's not uncommon to declare methods like this as well, so this change has to propagate through the ecosystem. This forces some more churn there too: I count 35 such methods in the core NIO repos listed above.

None of this is the end of the world, but I think it does prompt the question of whether the downside of these APIs justifies the cost of migrating them. I don't have good numbers for how this affects repos outside the core NIO repo, so it's hard to be confident of what the total ecosystem churn is. The result is that I'm on the fence: this change produces a lot of work, but I'm not sure how much it helps.

Some other info:

• A quick search of the vapor/vapor repo shows 7 usage sites: all get-based. 6 are the equivalent of peek, one is a misuse of get(at: 0).
• A quick search of the RediStack repo shows 9 usage sites: all get-based. 4 are the equivalent of peek, one is a debug hook that misuses get(at: 0) (low-impact), 3 are correct-but-dangerous uses of get(at: 0) where the reader index is definitely zero but peek could be used instead, and one is a correct use of get(at:) that projects indices from readableBytesView into the buffer.