Add option to remove throws keyword from RouterMethods handlers

[Cyberbeni]

Reading this thread reminded me of an occasion when I accidentally left a top level throw inside a get handler in one of my projects. I would prefer if this wasn't allowed, so I am forced to make a conscious decision whether it's a 400 or a 500 error (or a rare case when it's something else) instead of automatically converting the error to a 500(?) response for me.

The actual implementation would probably be #if TRAIT around typalias any Error/Never and then using typed throws with that type.

Hopefully the final release of 6.3 keeps the fix, so non-default traits become easily usable for everyone: #769 (comment)

[adam-fowler]

I wonder if you could do something clever with typed throws and still be non breaking.

[Cyberbeni]

I wonder if you could do something clever with typed throws and still be non breaking.

Yes, I'll submit a PR after work.

Hopefully the final release of 6.3 keeps the fix, so non-default traits become easily usable for everyone

Realized that the 6.3 fix probably isn't needed in case the trait is turned off by default. (Since we already have traits, hiding existing behavior behind a default on trait would be a breaking change, so the current behavior has to be the turned off trait state)

[adam-fowler]

What do we need the trait for? I think we could probably do this and not introduce any breaking changes.

[Cyberbeni]

    #if NonThrowingRouteHandlers
    public typealias RouteHandlerError = Never
    #else
    public typealias RouteHandlerError = any Error
    #endif

[adam-fowler]

You can't use traits to change behaviour. They can only be additive. It doesn't compose well.

Consider the situation where your package is reliant on hummingbird and another package also reliant on hummingbird. Your package enables the NonThrowingHandlers trait but the other library is dependent on having throwing handlers. Which version of hummingbird should the application link?

[Cyberbeni]

If we want the trait to be additive then we need to invert the logic, moving from throws(Never) to throws(any Error) input should be backwards compatible. But that requires the trait to be included in the defaults, which would be a breaking change, since we already have a trait, so people could already be defining the exact traits they want to use, which wouldn't include this new trait. (SPM doesn't allow setting traits for a package that doesn't have traits, so the developers of that package can hide some API behind "default on" traits without that being a breaking change one time, when they first add traits)

If a library author wants to make sure that their code works well with throws(Never) then they can always add a "Testing" trait that controls whether they require the NonThrowingRouteHandlers from Hummingbird or not and run their tests with swift test --traits Testing.

[Cyberbeni]

I'm open to other solutions based on something like this:

diff --git a/Sources/Hummingbird/Router/RouterMethods.swift b/Sources/Hummingbird/Router/RouterMethods.swift
index 679b5cf..f9321b5 100644
--- a/Sources/Hummingbird/Router/RouterMethods.swift
+++ b/Sources/Hummingbird/Router/RouterMethods.swift
@@ -13,6 +13,7 @@ public import HummingbirdCore
 @preconcurrency
 public protocol RouterMethods<Context>: _HB_SendableMetatype {
     associatedtype Context: RequestContext
+    associatedtype RouteHandlerError: Error
 
     /// Add responder to call when path and method are matched
     ///
@@ -35,11 +36,7 @@ public protocol RouterMethods<Context>: _HB_SendableMetatype {
 }
 
 extension RouterMethods {
-    #if NonThrowingRouteHandlers
-    public typealias RouteHandlerError = Never
-    #else
     public typealias RouteHandlerError = any Error
-    #endif
 
     /// Add path for async closure
     @discardableResult public func on(

But I think adding a second generic parameter to Router/etc. would be a breaking change, and you would need to pay attention at every Router/etc. initialization, so that you use the non-throwing one, instead of declaring it once.

[adam-fowler]

A second generic parameter to Router could be problematic in that everything would have to throw the typed error (including Middleware). Also the missing route is kind of a handler and that requires the ability to throw an HTTPError.

You could possibly have.

router.get("/", error: Never.self) { request,context in
    // compilation error as it throws an error
    try someFunction()
}

router.get("/", use: myHandler)
func myHandler(request: Request, context: some RequestContext) async throws(Never) -> Response {
}

Of course you'd have to remember to add the error type.

[Cyberbeni]

Marking each get/etc. call already works:

get("/") { request, context throws(Never) in
    // compilation error as it throws an error
    try someFunction()
}

I guess this most likely has to wait for a new major release and then we invert the trait and make it default off.

[adam-fowler]

Not a great fan of the trait regardless of whether it disables/enables throwing. It feels like overkill, but at this point I don't have any obvious alternatives

[Cyberbeni]

If this needs a major version bump then a second associatedtype for RouterMethods is also an option with the default being Never in Router/etc., so even if a dependency uses the throwing version, the non-throwing version is still the default for you.

[Cyberbeni]

Or we could make it throws(HTTPResponseError), which would avoid the main issue (throwing an error that is not consciously mapped to a specific error code) the simplest way. (In the next major version OFC)

edit:

Currently this is not allowed but either with a future Swift version or a type-erased type.

Thrown type 'any HTTPResponseError' does not conform to the 'Error' protocol