ronantakizawa/codereview-bench huggingface dataset:
Repository: vapor/vapor
File: Sources/Vapor/URLEncodedForm/URLEncodedFormSerializer.swift (Swift)

Code before:
    }
}

// MARK: Utilities

extension String {
    /// Prepares a `String` for inclusion in form-urlencoded data.
    func urlEncoded(codingPath: [CodingKey] = []) throws -> String {
        guard let result = self.addingPercentEncoding(
            withAllowedCharacters: Characters.allowedCharacters
        ) else {
            throw EncodingError.invalidValue(self, EncodingError.Context(
                codingPath: codingPath,
                debugDescription: "Unable to add percent encoding to \(self)"
            ))
        }
        return result
    }
}

/// Characters allowed in form-urlencoded data.
private enum Characters {
    static let allowedCharacters: CharacterSet = {
        var allowed = CharacterSet.urlQueryAllowed
        // these symbols are reserved for url-encoded form
        allowed.remove(charactersIn: "?&=[];+$")
        return allowed
    }()
}


Review comment (suggestion):
We should inverse this completely as per https://url.spec.whatwg.org/#application-x-www-form-urlencoded-percent-encode-set.

Essentially if it _isn't_ alphanumeric, `*`, `-`, `.` or `_` we should encode it

Code after:
    }
}

// MARK: Utilities

extension String {
    /// Prepares a `String` for inclusion in form-urlencoded data.
    func urlEncoded(codingPath: [CodingKey] = []) throws -> String {
        guard let result = self.addingPercentEncoding(
            withAllowedCharacters: Characters.allowedCharacters
        ) else {
            throw EncodingError.invalidValue(self, EncodingError.Context(
                codingPath: codingPath,
                debugDescription: "Unable to add percent encoding to \(self)"
            ))
        }
        return result
    }
}

/// Characters allowed in form-urlencoded data.
private enum Characters {
    // https://url.spec.whatwg.org/#application-x-www-form-urlencoded-percent-encode-set
    static let allowedCharacters: CharacterSet = {
        var allowed = CharacterSet.alphanumerics
        allowed.insert(charactersIn: "*-._")
        return allowed
    }()
}
Repository: vapor/vapor
File: Sources/Vapor/Commands/ServeCommand.swift (Swift)

Code before:
        }
        makeSignalSource(SIGTERM)
        makeSignalSource(SIGINT)
        self.box.withLockedValue { $0 = box }
    }

    @available(*, noasync, message: "Use the async asyncShutdown() method instead.")
    func shutdown() {
        var box = self.box.withLockedValue { $0 }
        box.didShutdown = true
        box.running?.stop()
        if let server = box.server {
            server.shutdown()
        }
        box.signalSources.forEach { $0.cancel() } // clear refs
        box.signalSources = []
        self.box.withLockedValue { $0 = box }
    }
    
    func asyncShutdown() async {
        var box = self.box.withLockedValue { $0 }
        box.didShutdown = true
        box.running?.stop()
        if let server = box.server {
            await server.shutdown()
        }
        box.signalSources.forEach { $0.cancel() } // clear refs
        box.signalSources = []
        self.box.withLockedValue { $0 = box }
    }
    
    deinit {
        assert(self.box.withLockedValue({ $0.didShutdown }), "ServeCommand did not shutdown before deinit")
    }
}


Review comment (suggestion):
```suggestion
        await box.server?.shutdown()
```

Code after:
        }
        makeSignalSource(SIGTERM)
        makeSignalSource(SIGINT)
        self.box.withLockedValue { $0 = box }
    }

    @available(*, noasync, message: "Use the async asyncShutdown() method instead.")
    func shutdown() {
        var box = self.box.withLockedValue { $0 }
        box.didShutdown = true
        box.running?.stop()
        if let server = box.server {
            server.shutdown()
        }
        box.signalSources.forEach { $0.cancel() } // clear refs
        box.signalSources = []
        self.box.withLockedValue { $0 = box }
    }
    
    func asyncShutdown() async {
        var box = self.box.withLockedValue { $0 }
        box.didShutdown = true
        box.running?.stop()
        await box.server?.shutdown()
        box.signalSources.forEach { $0.cancel() } // clear refs
        box.signalSources = []
        self.box.withLockedValue { $0 = box }
    }
    
    deinit {
        assert(self.box.withLockedValue({ $0.didShutdown }), "ServeCommand did not shutdown before deinit")
    }
}
Repository: vapor/vapor
File: Sources/Vapor/Utilities/URI.swift (Swift)

