RFC: go back to swift 5

[tomerd]

motivation: ease integration with middle-tier libraries that want to continue and support older versions of swift

changes:

• set min tools-version to 5.0
• drop use of property wrappers
• generate linux tests
• other small adjustment to swift 5 code style

[tomerd]

@helge5 @fabianfett this is what going back to 5.0 look like. biggest pain is loosing property wrappers and needing to generate linux tests. 5.1 may be a more compelling proposition, but I am open to both

[fabianfett]

I'm not a big fan, of the date.wrappedValue approach. IMHO how the date decoding is done should be an implementation detail and nothing that is exposed to the enduser in this way (forced to use .wrappedValue).

If we want to support 5.0 I would love to see the decoding implement in the structs itself. (we can just copy and paste code from my old repo).

@tomerd wdyt? I could do this...

[tomerd]

hi @fabianfett the end user is not exposed to wrappedValue, this is only exposed in the tests. here is an example of the impl - publicly the end user will see a Date just like before

public enum SNS {
    public struct Message {
        public enum Attribute {
            case string(String)
            case binary([UInt8])
        }

        public let signature: String
        public let messageId: String
        public let type: String
        public let topicArn: String
        public let messageAttributes: [String: Attribute]
        public let signatureVersion: String
        public let signingCertURL: String
        private var _timestamp: ISO8601WithFractionalSecondsCoding
        public var timestamp: Date {
            get { return self._timestamp.wrappedValue }
            set { self._timestamp = ISO8601WithFractionalSecondsCoding(wrappedValue: newValue) }
        }

        public let message: String
        public let unsubscribeUrl: String
        public let subject: String?
    }
}

extension SNS.Message: Decodable {
    enum CodingKeys: String, CodingKey {
        case signature = "Signature"
        case messageId = "MessageId"
        case type = "Type"
        case topicArn = "TopicArn"
        case messageAttributes = "MessageAttributes"
        case signatureVersion = "SignatureVersion"
        case _timestamp = "Timestamp"
        case signingCertURL = "SigningCertUrl"
        case message = "Message"
        case unsubscribeUrl = "UnsubscribeUrl"
        case subject = "Subject"
    }
}

the other approach is of course to do alway with the wrapper struct and decode/encode manually, but my hope was that we won't need to support 5.0 forever and so the code as proposed in this PR is more "ready" for putting property wrappers back

[ktoso]

I'm not sure tracing works on 5.0 (or could work, with the dynamic member lookup keypath magic...) I'll check and report back.

---

[stevapple]

If we give up PropertyWrappers, we should write custom encode and decode methods. I’m doing such things in this fork.

However, since the first official release of Amazon Linux 2 toolchain is 5.2.4, I don’t think it’s really necessary to support 5.0 and 5.1. Although Swift 5 provides an improved backward support, this will make the code a lot uglier and less flexible.

[helje5]

Is it necessary to declare the Package.swift as 5.2 to get the automatic tests on Linux? Shouldn't that still work if you use an actual Swift 5.2 toolchain to process/test the 5.0 package?

If features are optional, you can still use 5.x, you'd just have to guard such with #if swift(>=5.1).

For info about this change (whether it is going to happen or not), see the long thread in the Slack. It's a package resolution issue (for libraries where Lambda is an option, not a requirement), not a deployment issue.

[helje5]

Wrt the @ISO8601WithFractionalSecondsCoding, I'm not a fan of such things, it feels very much like a hack. I'd either do that in init(coder:), or better: in a custom Decoder which can deal with those inputs (i.e. adds appropriate date decoding strategies). (IMHO)

[helje5]

I'm not sure tracing works on 5.0 (or could work, with the dynamic member lookup keypath magic...) I'll check and report back.

I think 4.2 added the string based dynamicMemberLookup, 5.0 added dynamicCallable. The "static" dynamicMemberLookup stuff was 5.1 or 5.2. (I think)

[tomerd]

Is it necessary to declare the Package.swift as 5.2 to get the automatic tests on Linux? Shouldn't that still work if you use an actual Swift 5.2 toolchain to process/test the 5.0 package?

