RFC: declare public API

[weissi]

Motivation:

SemVer requires us to declare a public API, we should do so for NIO 2.

Modifications:

declare our public API.

Result:

• more transparency into our versioning policy.
• resolves NIO needs to clarify its minor-versioning rules #405

[weissi]

CC @helje5 / @tanner0101 / @ianpartridge / @pushkarnk / @MrMage / @tachyonics / @kjessup / @normanmaurer / @Lukasa / @Joannis / @jrose-apple / @0xTim

[ianpartridge]

I don't quite follow, doesn't NIO 1 declare a public API? How is that different from what you're proposing?

[ktoso]

Rules look good! Nice to specify more explicitly what is supported and what could get one into trouble.
Noticed some wording improvements, hope you don't mind :)

[ktoso]

nitpick, "it is" (as elsewhere)

[Lukasa]

While we're here, can we capitalise the first words of all of these points? They're full sentences.

[moiseev]

I believe the initializers should be called out specifically. Since in case of an initializer the _ goes to the argument label (or parameter name). For example:

struct S {
  public // not really
  init(_internalWannabe: x: Whatever) { ... }
}

At least these are the current rules for hiding symbols in SourceKit.

[ktoso]

this Recommendation -> these Guidelines?

[ktoso]

the - formatting is a bit hard to read to be honest; Since the new point is very close to the end of the examples/reasoning of previous one; Something to separate would be useful, be it a rule number or a --- ?

Or like:

Extensions to NIO types (level 3 or level 4)

what is allowed...
Why?
examples

[ktoso]

While that's true and cool, the first sentence sounds a bit too soft to me... Perhaps start re stating that ignoring those rules may cause you to have to rewrite things etc, and then mention the happy news that there is a compat suite so you'd get heads up (but still MAY need to change things if team decides to have to change such internal API etc)

[ktoso]

typo: confirm -> conform

[weissi]

@ianpartridge well, we don't have a document specifying what we consider public API which makes SemVer unclear. We have always operated under pretty much the principles states in this doc (except that we introduced new types not prefixed with NIO). So yes, NIO 1 has a public API but we never declared what we're okay with you doing with it, which is just a detail but it has lead to quite a few discussions here and there...

Without declaring what exactly we consider public API, one might interpret SemVer the way that anything that might break any potentially existing project is SemVer major.
But in Swift this interpretation means that that adding anything public would be a new major version. That would make SemVer way less useful because the minor version digit would never be incremented as there is no such thing as an addition that might not possibly break anyone. We'd only have: major+1 when adding or removing anything; patch+1 if just fixing a bug without API changes. That's not a very useful reading of SemVer because what you really want is:

• something that might break me: major+1
• new features that shouldn't break me: minor+1
• bug fixes: patch+1

With this document we hope to clarify what additions to our APIs we consider SemVer minor. But really nothing changes :)

[ianpartridge]

OK thanks. I think this is valuable for the SPM ecosystem in general and it would be good if some of this could go "upstream" to there :)

[Lukasa]

Nit: space after closing parenthesis.

[Lukasa]

Nit: replace "requires to" with "requires projects to".

[Lukasa]

Blank line above this please.

[Lukasa]

While we're here, can we capitalise the first words of all of these points? They're full sentences.

[Lukasa]

A blank line above this please.

[Lukasa]

Missing trailing period.

[Lukasa]

Nit: "others'"

[Lukasa]

Nit: replace "strongly recommend to depend" with "strongly recommend depending"

[Lukasa]

Nit: downcase "you"

[Lukasa]

Italics and a parenthetical? How dramatic! I'd recommend either only the italics or only the parenthetical.

[jrose-apple]

It's nice to have something like this written up, especially in a language that allows overloading and type inference. It goes with the discussions that have been happening on the forums too.

[jrose-apple]

This one seems phrased a little on the restrictive side to me. Unlike in Objective-C, there's no run-time conflict when this rule is broken, and eventually we'll have a way to disambiguate.

If you wanted to split the difference, maybe this should be qualified to "public API"? Helpers within a module are generally acceptable (though I should double-check that we actually prefer them in tie-breakers).

The condition you're using may not be sufficient to cover everything, either, depending on how you phrase it. I could have a parameter of type Array<MyType>, but [] wouldn't uniquely identify it.

[weissi]

@jrose-apple thank you! This was meant to be public extensions. This demonstrates the clash I mean:

(everything below a // MODULE: xyz would be in that module)

import BadModule
import RandomClient

doIt()
doItBad()

// MODULE: NIO
public struct ByteBuffer {
    public init() {
    }
    public func foo() {
        print("NIO.foo")
    }
}

// MODULE: BadModule
import NIO

public extension ByteBuffer {
    func foo() {
        print("BadModule.foo")
    }
}

public func doItBad() {
    ByteBuffer().foo()
}

// MODULE: RandomClient

import NIO
import BadModule

public func doIt() {
    ByteBuffer().foo()
}

which would get you

Compile Swift Module 'NIO' (1 sources)
Compile Swift Module 'BadModule' (1 sources)
Compile Swift Module 'RandomClient' (1 sources)
/private/tmp/.gen-swift-perf-test_vUBGJT/Sources/RandomClient/module.swift:35:1: error: ambiguous use of 'foo()'
ByteBuffer().foo()
^
NIO.ByteBuffer:3:17: note: found this candidate
    public func foo()
                ^
BadModule.ByteBuffer:2:17: note: found this candidate
    public func foo()
                ^

internal helpers are always fine here and they would be preferred which is what we want 👍

[MrMage]

Sorry, minor typo drive-by: s/confirm/conform/

[jrose-apple]

Hm. But what if SwiftNIO grows a new exported dependency?

[weissi]

if we vendor new stuff, we symbol-prefix everything:

• https://github.com/apple/swift-nio/blob/master/Sources/CNIOHTTPParser/update_and_patch_http_parser.sh#L48-L65
• https://github.com/apple/swift-nio/blob/master/Sources/CNIOSHA1/update_and_patch_sha1.sh#L43
• https://forums.swift.org/t/rfc-moving-swiftnio-ssl-to-boringssl/18280

you're right thought that if we added a new shared library dependency this might be an issue. I guess we should not do that...
But the same issue exists for all shared libraries we or Swift itself depends on.

@Lukasa / @normanmaurer should we do any guarantees here? I'd be fine to say that we won't add any further shared library dependencies without a new major version.

[jrose-apple]

What if your dependencies grow a new exported dependency?

(I'm trying to reductio ad absurdum here. Swift's namespacing model just doesn't allow this guarantee in practice.)

[weissi]

@jrose-apple thank you, this is very valuable! So what I'm trying to say (and I'll try to rephrase this) is that we won't add any C symbols into our binaries. We have done this (http_parser.c wasn't name-spaced in NIO 1.0.0) and it lead to problems because people combined NIO with something else that also vendored a non-name-spaced http_parser.c.
I understand that Swift's namespacing model doesn't allow those guarantees but so far Swift mangles enough information into the symbol names that we didn't suffer any of those clashes in reality. For the vendored C stuff however this was totally different and our users have actually hit this which is why we symbol-prefix everything C now.

We're also in a pretty unique spot with NIO that we don't have many dependencies at all. NIO core dynamically links the Swift stdlib, runtime, etc (not Foundation), zlib, libc and nothing else and we're not changing this. The rule has always been: Anything that depends on other system libraries goes into a separate package (therefore swift-nio-ssl and swift-nio-transport-services). And I don't think swift-nio-ssl/transport-services should grow a new dependency without a fresh major version. All of our packages so far only depend on only NIO stuff.

I believe this should basically get us to a place where if NIO 2.x.m works fine, the user types swift package update gets 2.y.n (where y >= x) and there's no way it doesn't work (unless the user has their own modules/C functions with CNIO,c_nio,NIO, ... prefixes and we added one that clashed). Do you think this is accurate?

$ nm ./.build/x86_64-apple-macosx10.10/debug/NIOTLSServer | grep ' T ' | grep -v -e NIO -e catmc -e c_nio
0000000100000000 T __mh_execute_header
00000001001a8010 T _main
$ nm ./.build/x86_64-apple-macosx10.10/debug/NIOHTTP1Server | grep ' T ' | grep -v -e NIO -e catmc -e c_nio
0000000100000000 T __mh_execute_header
00000001001aecc0 T _main

(catmc is for historical reasons the prefix for our C atomics but they have been there since 1.0.0 so nothing got added and we should really rename them to c_nio_)