Code before:

extension CharacterSet {
    /// The set of characters allowed in a URI scheme, as per [RFC 3986 § 3.1].
    ///
    /// [RFC 3986 § 3.1]: https://datatracker.ietf.org/doc/html/rfc3986#section-3.1
    fileprivate static var urlSchemeAllowed: Self {
        // Intersect the alphanumeric set plus additional characters with the host-allowed set to ensure
        // we get only ASCII codepoints in the result.
        .urlHostAllowed.intersection(.alphanumerics.union(.init(charactersIn: "+-.")))
    }
    
    /// The set of characters allowed in a URI path, as per [RFC 3986 § 3.3].
    ///
    /// > Note: This is identical to the built-in `urlPathAllowed` on macOS; on Linux it adds the missing
    /// > semicolon character to the set.
    ///
    /// [RFC 3986 § 3.3]: https://datatracker.ietf.org/doc/html/rfc3986#section-3.3
    fileprivate static var urlCorrectPathAllowed: Self {
        .urlPathAllowed.union(.init(charactersIn: ";"))
    }
}

/// On Linux and in older Xcode versions, URLComponents incorrectly treats `;` as *not* allowed in the path component.
fileprivate let urlPathAllowedIsBroken: Bool = {
    return CharacterSet.urlPathAllowed != CharacterSet.urlCorrectPathAllowed
}()


Review comment (bug):
```suggestion
private let urlPathAllowedIsBroken: Bool = {
    CharacterSet.urlPathAllowed != CharacterSet.urlCorrectPathAllowed
}()
```

Code after:
            .replacingOccurrences(of: "]", with: "%5D", options: .literal)
    }
}

extension CharacterSet {
    /// The set of characters allowed in a URI scheme, as per [RFC 3986 § 3.1].
    ///
    /// [RFC 3986 § 3.1]: https://datatracker.ietf.org/doc/html/rfc3986#section-3.1
    fileprivate static var urlSchemeAllowed: Self {
        // Intersect the alphanumeric set plus additional characters with the host-allowed set to ensure
        // we get only ASCII codepoints in the result.
        .urlHostAllowed.intersection(.alphanumerics.union(.init(charactersIn: "+-.")))
    }
    
    /// The set of characters allowed in a URI path, as per [RFC 3986 § 3.3].
    ///
    /// > Note: This is identical to the built-in `urlPathAllowed` on macOS; on Linux it adds the missing
    /// > semicolon character to the set.
    ///
    /// [RFC 3986 § 3.3]: https://datatracker.ietf.org/doc/html/rfc3986#section-3.3
    fileprivate static var urlCorrectPathAllowed: Self {
        .urlPathAllowed.union(.init(charactersIn: ";"))
    }
}

/// On Linux and in older Xcode versions, URLComponents incorrectly treats `;` as *not* allowed in the path component.
private let urlPathAllowedIsBroken: Bool = {
    CharacterSet.urlPathAllowed != CharacterSet.urlCorrectPathAllowed
}()
Repository: vapor/vapor
File: Sources/Vapor/Utilities/RFC1123.swift (Swift)

Code before:
#if swift(>=5.10)
#if canImport(Darwin)
@preconcurrency import Darwin
#elseif canImport(Glibc)
@preconcurrency import Glibc
#elseif canImport(Musl)
@preconcurrency import Musl
#elseif canImport(WinSDK)
@preconcurrency import WinSDK
#endif
#endif
import Foundation
import NIOPosix
import NIOCore
import NIOConcurrencyHelpers

/// An internal helper that formats cookie dates as RFC1123
private final class RFC1123 {
    /// A static RFC1123 helper instance
    static var shared: RFC1123 {
        .init()
    }
    
    /// The RFC1123 formatter
    private let formatter: DateFormatter
    
    /// Creates a new RFC1123 helper
    private init() {
        let formatter = DateFormatter()
        let enUSPosixLocale = Locale(identifier: "en_US_POSIX")
        formatter.locale = enUSPosixLocale
        formatter.timeZone = TimeZone(secondsFromGMT: 0)
        formatter.dateFormat = "EEE, dd MMM yyyy HH:mm:ss z"
        formatter.calendar = Calendar(identifier: .gregorian)
        self.formatter = formatter
    }
    
    func string(from date: Date) -> String {
        return formatter.string(from: date)
    }
    
    func date(from string: String) -> Date? {
        return formatter.date(from: string)
    }
}