@helje5 if we are declaring that the package is compatible with tools-version 5.0 we need to be able and test it on that toolchain, so we are forced to generate the linuxmain for that to work. in >= 5.1 we would test with --enable-test-discovery

[pokryfka]

Personally I am not sure eager decoding of date is a necessary at all; note that:

• JSON does not define any date type
• Amazon uses string, integer and number (can be floating point) types to encode dates using different formats
• encoded values may at times be slightly different than referenced standards or have nuances, see https://github.com/swift-server/swift-aws-lambda-runtime/blob/f731664c4d5e751f0268c8da099e279d1d9ee9cb/Sources/AWSLambdaEvents/Utils/DateWrappers.swift#L114
• types defined in AWSLambdaEvents already use different date types:
  • Foundation.Date decoded from JSON number type, see https://github.com/swift-server/swift-aws-lambda-runtime/blob/f731664c4d5e751f0268c8da099e279d1d9ee9cb/Sources/AWSLambdaEvents/DynamoDB.swift#L121
  • Foundation.Date in a property wrappers (Swift 5.1), most of the time decoded from JSON string
  • String and UInt64 in APIGateway.V2, see in https://github.com/swift-server/swift-aws-lambda-runtime/blob/f731664c4d5e751f0268c8da099e279d1d9ee9cb/Sources/AWSLambdaEvents/APIGateway%2BV2.swift#L54
• using Foundation.Date type adds dependency on Foundation which has performance penalty
• locale aware date formatters are not very fast (especially during cold start)

This may vary case to case, I considered my very own uses of date attributes:

• most of a time I dont use it at all;
• I use it for logging - String is just fine, no need to decode and encode it; or
• maybe part of an identifier, for example part the range key in DynamoDb table - String is just fine

For my very personal use I decided to use String types (or unsigned integer if "unix time" is provided).

[ktoso]

Overall: I'm not sure the supporting of all the way 5.0 in newly developed libraries is the highest priority... We have to keep moving forward, and incentivize doing so.

Specifically: Swift-Tracing and it's way to declare attributes in a pleasant (non insane) and type-safe way depends on typesafe dynamic member lookup. This won't work on Swift 5.0 ever, and could 5.1 but I'm hitting some compiler crashes right now (could be my environment's fault, not sure).

To be specific what we'd lose:

Attributes are normally plain APIs where string keys and a predefined set of value types are allowed:

import TracingInstrumentation

span.attributes["http.url"] = "\(niorequest.uri)
span.attributes["http.method"] = "\(niorequest.method)"

Though we're able to add industry specified well known values, and not tie the core library to those standards (as we do not want to, because we may diverge in the core slightly):

import OpenTelemetrySemanticConventions // (currently named OpenTelemetryInstrumentationSupport)

span.attributes.http.url = niorequest.uri // type-safe
span.attributes.http.method = niorequest.method // type-safe
// AND do note that this is type-safe, user extensible and can autocomplete / show docs etc, 
// so even newcomers will set the _right_ attributes

or even pre-baked "set all the interesting attributes":

import OpenTelemetrySemanticConventionsNIO // does not exist (yet)

// and we can even optionally do:
span.attributes.http.request = niorequest

// which would populate all the "we want to keep those" attributes (there can be _many_)

This depends on

    subscript<T>(dynamicMember dynamicMember: KeyPath<NestedAttributes, SpanAttributeKey<T>>) -> T? where T: SpanAttributeConvertible { get set }

Which is not a thing in early Swift 5 releases. We worked on this specific API spelling with Swift team to ensure it not only looks great but also will be efficient, and our goal with the swift-tracing API is to provide really best in class and "awesome to use" API to those things. Thus, I don't think we should drop the language requirement lower.

Now, Lambda should depend on swift-tracing because it should startSpan and end spans automatically on behalf of users, and thanks to the xRay backend users will automatically get the full tracing experience end to end. Thus, lambda can require at least 5.1 (if I figure out the compiler crashes), but definitely not 5.0.