$ nm ./.build/x86_64-apple-macosx10.10/debug/NIOHTTP1Server | grep -i ' U ' | grep -v -e ' _swift' -e ' _$S' -e ' _pthread' -e ' _obj' -e ' _OBJ' -e ' _dispatch' -e ' __swift' -e ' _class' -e ' _CF' -e ' __objc' -e ' __Block' -e ' _protocol' -e ' _proper'
                 U __DefaultRuneLocale
                 U __NSConcreteStackBlock
                 U ___assert_rtn
                 U ___error
                 U ___memmove_chk
                 U ___memset_chk
                 U ___stack_chk_fail
                 U ___stack_chk_guard
                 U __dyld_register_func_for_add_image
                 U __stdlib_malloc_size
                 U _abort
                 U _accept
                 U _asprintf
                 U _bind
                 U _bzero
                 U _calloc
                 U _close
                 U _connect
                 U _deflate
                 U _deflateBound
                 U _deflateEnd
                 U _deflateInit2_
                 U _dup
                 U _errx
                 U _free
                 U _freeaddrinfo
                 U _freeifaddrs
                 U _getaddrinfo
                 U _getifaddrs
                 U _getpeername
                 U _getpid
                 U _getsockname
                 U _getsockopt
                 U _gettimeofday
                 U _hash_create
                 U _hash_search
                 U _if_nametoindex
                 U _inet_ntop
                 U _inet_pton
                 U _ivar_getName
                 U _ivar_getOffset
                 U _kCFCoreFoundationVersionNumber
                 U _kevent
                 U _kqueue
                 U _listen
                 U _log2
                 U _lseek
                 U _malloc
                 U _memchr
                 U _memcmp
                 U _memcpy
                 U _memset
                 U _method_setImplementation
                 U _read
                 U _realloc
                 U _recvfrom
                 U _sel_getUid
                 U _sendfile
                 U _sendmsg
                 U _sendto
                 U _setsockopt
                 U _shutdown
                 U _socket
                 U _strcmp
                 U _strerror
                 U _strlen
                 U _strncmp
                 U _sysconf
                 U _write
                 U _writev
                 U dyld_stub_binder

[jrose-apple]

I guess it's just as possible that zlib adds an unprefixed function, but because you control your dependencies so tightly you might be able to get away with this.

[moiseev]

Nitpick: our users' projects – I believe the apostrophe is attached to a wrong word.

[weissi]

thank you @moiseev, @jrose-apple, @Lukasa and @ktoso , took all your comments. Let me know what you think about v2 :)

[tanner0101]

SSWG documents make use of serial comma, so it might make sense to follow suit in this document as well for consistency.

[tanner0101]

those guidelines

Should probably say these like the title does.

[tanner0101]

Should this specify explicitly "public" or "open" methods and types? For instance, using @testable import you could possibly get access to unstable methods not prefixed with _.

[Lukasa]

Maybe we should say "exported"?

[Lukasa]

Nit: "i.e."

[Lukasa]

We may want to say here that NIO may conform its type to said protocol, or the package that provides the protocol may conform the NIO type to their protocol.

[Lukasa]

And here, maybe say that NIO might add this conformance or the owner of the type may add the conformance.

[Lukasa]

Do we need the backticks here? Should we just say "a major version greater than 1"?

[Lukasa]

I still think the parentheses and the italics is excessive. :D

[helje5]

As discussed with @weissi OOB, I'm not sure that this is necessary. Shadowing should work here too, no? (i.e. the new type doesn't introduce compiler amgiguity?) (this specific section lacks the "What's the point of this restriction?" :-)

This is slightly off-topic, but IMO all stuff should either be prefixed NIO or not, for API consistency. When looking at the sidebar of https://apple.github.io/swift-nio/docs/current/NIO/index.html those NIO types immediately hit your eye as outliers.
Right now NIO mostly gets away w/o prefixes because it just renamed all types Foundation already has (EventLoop instead of RunLoop etc, i.e. it used the Java terminology). I would personally either continue doing this (and e.g. event new names for NIOAny, maybe ChannelBucket?), or prefix everything for consistency and ideally rename stuff to match Foundation (e.g. NIORunLoop instead of EventLoop).

[weissi]

@helje5 let's assume three modules:

1. NIO which in version 2.0 does not declare a struct Tomato
2. Caprese which declares a public struct Tomato
3. MyApp

Let's also assume that MyApp has both import Caprese and import NIO and it uses Tomato (implicitly from Caprese for example like this):

let t = Tomato()

If now in version NIO in version 2.1.0 introduces a public struct Tomato { ... } as well, then now compilation of MyApp will fail will an ambiguity error.

The only way to resolve it is that NIO introduces it as NIOTomato and everything is good. Does that make sense?

[weissi]

btw, event loop is also not "Java terminology" it's just a standard term for a mechanism that dispatches events into a program. Run loop is less specific.

[helje5]

Makes sense, maybe you want to add the explanation to the document (like you did in the other sections).
(P.S.: I wanted to say "Netty terminology", but that is nitpicking. I think the point in itself is valid).

[weissi]

@helje5 added a paragraph, useful?

[weissi]

@helje5 this is what I added.

[IOOI-SqAR]

is there a reason why this isn't scheduled for the 2.0.0 Milestone?

[weissi]

@Lukasa what do you think. Is this good to go as a 1st pass?

[Lukasa]

Yup, looks good to me.