Review comment (performance):
Doesn't `RFC1123` need to be `Sendable`? This is my one concern with this is this is used for the `Response` so could introduce a slow down but I don't think we have much choice with a/a

Code after:
#if swift(>=5.10)
#if canImport(Darwin)
@preconcurrency import Darwin
#elseif canImport(Glibc)
@preconcurrency import Glibc
#elseif canImport(Musl)
@preconcurrency import Musl
#elseif canImport(WinSDK)
@preconcurrency import WinSDK
#endif
#endif
import Foundation
import NIOPosix
import NIOCore
import NIOConcurrencyHelpers

/// An internal helper that formats cookie dates as RFC1123
private final class RFC1123: Sendable {
    /// A static RFC1123 helper instance
    static var shared: RFC1123 {
        .init()
    }
    
    /// The RFC1123 formatter
    private let formatter: DateFormatter
    
    /// Creates a new RFC1123 helper
    private init() {
        let formatter = DateFormatter()
        let enUSPosixLocale = Locale(identifier: "en_US_POSIX")
        formatter.locale = enUSPosixLocale
        formatter.timeZone = TimeZone(secondsFromGMT: 0)
        formatter.dateFormat = "EEE, dd MMM yyyy HH:mm:ss z"
        formatter.calendar = Calendar(identifier: .gregorian)
        self.formatter = formatter
    }
    
    func string(from date: Date) -> String {
        self.formatter.string(from: date)
    }
    
    func date(from string: String) -> Date? {
        self.formatter.date(from: string)
    }
}
Repository: vapor/vapor
File: Sources/Vapor/HTTP/BasicResponder.swift (Swift)

Code before:
import ServiceContextModule

/// A basic, closure-based `Responder`.
public struct BasicResponder: Responder {
    /// The stored responder closure.
    private let closure: @Sendable (Request) throws -> EventLoopFuture<Response>

    /// Create a new `BasicResponder`.
    ///
    ///     let notFound: Responder = BasicResponder { req in
    ///         let res = req.response(http: .init(status: .notFound))
    ///         return req.eventLoop.newSucceededFuture(result: res)
    ///     }
    ///
    /// - parameters:
    ///     - closure: Responder closure.
    @preconcurrency public init(
        closure: @Sendable @escaping (Request) throws -> EventLoopFuture<Response>
    ) {
        self.closure = closure
    }

    /// See `Responder`.
    public func respond(to request: Request) -> EventLoopFuture<Response> {
        do {
            return try ServiceContext.withValue(request.serviceContext) {
                try closure(request)
            }
        } catch {
            return request.eventLoop.makeFailedFuture(error)
        }
    }
}


Review comment (performance):
My one concern with this is a potential performance impact and whether we should put it behind a flag or not, given most people won't use it.

Thoughts @gwynne 

Code after:

/// A basic, closure-based `Responder`.
public struct BasicResponder: Responder {
    /// The stored responder closure.
    private let closure: @Sendable (Request) throws -> EventLoopFuture<Response>

    /// Create a new `BasicResponder`.
    ///
    ///     let notFound: Responder = BasicResponder { req in
    ///         let res = req.response(http: .init(status: .notFound))
    ///         return req.eventLoop.newSucceededFuture(result: res)
    ///     }
    ///
    /// - parameters:
    ///     - closure: Responder closure.
    @preconcurrency public init(
        closure: @Sendable @escaping (Request) throws -> EventLoopFuture<Response>
    ) {
        self.closure = closure
    }

    /// See `Responder`.
    public func respond(to request: Request) -> EventLoopFuture<Response> {
        do {
            return try request.propagateTracingIfEnabled {
                try closure(request)
            }
        } catch {
            return request.eventLoop.makeFailedFuture(error)
        }
    }
}
Repository: vapor/vapor
File: Sources/Vapor/Request/Request.swift (Swift)

Code before:
        self._logger = .init(logger)
        self._serviceContext = .init(.topLevel)
        
        let storageBox = RequestBox(
            method: method,
            url: url,
            version: version,
            headers: headers,
            isKeepAlive: true,
            route: nil,
            parameters: .init(),
            byteBufferAllocator: byteBufferAllocator
        )
        self.requestBox = .init(storageBox)
        self.id = requestId
        self.application = application
        
        self.remoteAddress = remoteAddress
        self.eventLoop = eventLoop
        self._storage = .init(.init())
        self.bodyStorage = .init(bodyStorage)
    }
    
    /// Automatically restores tracing serviceContext around the provided closure
    func propagateTracingIfEnabled<T>(_ closure: () throws -> T) rethrows -> T {
        if self.application.traceAutoPropagation {
            return try ServiceContext.withValue(self.serviceContext) {
                try closure()
            }
        } else {
            return try closure()
        }
    }
}