Summing up though: I don't think it is unreasonable to require new projects to support "two language versions back", this makes a lot of sense, but I don't think the 5.0 requirement is reasonable. 5.1 is on the edge... We can/should try to make tracing work there and I'll give it some more looking why that compiler crash I'm seeing now. But realistically I think new libraries now should aim to support 5.2 and 5.3 right now, and we'll make a "special effort" for the 5.1 IMO.

I'll try a bit harder and see if we can get tracing to support 5.1, if that'd help?

[pokryfka]

@ktoso

I dont know min Swift version requirement as far as Lambda is concerned, SwiftNIO 2 supports Swift 5.0
and arguably all libraries using SwiftNIO should be able to use and benefit from swift-tracing.

or even pre-baked "set all the interesting attributes":

import OpenTelemetrySemanticConventionsNIO // does not exist (yet)

// and we can even optionally do:
span.attributes.http.request = niorequest

this could also be achieved with a type safe extension without dynamic member lookup and without (required by subscript) attributes getter (which goes against OTel recommendations), example from https://github.com/pokryfka/aws-xray-sdk-swift/blob/master/Sources/AWSXRayRecorder/Segment%2BNIOHTTP.swift:

import NIOHTTP1
 
extension XRayRecorder.Segment {
    public func setHTTPRequest(_ request: HTTPRequestHead) {
        let userAgent = request.headers["User-Agent"].first
        let clientIP = request.headers["X-Forwarded-For"].first
        setHTTPRequest(method: request.method.rawValue, url: request.uri, userAgent: userAgent, clientIP: clientIP)
    }
}

[helje5]

I don't think it is unreasonable to require new projects to support "two language versions back"

I 100% agree with that, that would be Swift 4 and 5. It is reasonable to expect support for both if you are really serious about all this (like IBM has been).
For my own projects I decided to start being lazy and only support 5.0+ (essentially stick to what NIO 2 is doing, though NIO 1 is also still available, and I did support that for quite a while to have Swift 4).

new libraries

My complaint isn't about new libraries. It's about adding (optional) Lambda support to existing things (either libraries or apps). That's what started this whole thing. I'd also like to repeat that you don't have to do this for me, I personally don't really care that much, though I think you should absolutely do it. The Lambda is conceptually "infrastructure software", i.e. not a leaf module like say SwiftBlocksUI. As such, it should support the lowest possible version.

Besides existing code, another issue are platforms which may not have the latest toolchain. Which funny enough affects Amazon Linux itself :-) (no 5.3 yet) More commonly Raspi's.

[ktoso]

With due respect, the Swift 4 support requirement is your opinion and not a fact; and the passive aggressive nitpicking tone is not welcome or helpful in getting the situation improved.

Looping back to the actual task though:

I think we can #if swift/compiler(>=5.1/2) around the subscript based features causing issues on Swift 5.1 (and not existing on 5.0). The 5.1 compiler crash I'm not sure we'll get a fix or workaround for, so we may need to disable it there too. (For that matter, it would allow 5.0 users as well). Those can nice accessors can be seen as "optional", because users can keep using the string based not-so-well-typed API (like @pokryfka mentions).

We would not offer mirroed "set... style" APIs I think, though. It is a specific goal of those packages to expose the best "swifty" API for those types as possible, even if we have to require e.g. > 5.1 for the nicer API.

It would allow 5.1/5.0 users, so that's a win, and would enable this PR -- if we want to merge it, that's a road we can take, though I'll leave the final decision how we want to approach supporting Swift versions up to @tomerd. I do not think we're committing "in general" to always support a X.0 though, but here we can take the extra effort if necessary.

[ktoso]

FYI: For what it's worth we ensured compatibility of swift-tracing with 5.0+ now: slashmo/gsoc-swift-tracing@1986c52 Thanks to @slashmo for taking on the work 👍

Lamba is free to make its own decision, whatever it might be -- would not be hampered by swift-tracing.

[fabianfett]

SwiftNIO now only goes back to Swift 5.2 and we support that.