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

Code before:
    @available(*, deprecated, renamed: "NIOLocks")
    public final class Locks {
        public let main: Lock
        var storage: [ObjectIdentifier: Lock]

        init() {
            self.main = .init()
            self.storage = [:]
        }

        public func lock<Key>(for key: Key.Type) -> Lock
            where Key: LockKey
        {
            self.main.lock()
            defer { self.main.unlock() }
            if let existing = self.storage[ObjectIdentifier(Key.self)] {
                return existing
            } else {
                let new = Lock()
                self.storage[ObjectIdentifier(Key.self)] = new
                return new
            }
        }
    }
    
    public final class NIOLocks {
        public let main: NIOLock
        var storage: [ObjectIdentifier: NIOLock]
        
        init() {
            self.main = .init()
            self.storage = [:]
        }
        
        public func lock<Key>(for key: Key.Type) -> NIOLock
        where Key: LockKey
        {
            self.main.lock()
            defer { self.main.unlock() }
            if let existing = self.storage[ObjectIdentifier(Key.self)] {
                return existing
            } else {
                let new = NIOLock()
                self.storage[ObjectIdentifier(Key.self)] = new
                return new
            }
        }
    }

    @available(*, deprecated, renamed: "nioLocks")
    public var locks = Locks()

Review comment (suggestion):
Since this is `Application.Lock` I don't think we actually need to rename any of this

Code after:
    public final class Locks {
        public let main: NIOLock
        var storage: [ObjectIdentifier: NIOLock]

        init() {
            self.main = .init()
            self.storage = [:]
        }

        public func lock<Key>(for key: Key.Type) -> NIOLock
            where Key: LockKey
        {
            self.main.lock()
            defer { self.main.unlock() }
            if let existing = self.storage[ObjectIdentifier(Key.self)] {
                return existing
            } else {
                let new = NIOLock()
                self.storage[ObjectIdentifier(Key.self)] = new
                return new
            }
        }
    }

    public var locks: Locks

    public var sync: NIOLock {
        self.locks.main
    }
    
    public enum EventLoopGroupProvider {
        case shared(EventLoopGroup)
        case createNew
    }

    public init(
        _ environment: Environment = .development,
        _ eventLoopGroupProvider: EventLoopGroupProvider = .createNew
    ) {
        Backtrace.install()
        self.environment = environment
        self.eventLoopGroupProvider = eventLoopGroupProvider
        switch eventLoopGroupProvider {
        case .shared(let group):
            self.eventLoopGroup = group
        case .createNew:
            self.eventLoopGroup = MultiThreadedEventLoopGroup(numberOfThreads: System.coreCount)
        }
        self.locks = .init()
        self.didShutdown = false
        self.logger = .init(label: "codes.vapor.application")
Repository: vapor/vapor
File: Sources/Vapor/Services/Service.swift (Swift)

Code before:
        struct Key: StorageKey {
            typealias Value = Storage
        }

        public var service: ServiceType {
            guard let makeService = self.storage.makeService else {
                fatalError("No service configured for \(ServiceType.self)")
            }
            return makeService(self.application)
        }

        public func use(_ provider: Provider) {
            provider.run(self.application)
        }

        public func use(_ makeService: @escaping (Application) -> ServiceType) {
            self.storage.makeService = makeService
        }

        func initialize() {
            self.application.storage[Key.self] = .init()
        }

        private var storage: Storage {
            if self.application.storage[Key.self] == nil {
                self.initialize()
            }
            return self.application.storage[Key.self]!
        }
    }
}


Review comment (suggestion):
I know this works fine, but can we make `initialize()` return the newly created entity? Then return that here.

Code after:
                self.makeService = .init(nil)
            }
        }

        struct Key: StorageKey {
            typealias Value = Storage
        }

        public var service: ServiceType {
            guard let makeService = self.storage.makeService.withLockedValue({ $0 }) else {
                fatalError("No service configured for \(ServiceType.self)")
            }
            return makeService(self.application)
        }

        public func use(_ provider: Provider) {
            provider.run(self.application)
        }

        public func use(_ makeService: @escaping @Sendable (Application) -> ServiceType) {
            self.storage.makeService.withLockedValue { $0 = makeService }
        }

        func initialize() -> Storage {
            let new = Storage()
            self.application.storage[Key.self] = new
            return new
        }

        private var storage: Storage {
            if let storage = application.storage[Key.self] {
                return storage
            } else {
                return self.initialize()
            }
        }
    }
}
Repository: vapor/vapor
File: Sources/Vapor/Services/Service.swift (Swift)