Review comment (suggestion):
Can we store this directly on the `Request` as it doesn't need to be mutable on `Request`. That avoids us having to access a lock on every call to this function, which is pretty often per request.

E.g.

```swift
let traceAutoPropagation: Bool
```
And set it up in the initialiser

Code after:
        
        var logger = logger
        logger[metadataKey: "request-id"] = .string(requestId)
        self._logger = .init(logger)
        self._serviceContext = .init(.topLevel)
        
        let storageBox = RequestBox(
            method: method,
            url: url,
            version: version,
            headers: headers,
            isKeepAlive: true,
            route: nil,
            parameters: .init(),
            byteBufferAllocator: byteBufferAllocator
        )
        self.requestBox = .init(storageBox)
        self.id = requestId
        self.application = application
        self.traceAutoPropagation = application.traceAutoPropagation
        
        self.remoteAddress = remoteAddress
        self.eventLoop = eventLoop
        self._storage = .init(.init())
        self.bodyStorage = .init(bodyStorage)
    }
    
    /// Automatically restores tracing serviceContext around the provided closure
    func propagateTracingIfEnabled<T>(_ closure: () throws -> T) rethrows -> T {
        if self.traceAutoPropagation {
            return try ServiceContext.withValue(self.serviceContext) {
                try closure()
            }
        } else {
            return try closure()
        }
    }
}
Repository: vapor/vapor
File: Sources/VaporTesting/withApp.swift (Swift)

Code before:
import Vapor

/// Perform a test while handling lifecycle of the application.
/// Feel free to create a custom function like this, tailored to your project.
///
/// Usage:
/// ```swift
/// @Test
/// func helloWorld() async throws {
///     try await withApp(configure: configure) { app in
///         try await app.testing().test(.GET, "hello", afterResponse: { res async in
///             #expect(res.status == .ok)
///             #expect(res.body.string == "Hello, world!")
///         })
///     }
/// }
/// ```
///
/// - Parameters:
///   - configure: The method where you should register services like routes, databases, providers, and more.
///   - test: The method where you can perform your tests with the configured application.
@discardableResult public func withApp<T>(
    configure: ((Application) async throws -> Void)? = nil,
    _ test: (Application) async throws -> T
) async throws -> T {
    let app = try await Application.make(.testing)
    let result: T
    do {
        if let configure {
            try await configure(app)
        }
        result = try await test(app)
    } catch {
        try? await app.asyncShutdown()
        throw error
    }
    try await app.asyncShutdown()
    return result
}


Review comment (suggestion):
```suggestion
@discardableResult
public func withApp<T>(
```

Code after:
import Vapor

/// Perform a test while handling lifecycle of the application.
/// Feel free to create a custom function like this, tailored to your project.
///
/// Usage:
/// ```swift
/// @Test
/// func helloWorld() async throws {
///     try await withApp(configure: configure) { app in
///         try await app.testing().test(.GET, "hello", afterResponse: { res async in
///             #expect(res.status == .ok)
///             #expect(res.body.string == "Hello, world!")
///         })
///     }
/// }
/// ```
///
/// - Parameters:
///   - configure: A closure where you can register routes, databases, providers, and more.
///   - test: A closure which performs your actual test with the configured application.
@discardableResult
public func withApp<T>(
    configure: ((Application) async throws -> Void)? = nil,
    _ test: (Application) async throws -> T
) async throws -> T {
    let app = try await Application.make(.testing)
    let result: T
    do {
        try await configure?(app)
        result = try await test(app)
    } catch {
        try? await app.asyncShutdown()
        throw error
    }
    try await app.asyncShutdown()
    return result
}


/// Perform a test while handling lifecycle of the application.
/// Feel free to create a custom function like this, tailored to your project.
///
/// Usage:
/// ```swift
/// @Test
/// func helloWorld() async throws {
Repository: vapor/vapor
File: Sources/VaporTesting/withApp.swift (Swift)

Code before:
/// Usage:
/// ```swift
/// @Test
/// func helloWorld() async throws {
///     try await withApp(configure: configure) { app in
///         try await app.testing().test(.GET, "hello", afterResponse: { res async in
///             #expect(res.status == .ok)
///             #expect(res.body.string == "Hello, world!")
///         })
///     }
/// }
/// ```
///
/// - Parameters:
///   - configure: The method where you should register services like routes, databases, providers, and more.
///   - test: The method where you can perform your tests with the configured application.
@discardableResult public func withApp<T>(
    configure: ((Application) async throws -> Void)? = nil,
    _ test: (Application) async throws -> T
) async throws -> T {
    let app = try await Application.make(.testing)
    let result: T
    do {
        if let configure {
            try await configure(app)
        }
        result = try await test(app)
    } catch {
        try? await app.asyncShutdown()
        throw error
    }
    try await app.asyncShutdown()
    return result
}


Review comment (suggestion):
```suggestion
        try await configure?(app)
```

Code after:
/// Usage:
/// ```swift
/// @Test
/// func helloWorld() async throws {
///     try await withApp(configure: configure) { app in
///         try await app.testing().test(.GET, "hello", afterResponse: { res async in
///             #expect(res.status == .ok)
///             #expect(res.body.string == "Hello, world!")
///         })
///     }
/// }
/// ```
///
/// - Parameters:
///   - configure: A closure where you can register routes, databases, providers, and more.
///   - test: A closure which performs your actual test with the configured application.
@discardableResult
public func withApp<T>(
    configure: ((Application) async throws -> Void)? = nil,
    _ test: (Application) async throws -> T
) async throws -> T {
    let app = try await Application.make(.testing)
    let result: T
    do {
        try await configure?(app)
        result = try await test(app)
    } catch {
        try? await app.asyncShutdown()
        throw error
    }
    try await app.asyncShutdown()
    return result
}


/// Perform a test while handling lifecycle of the application.
/// Feel free to create a custom function like this, tailored to your project.
///
/// Usage:
/// ```swift
/// @Test
/// func helloWorld() async throws {
///     try await withApp { app in
///         try await app.testing().test(.GET, "hello", afterResponse: { res async in
///             #expect(res.status == .ok)
///             #expect(res.body.string == "Hello, world!")
///         })
///     }
/// }
/// ```
@discardableResult
Repository: vapor/vapor
File: Sources/VaporTesting/withApp.swift (Swift)

Code before:
import Vapor

/// Perform a test while handling lifecycle of the application.
/// Feel free to create a custom function like this, tailored to your project.
///
/// Usage:
/// ```swift
/// @Test
/// func helloWorld() async throws {
///     try await withApp(configure: configure) { app in
///         try await app.testing().test(.GET, "hello", afterResponse: { res async in
///             #expect(res.status == .ok)
///             #expect(res.body.string == "Hello, world!")
///         })
///     }
/// }
/// ```
///
/// - Parameters:
///   - configure: The method where you should register services like routes, databases, providers, and more.
///   - test: The method where you can perform your tests with the configured application.
@discardableResult public func withApp<T>(
    configure: ((Application) async throws -> Void)? = nil,
    _ test: (Application) async throws -> T
) async throws -> T {
    let app = try await Application.make(.testing)
    let result: T
    do {
        if let configure {
            try await configure(app)
        }
        result = try await test(app)
    } catch {
        try? await app.asyncShutdown()
        throw error
    }
    try await app.asyncShutdown()
    return result
}


Review comment (suggestion):
```suggestion
///   - configure: A closure where you can register routes, databases, providers, and more.
```

Code after:
import Vapor

/// Perform a test while handling lifecycle of the application.
/// Feel free to create a custom function like this, tailored to your project.
///
/// Usage:
/// ```swift
/// @Test
/// func helloWorld() async throws {
///     try await withApp(configure: configure) { app in
///         try await app.testing().test(.GET, "hello", afterResponse: { res async in
///             #expect(res.status == .ok)
///             #expect(res.body.string == "Hello, world!")
///         })
///     }
/// }
/// ```
///
/// - Parameters:
///   - configure: A closure where you can register routes, databases, providers, and more.
///   - test: A closure which performs your actual test with the configured application.
@discardableResult
public func withApp<T>(
    configure: ((Application) async throws -> Void)? = nil,
    _ test: (Application) async throws -> T
) async throws -> T {
    let app = try await Application.make(.testing)
    let result: T
    do {
        try await configure?(app)
        result = try await test(app)
    } catch {
        try? await app.asyncShutdown()
        throw error
    }
    try await app.asyncShutdown()
    return result
}


/// Perform a test while handling lifecycle of the application.
/// Feel free to create a custom function like this, tailored to your project.
///
/// Usage:
/// ```swift