Code before:
public extension Application {
    struct Service<ServiceType> {

        let application: Application

        public init(application: Application) {
            self.application = application
        }

        public struct Provider {
            let run: (Application) -> ()

            public init(_ run: @escaping (Application) -> ()) {
                self.run = run
            }
        }

        final class Storage {
            var makeService: ((Application) -> ServiceType)?
            init() { }
        }

        struct Key: StorageKey {
            typealias Value = Storage
        }

        public var service: ServiceType {

Review comment (suggestion):
`public struct` instead of `public extension`? I tend to be explicit in marking things public in libraries/frameworks, so future changes don't accidentally expose internal API.

Code after:
import NIOConcurrencyHelpers

extension Application {
    public struct Service<ServiceType> {

        let application: Application

        public init(application: Application) {
            self.application = application
        }

        public struct Provider {
            let run: (Application) -> ()

            public init(_ run: @escaping @Sendable (Application) -> ()) {
                self.run = run
            }
        }

        final class Storage: Sendable {
            let makeService: NIOLockedValueBox<(@Sendable (Application) -> ServiceType)?>
            init() {
                self.makeService = .init(nil)
            }
        }

        struct Key: StorageKey {
Repository: vapor/vapor
File: Tests/VaporTests/ClientTests.swift (Swift)

Code before:
import Vapor
import XCTest

final class ClientTests: XCTestCase {
    func testClientConfigurationChange() throws {
        let app = Application(.testing)
        defer { app.shutdown() }

        app.http.client.configuration.redirectConfiguration = .disallow

        app.get("redirect") {
            $0.redirectTo("foo")
        }

        try app.server.start(address: .hostname("localhost", port: 8080))
        defer { app.server.shutdown() }

        let res = try app.client.get("http://localhost:8080/redirect").wait()

        XCTAssertEqual(res.status, .seeOther)
    }
    
    func testClientConfigurationCantBeChangedAfterClientHasBeenUsed() throws {
        let app = Application(.testing)
        defer { app.shutdown() }

        app.http.client.configuration.redirectConfiguration = .disallow

        app.get("redirect") {
            $0.redirectTo("foo")
        }

        try app.server.start(address: .hostname("localhost", port: 8080))
        defer { app.server.shutdown() }

        _ = try app.client.get("http://localhost:8080/redirect").wait()
        
        app.http.client.configuration.redirectConfiguration = .follow(max: 1, allowCycles: false)
        let res = try app.client.get("http://localhost:8080/redirect").wait()
        XCTAssertEqual(res.status, .seeOther)
    }

Review comment (suggestion):
We can't just change the tests to use the new method. We should be testing the 'old' method in addition to the new one.

Code after:
import XCTest
import Vapor
import NIOCore
import Logging
import AsyncHTTPClient
import NIOEmbedded

final class ClientTests: XCTestCase {
    func testClientConfigurationChange() throws {
        let app = Application(.testing)
        defer { app.shutdown() }

        app.http.client.configuration.redirectConfiguration = .disallow

        app.get("redirect") {
            $0.redirect(to: "foo")
        }

        try app.server.start(address: .hostname("localhost", port: 8080))
        defer { app.server.shutdown() }

        let res = try app.client.get("http://localhost:8080/redirect").wait()

        XCTAssertEqual(res.status, .seeOther)
    }
    
    func testClientConfigurationCantBeChangedAfterClientHasBeenUsed() throws {
        let app = Application(.testing)
        defer { app.shutdown() }

        app.http.client.configuration.redirectConfiguration = .disallow

        app.get("redirect") {
            $0.redirect(to: "foo")
        }

        try app.server.start(address: .hostname("localhost", port: 8080))
        defer { app.server.shutdown() }

        _ = try app.client.get("http://localhost:8080/redirect").wait()
Repository: vapor/vapor
File: Sources/Vapor/Middleware/FileMiddleware.swift (Swift)

Code before:
import Foundation
import NIOCore
import CryptoKit

/// Serves static files from a public directory.
///
/// `FileMiddleware` will default to `DirectoryConfig`'s working directory with `"/Public"` appended.
public final class FileMiddleware: Middleware {
    /// The public directory. Guaranteed to end with a slash.
    private let publicDirectory: String
    private let defaultFile: String?
    private let directoryAction: DirectoryAction
    private let advancedETagComparison: Bool

    public struct BundleSetupError: Equatable, Error {
        
        /// The description of this error.
        let description: String
        
        /// Cannot generate Bundle Resource URL
        public static let bundleResourceURLIsNil: Self = .init(description: "Cannot generate Bundle Resource URL: Bundle Resource URL is nil")
        
        /// Cannot find any actual folder for the given Public Directory
        public static let publicDirectoryIsNotAFolder: Self = .init(description: "Cannot find any actual folder for the given Public Directory")
    }

    public struct ETagHashes: StorageKey {
        public typealias Value = [String: FileHash]

Review comment (suggestion):
```suggestion
import Crypto
```

This will clear up the CI errors on Linux.

Code after:
import Foundation
import NIOCore

/// Serves static files from a public directory.
///
/// `FileMiddleware` will default to `DirectoryConfig`'s working directory with `"/Public"` appended.
public final class FileMiddleware: Middleware {
    /// The public directory. Guaranteed to end with a slash.
    private let publicDirectory: String
    private let defaultFile: String?
    private let directoryAction: DirectoryAction
    private let advancedETagComparison: Bool

    public struct BundleSetupError: Equatable, Error {
        
        /// The description of this error.
        let description: String
        
        /// Cannot generate Bundle Resource URL
        public static let bundleResourceURLIsNil: Self = .init(description: "Cannot generate Bundle Resource URL: Bundle Resource URL is nil")
        
        /// Cannot find any actual folder for the given Public Directory
        public static let publicDirectoryIsNotAFolder: Self = .init(description: "Cannot find any actual folder for the given Public Directory")
    }

    struct ETagHashes: StorageKey {
        public typealias Value = [String: FileHash]
Repository: vapor/vapor
File: Sources/Vapor/Response/Response+Body.swift (Swift)

Code before:
        }
    }
}

private final class ResponseBodyCollector: BodyStreamWriter {
    let buffer: NIOLockedValueBox<ByteBuffer>
    let eventLoop: EventLoop
    let promise: EventLoopPromise<ByteBuffer>

    init(eventLoop: EventLoop, byteBufferAllocator: ByteBufferAllocator) {
        self.buffer = .init(byteBufferAllocator.buffer(capacity: 0))
        self.eventLoop = eventLoop
        self.promise = eventLoop.makePromise(of: ByteBuffer.self)
    }

    func write(_ result: BodyStreamResult, promise: EventLoopPromise<Void>?) {
        switch result {
        case .buffer(var buffer):
            _ = self.buffer.withLockedValue { $0.writeBuffer(&buffer) }
        case .error(let error):
            self.promise.fail(error)
        case .end:
            self.promise.succeed(self.buffer.withLockedValue { $0 })
        }
        promise?.succeed(())
    }
}


Review comment (bug):
Borrowed slightly from my much more heavily revised version:
```swift
    func write(_ result: BodyStreamResult, promise: EventLoopPromise<Void>?) {
        let future = self.eventLoop.submit {
            switch result {
            case .buffer(var buffer):
                self.buffer.writeBuffer(&buffer)
            case .error(let error):
                self.promise.fail(error)
                throw error
            case .end:
                self.promise.succeed(self.buffer)
            }
        }
        // N.B.: Preexisting semantic bug: Errors in the stream should fail the individual write promise.
        if let promise { future.cascade(to: promise) }
    }
```

Code after:
        }
    }
}

// Since all buffer mutation is done on the event loop, we can be unchecked here.
// This removes the need for a lock and performance hits from that
// Any changes to this type need to be carefully considered
private final class ResponseBodyCollector: BodyStreamWriter, @unchecked Sendable {
    var buffer: ByteBuffer
    let eventLoop: EventLoop
    let promise: EventLoopPromise<ByteBuffer>

    init(eventLoop: EventLoop, byteBufferAllocator: ByteBufferAllocator) {
        self.buffer = byteBufferAllocator.buffer(capacity: 0)
        self.eventLoop = eventLoop
        self.promise = eventLoop.makePromise(of: ByteBuffer.self)
    }

    func write(_ result: BodyStreamResult, promise: EventLoopPromise<Void>?) {
        let future = self.eventLoop.submit {
            switch result {
            case .buffer(var buffer):
                self.buffer.writeBuffer(&buffer)
            case .error(let error):
                self.promise.fail(error)
                throw error
            case .end:
                self.promise.succeed(self.buffer)
            }
        }
        // Fixes an issue where errors in the stream should fail the individual write promise.
        if let promise { future.cascade(to: promise) }
    }
}
Repository: vapor/vapor
File: Sources/Development/routes.swift (Swift)

Code before:
import class Foundation.Bundle
@preconcurrency import Vapor
import NIOCore
import NIOHTTP1
import NIOConcurrencyHelpers

struct Creds: Content {
    var email: String
    var password: String
}

public func routes(_ app: Application) throws {
    app.on(.GET, "ping") { req -> StaticString in
        return "123" as StaticString
    }


    // ( echo -e 'POST /slow-stream HTTP/1.1\r\nContent-Length: 1000000000\r\n\r\n'; dd if=/dev/zero; ) | nc localhost 8080
    app.on(.POST, "slow-stream", body: .stream) { req -> EventLoopFuture<String> in
        let done = req.eventLoop.makePromise(of: String.self)

        let totalBox = NIOLoopBoundBox(0, eventLoop: req.eventLoop)
        req.body.drain { result in
            let promise = req.eventLoop.makePromise(of: Void.self)

            switch result {
            case .buffer(let buffer):

Review comment (suggestion):
I think we should keep this so we know when the Sendable warning is gone. This will also mask anything we've missed in the future etc

Code after:
import class Foundation.Bundle
import Vapor
import NIOCore
import NIOHTTP1
import NIOConcurrencyHelpers

struct Creds: Content {
    var email: String
    var password: String
}

public func routes(_ app: Application) throws {
    app.on(.GET, "ping") { req -> StaticString in
        return "123" as StaticString
    }


    // ( echo -e 'POST /slow-stream HTTP/1.1\r\nContent-Length: 1000000000\r\n\r\n'; dd if=/dev/zero; ) | nc localhost 8080
    app.on(.POST, "slow-stream", body: .stream) { req -> EventLoopFuture<String> in
        let done = req.eventLoop.makePromise(of: String.self)

        let totalBox = NIOLoopBoundBox(0, eventLoop: req.eventLoop)
        req.body.drain { result in
            let promise = req.eventLoop.makePromise(of: Void.self)

            switch result {
            case .buffer(let buffer):
Repository: vapor/vapor
File: Sources/Vapor/Authentication/AuthenticationCache.swift (Swift)

Code before:

    private struct CacheKey: StorageKey {
        typealias Value = Cache
    }

    private var cache: Cache {
        get {
            if let existing = self.request.storage[CacheKey.self] {
                return existing
            } else {
                let new = Cache()
                self.request.storage[CacheKey.self] = new
                return new
            }
        }
        set {
            self.request.storage[CacheKey.self] = newValue
        }
    }
}

// This is to get around the fact that for legacy reasons we can't enforce Sendability on Authenticatable
// types (e.g. Fluent 4 models can never be Sendable because they're reference types with mutable values
// required by protocols and property wrappers). This allows us to store the Authenticatable type in a
// safe-most-of-the-time way. This does introduce an edge case where type could be stored and mutated in
// multiple places. But given how Vapor and its users use Authentication this should almost always never
// occur and it was decided the trade-off was acceptable
// As the name implies, the usage of this is unsafe because it disables the sendable checking of the
// compiler and does not add any synchronisation.
@usableFromInline
internal struct UnsafeAuthenticationBox<A>: @unchecked Sendable {
    @usableFromInline
    let authenticated: A
    
    @inlinable
    init(_ authenticated: A) {
        self.authenticated = authenticated
    }
}


Review comment (security):
```suggestion
// multiple places. But given how Vapor and its users use Authentication this should almost never
```

Code after:

    private struct CacheKey: StorageKey {
        typealias Value = Cache
    }

    private var cache: Cache {
        get {
            if let existing = self.request.storage[CacheKey.self] {
                return existing
            } else {
                let new = Cache()
                self.request.storage[CacheKey.self] = new
                return new
            }
        }
        set {
            self.request.storage[CacheKey.self] = newValue
        }
    }
}

// This is to get around the fact that for legacy reasons we can't enforce Sendability on Authenticatable
// types (e.g. Fluent 4 models can never be Sendable because they're reference types with mutable values
// required by protocols and property wrappers). This allows us to store the Authenticatable type in a
// safe-most-of-the-time way. This does introduce an edge case where type could be stored and mutated in
// multiple places. But given how Vapor and its users use Authentication this should almost never
// occur and it was decided the trade-off was acceptable
// As the name implies, the usage of this is unsafe because it disables the sendable checking of the
// compiler and does not add any synchronisation.
@usableFromInline
internal struct UnsafeAuthenticationBox<A>: @unchecked Sendable {
    @usableFromInline
    let authenticated: A
    
    @inlinable
    init(_ authenticated: A) {
        self.authenticated = authenticated
    }
}
Repository: vapor/vapor
File: Tests/AsyncTests/AsyncCommandsTests.swift (Swift)

Code before:
import XCTest
import Vapor

final class AsyncCommandsTests: XCTestCase {
    func testAsyncCommands() throws {
        let app = Application(.testing)
        defer { app.shutdown() }

        struct FooCommand: AsyncCommand {
            struct Signature: CommandSignature {
                @Argument(name: "name")
                var name: String
            }

            let help = "Does the foo."

            func run(using context: CommandContext, signature: Signature) throws {
                context.console.output("Hello, \(signature.name)!")
            }
        }

        app.asyncCommands.use(FooCommand(), as: "foo")

        app.environment.arguments = ["vapor", "foo", "bar"]

        XCTAssertNoThrow(try app.start())
    }
}


Review comment (suggestion):
Shouldn't this be
```suggestion
        app.environment.arguments = ["vapor", "foo", "--name", "bar"]
```


Code after:
import XCTest
import Vapor

final class AsyncCommandsTests: XCTestCase {
    func testAsyncCommands() throws {
        let app = Application(.testing)
        defer { app.shutdown() }

        app.asyncCommands.use(FooCommand(), as: "foo")

        app.environment.arguments = ["vapor", "foo", "bar"]

        XCTAssertNoThrow(try app.start())

        XCTAssertTrue(app.storage[TestStorageKey.self] ?? false)
    }
}

extension AsyncCommandsTests {
    struct TestStorageKey: StorageKey {
        typealias Value = Bool
    }

    struct FooCommand: AsyncCommand {
        struct Signature: CommandSignature {
            @Argument(name: "name")
            var name: String
        }

        let help = "Does the foo."

        func run(using context: CommandContext, signature: Signature) throws {
            context.application.storage[TestStorageKey.self] = true
        }
    }
}
Repository: vapor/vapor
File: Sources/Development/entrypoint.swift (Swift)

Code before:
import Vapor
import Logging

@main
struct Entrypoint {
    static func main() async throws {
        var env = try Environment.detect()
        try LoggingSystem.bootstrap(from: &env)

        let app = try await Application.make(env)
        do {
            try configure(app)
            try await app.execute()
            try await app.asyncShutdown()
        } catch {
            try await app.asyncShutdown()
            throw error
        }
    }
}



Review comment (suggestion):
```suggestion
            try? await app.asyncShutdown()
```

Code after:
import Vapor
import Logging

@main
struct Entrypoint {
    static func main() async throws {
        var env = try Environment.detect()
        try LoggingSystem.bootstrap(from: &env)

        let app = try await Application.make(env)
        do {
            try configure(app)
            try await app.execute()
            try await app.asyncShutdown()
        } catch {
            try? await app.asyncShutdown()
            throw error
        }
    }
}