Skip to content

Conversation

the10thWiz
Copy link
Collaborator

@the10thWiz the10thWiz commented Jun 26, 2024

Addresses #749, adds support for typed catchers via the transient crate.

When a FromRequest, FromParam, FromQuery, or FromData implementation returns an Error or Forward, if the returned error implements Any<Co<'r>>, it is boxed and travels with the Outcome.

A catcher can have a third parameter, now (&E, Status, &Request), where E is some type that implements Any<Co<'r>>. If the error contained in the Outcome is of the same type, it is downcast to
the specific type, and passed to the catcher.

A working example can be seen in the error-handling example - simply visit /hello/name/a to see a ParseIntError returned by the FromParam of i8.

TODO

  • Better syntax for passing the error type to catchers. This is likely going to look like a new FromError trait, and a error = "<error>" parameter for the upcast error.
    • Requires new compile tests
  • Catchers should carry an Option<TypeId>, and use it to prevent false collisions.
    • Update matcher code to also check TypeId
  • Special TypedError trait
  • Responder should return an Outcome<..., Self::Error, ...>
  • Improve example error message
  • Any<Co<'r>> should be Any<Inv<'r>>. Transient actually already protects us from casting Any<Inv<'r>> to a different Any<Inv<'l>>, by telling the compiler that Inv<'a> is invariant wrt 'a.
  • Documentation needs to be written - this is a pretty big change, and somewhat difficult to use. There are a number of small gotchas - from types that don't implement Transient, to simply using the wrong error type in the catcher. I'm not sure what the right way to present this is.
  • We should consider logging or carrying the error type's name, to help users identify what the type is, and why their catcher doesn't catch it. (Also log if the type implements Transient, since many types don't).
    • Tests (likely using a tracing subscriber) should validate this (I'm not sure how, since type_name isn't a stable name - nevermind, it is, since the test is compiled with the same version).
  • Export a TypedError derive macro.
    • I'm current involved with @JRRudy1 in working on an improved derive macro, which would be safe to use.
    • Also derive a Transient implementation at the same time.
  • rocket::form::Errors (and it's parts) should implement Transient. (Now non-issue)
  • FromError trait
  • Fairings should be able to reject requests (and include a TypedError)
  • Clean up TODOs
  • Contrib
    • dyn_templates: The responder and fairing impls need to be updated.
    • (sync_)db_pools: The error types should implement TypedError.
    • ws: The error types should implement TypedError, and responder/fairing impls need to be updated.

@the10thWiz the10thWiz self-assigned this Jun 26, 2024
@the10thWiz the10thWiz modified the milestone: 0.6.0 Jun 26, 2024
@the10thWiz the10thWiz linked an issue Jun 26, 2024 that may be closed by this pull request
@the10thWiz the10thWiz requested a review from SergioBenitez June 26, 2024 03:07
Copy link
Member

@SergioBenitez SergioBenitez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so, so much for this. As you know, this is a feature that's been
requested for quite some time now, so it's exciting to see some real progress
here.

While reviewing this PR, I found myself consistently asking:

  • Does this overcome the fundamental technical hurdles that have plagued this
    getting this feature into Rocket?
  • Does this meet the usability requirements we expect?
  • Is this feature-complete enough to avoid missing expectation due to missing
    features?

I believe the answer to the first question is yes, which is truly exciting!
I temper this statement only because I also believe we're missing some key
usability-impacting features, and there's a concern that the approach won't
extend to these features, but at a first glance it seems probable.

Feature-Completeness

This PR current allows errors from input guards (FromParam, FromSegments,
FromForm, and FromData) to be type-erased and later downcast into a concrete
type in an error catcher. The critical missing piece is allowing the same for
responders. In particular, we want the following to compile and work as one
might expect:

#[derive(Debug, Transient /* Error */)]
struct MyType<'r>(&'r str);

#[get("/<foo>")]
fn hello(foo: &str) -> Result<&'static str, MyType<'_>> {
    Err(MyType(foo))
}

#[catch(default)]
fn error(foo: &MyType) -> &str {
    foo.0
}

This means that Responder needs to change to accommodate an error type.

type Outcome<'o> = Outcome<Response<'o>, E, Status>;

pub trait Responder<'r, 'o: 'r> {
    type Error: Debug + 'o;

    fn respond_to(self, request: &'r Request<'_>) -> Outcome<'o, Self::Error>;
}

Where:

  • Outcome::Success(Response) is akin to today's Ok(Response)
  • Outcome::Forward(Status) is akin to today's Err(Status) and can be read
    as "forwarding to the catcher for Status"
  • Outcome::Error(Self::Error) is new. For many types, Self:Error will be
    set to Infallible, implying that the responder only succeeds or forwards.

One thing to consider is that we probably want to associate a status with the
error, and we also want to allow the error to respond directly if nothing
catches it.

Here we have two obvious approaches:

  • Require Responder::Error: Responder, apply the static dispatch
    specialization trick to check for T: Transient as this PR does. Check that
    we don't run into an infinite error-loop (i.e, a responder that returns
    itself as an error which then errors with itself, and so on).

  • Create a new trait, Error, and apply the static dispatch specialization
    trick to check for T: Error. The trait might look something like:

    trait Error<'r>: Transient + .. {
        fn respond_to(&self, request: &'r Request<'_>) -> Result<Response<'r>, Status> {
            Err(self.status())
        }
    
        /// Maybe? Ignore for now.
        fn status(&self) -> Status { Status::InternalServerError }
    
        /// Log on catcher failure: expected type Foo, have error Bar.
        fn type_name(&self) -> &'static str { .. }
    
        fn upcast(self) -> Box<dyn Transient<'r>> { .. }
    
        fn source(&self) -> Option<Box<dyn Error<'r>>> { .. }
    }

It's not obvious to me at this point which approach might be better, or if
there's another approach that's missing here. The main difference is that the
former requires the error type to implement Responder, which may not be what
we want, and furthermore that the Responder implementation not vary between
being used as a regular response and as an error. As such, I lean towards the
latter approach.

Usability

My main concerns on the usability front are two-fold:

  • the extensions to the #[catch] macro. In particular, I'd like to find a
    way to remove the need for additional attribute arguments of status and
    perhaps even error.
  • the difficulty in implementing custom downcastable errors

#[catch]

In other attributes, like #[route], we need attribute parameters because
either they provide information about what to parse (path, segments, and query
params) or they operate in a fundamentally different manner at the type-system
level: (data and request). There's also a security and category-theoretical
argument to be made about creating different class types for parseable
parameters.

But with status and error, none of these argument need necessarily apply:
all of the information derives from references or copy-able types without and
particular security considerations. So the only reason* (there's one reason we
might want an error argument, actually) for the arguments to exist is for our
sake in implementing codegen.

More concretely, the #[catch] attribute separates types into three categories:

  • T: FromRequest
  • T: Transient via error =
  • Status via status =

I argue that in reality, these should all be one:

  • T: FromError

    trait FromError<'r> {
        fn from_error(request: &'r Request<'_>, error: Option<ErasedError>);
    }

Then we can recover all of the obvious implementations:

impl<'r> FromError<'r> for Status {
    fn from_error(request: &'r Request<'_>, error: Option<ErasedError>) {
        error.as_ref().map_or(Status::InternalServerError, |e| e.status())
    }
}

Implementing Custom Errors

Consider the following complete program:

use rocket::*;
use rocket::request::{self, Request, FromRequest};
use rocket::outcome::Outcome;
use rocket::http::Status;

#[derive(Debug, transient::Transient)]
struct MyError<'r>(&'r str);

#[derive(Debug)]
struct MyGuard;

#[rocket::async_trait]
impl<'r> FromRequest<'r> for MyGuard {
    type Error = MyError<'r>;

    async fn from_request(req: &'r Request<'_>) -> request::Outcome<Self, Self::Error> {
        Outcome::Error((Status::InternalServerError, MyError(req.uri().path().as_str())))
    }
}

#[get("/")]
fn index(guard: MyGuard) -> &'static str {
    "Hello, world!"
}

#[catch(default, error = "<foo>")]
fn error<'a>(foo: &MyError<'a>) -> &'a str {
    foo.0
}

#[launch]
fn rocket() -> _ {
    rocket::build()
        .mount("/", routes![index])
        .register("/", catchers![error])
}

There are a few issues here:

  • I needed to depend on transient directly or else the derive wouldn't work
    since it references transient::. This shouldn't need to be the case.
  • The error catcher fails at runtime because, for whatever reason, the error
    type MyError is not meeting the specialization requirements. I would
    expect that if my type doesn't meet the requirements to be an error, then
    I wouldn't be allowed to use it as typed error at all, and my program would
    fail to compile.
  • Covariance needs to be declared: can we figure it out automatically, i.e,
    via by calling rustc, or for references syntactically (&T)?

This also begs the question: what exactly is required to be implemented of error
types? Transient does not appear to be sufficient. This further supports the
idea of a Rocket-specific Error trait that has all of the requirements
built-in. And if Error is a Rocket trait, then we can re-export the derive
to allow users to do the following, completely relegating transient to an
implementation detail.

#[derive(rocket::Error)]
struct MyError<'r> { .. }

As an aside, the following gives an error message that we should look to improve:

#[derive(Debug, Responder, Transient)]
struct MyType<'r>(&'r str);

#[catch(default, error = "<foo>")]
fn error<'a>(uri: &Origin, foo: &'a MyType) -> &'a str {
    foo.0
}

@the10thWiz
Copy link
Collaborator Author

@SergioBenitez I've made some significant progress, and I've found a couple ways to improve the API. I wanted to put them in writing, both so you can comment on them sooner rather than later, and for future reference.

The 'final' API

I've opted to remove Status from anywhere a TypedError is returned, rather relying on the error to provide a status. To aid in this, there are TypedError implementations for Status, (Status, T) and Custom<T> (where T: TypedError), so the status can be customized without being forced to add a status member.

The final form of the TypedError trait:

// `AsAny` is a sealed/blanket impl for a `fn as_any(&self) -> &dyn Any`
pub trait TypedError<'r>: AsAny<Inv<'r>> + Send + Sync + 'r {
    /// Generates a default response for this type (or forwards to a default catcher)
    fn respond_to(&self, request: &'r Request<'_>) -> Result<Response<'r>, Status> {
        Err(self.status())
    }

    /// A descriptive name of this error type. Defaults to the type name.
    fn name(&self) -> &'static str { std::any::type_name::<Self>() }

    /// The error that caused this error. Defaults to None.
    ///
    /// # Warning
    /// A typed catcher will not attempt to follow the source of an error
    /// more than (TODO: exact number) 5 times.
    fn source(&'r self) -> Option<&'r (dyn TypedError<'r> + 'r)> { None }

    /// Status code
    fn status(&self) -> Status { Status::InternalServerError }
}

It should be fairly obvious what these methods get used for.

One change, is that .source() will be called multiple times (currently up to 5). I'm preventing loops by simply placing a reasonable upper limit on the number of times it can be called, since I don't think there is a better option. (I considered checking for pointer equality with a previous value, but I'm not confident we can rely on this, since ZSTs don't have a meaningful address, and false positives might be possible).

Remaining decisions

Is TypedError too onerous of a trait bound for error types? I don't think so. Pretty much every std error is already a typed error, and Status can be used if you don't want to deal with an actual type. The derive macro has very few requirements (we only support a single lifetime parameter), so any type you control can pretty trivially implement TypedError. The one exception would be third-party errors (such as sqlite::Error, which obviously doesn't implement TypedError), but we could provide some kind of Debug<E> impl, that enables any 'static to act as a TypedError. I also don't think it's a significant burden to ask rocket-specific crates to add TypedError to their error types, and wrap any error types they don't control.

With this in mind, if it isn't too much of a burden, we might be able to eliminate the specialization trick, since each of these error types would be required to implement TypedError. This would also allow Responder to return a Result<Response, Self::Error>, since it provides all the functionality we would require.

The main alternative would be to require some kind of HasStatus trait, with a single fn status(&self) -> Status method, and specialize on TypedError. However, I don't think there is much value in allowing types to implement HasStatus without TypedError - the only additional req is Transient, which is trivially derived with the TypedError derive.

I'm also considering removing respond_to from TypedError, since it's pretty unlikely it actually gets called. Any catcher (even an untyped one) prevents the respond_to method from getting called.

Potential issues

The respond_to method is tried after untyped catchers, so an untyped catcher can prevent respond_to from ever getting called. Because of this, a default catcher mounted at the root will prevent respond_to from ever getting called, for any error.

We likely need to provide error wrappers for the contrib db crates, since there really isn't a good way to implement TypedError for the DB error types.

@the10thWiz the10thWiz marked this pull request as ready for review November 22, 2024 05:31
@the10thWiz
Copy link
Collaborator Author

There are a few major updates with this. It's a partial rewrite, based on the latest version of main, along with a few other changes along the way.

First, the TypedError's source() method now takes a parameter, so that it can return multiple errors. The only use-case in current code is Either<A, B>, where the error type is (A, B).

Second, we now use a git dependency for transient, since there are a few changes that have not yet been merged into upstream. There are also some other issues I need to sort out with the CI for transient, but that shouldn't prevent reviewing this PR.

Third, TypedError is no longer optional, and the specialization trick is no longer used. We provide two primary options for implementing TypedError: the derive macro, which can be applied to almost any type, and Debug<E>, which provides a default implementation for any 'static type.

Overall, I'm happy with this PR. From updating the tests, and a personal project I'm working on at the same time, there are only a couple of pain points. Since I'm deep in this project, I can't say with any level of certainty how easy it is to identify what type you should use in the catcher. I hope it's fairly easy with the error_name parameter in the trace logs. Also, a number of examples in Rocket itself use Result as a responder, in a way that no longer works, since Result now converts the Err variant into a TypedError that can be caught by a catcher. I rewrote the examples to use Either, but it did make them more verbose.

@SergioBenitez Please review this when you get a chance.

@SergioBenitez
Copy link
Member

@the10thWiz I can actually review the PR this weekend! Looking forward to it.

@the10thWiz
Copy link
Collaborator Author

@SergioBenitez Awesome!

@SergioBenitez
Copy link
Member

@the10thWiz This needs to be rebased on master.

the10thWiz and others added 7 commits September 1, 2025 18:39
- Minor updates across the board
- Rewrite of part of the requests page to reflect the new api
So you don't need to manually select the link with your mouse to view
these examples.
From some basic testing, I cannot identify what advantage this provides.
I assume this was a workaround for a bug in rust_analyzer, but I cannot
identify what it might be.
Copy link
Member

@SergioBenitez SergioBenitez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks amazing. Seriously, incredible job @the10thWiz!

I'll need way more time than I budgeted this weekend to give this a proper review, but I wanted to leave a few comments first. At a high level, the typed catchers themselves look absolutely amazing. I think you've made really good decisions. I've left a few comments here and there, but nothing major. However, there's quite a bit of unsafe code being introduced without explanation, and that must be addressed before we can merge this. Better yet, whatever unsafe code can be removed should be.

The only interesting point related to design is the introduction of a new fairing class. I've left a comment on that and look forward to your response.

Finally, I think a bit of the code can be cleaned up, made pithier and more understandable, but for the most part, this is excellent. I'll have more on this when I can perform a thorough review.

Comment on lines -46 to -53
#[cfg(all(not(doc), rust_analyzer))]
#original

#[cfg(all(doc, not(rust_analyzer)))]
#doc
#original

#[cfg(not(any(doc, rust_analyzer)))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to keep this around to work-around some rust-analyzer stuff, by allowing users to configure rust analyzer to set the rust_analyzer cfg when it runs over the code.

Comment on lines 62 to 63
.map_err(|diag| diag.help("`#[catch]` expects a status code int or `default`: \
`#[catch(404)]` or `#[catch(default)]`"))?;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably isn't correct anymore since this can fail if error fails, I believe? Or does error never fail?

Comment on lines +65 to +101
let mut diags = Diagnostics::new();
let mut guards = Vec::new();
let mut error = None;
for (index, arg) in function.sig.inputs.iter().enumerate() {
if let Some((ident, ty)) = arg.typed() {
match meta.error.as_ref() {
Some(err) if Name::from(ident) == err.name => {
error = Some(Guard {
source: meta.error.clone().unwrap().value,
fn_ident: ident.clone(),
ty: ty.clone(),
});
}
_ => {
guards.push(Guard {
source: Dynamic { name: Name::from(ident), index, trailing: false },
fn_ident: ident.clone(),
ty: ty.clone(),
})
}
}
} else {
let span = arg.span();
let diag = if arg.wild().is_some() {
span.error("handler arguments must be named")
.help("to name an ignored handler argument, use `_name`")
} else {
span.error("handler arguments must be of the form `ident: Type`")
};

diags.push(diag);
}
}
if meta.error.is_some() != error.is_some() {
let span = meta.error.unwrap().span();
diags.push(span.error("Error parameter not found on function"));
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be cleaned up significantly using iterators. See how this is done in route/parse.rs.

@@ -3,6 +3,7 @@ use rocket::request::{self, FlashMessage, FromRequest, Request};
use rocket::response::{Redirect, Flash};
use rocket::http::{CookieJar, Status};
use rocket::form::Form;
use rocket::either::Either;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also import Left and Right to use them naturally.

@@ -1,6 +1,6 @@
use std::sync::atomic::{AtomicUsize, Ordering};

use rocket::State;
use rocket::{State, StateMissing};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably be named StateError or something? Looks to be what the stdlib prefers.

Comment on lines 73 to 75
Either::Right(
Template::render("index", Context::err(&conn, "Failed to toggle task.").await)
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Either::Right(
Template::render("index", Context::err(&conn, "Failed to toggle task.").await)
)
Right(Template::render("index", Context::err(&conn, "Failed to toggle task.").await))

@@ -25,7 +25,7 @@ pushd "${PROJECT_ROOT}" > /dev/null 2>&1
--crate-version ${DOC_VERSION} \
--enable-index-page \
--generate-link-to-definition" \
cargo doc -Zrustdoc-map --no-deps --all-features \
cargo +nightly doc -Zrustdoc-map --no-deps --all-features \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this actually necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The -Z flags are exclusive to Rust nightly. You might have set up a directory override on your machine (and iirc, the workflow also changes the default for the directory).

I think it's marginally more convenient for running it locally.

Comment on lines 73 to 75
fn respond_to(self, __req: &'r #Request<'_>)
-> #_response::Result<'r, 'o>
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fn respond_to(self, __req: &'r #Request<'_>)
-> #_response::Result<'r, 'o>
{
fn respond_to(self, __req: &'r #Request<'_>) -> #_response::Result<'r, 'o> {

@@ -354,6 +393,7 @@ impl fmt::Debug for Catcher {
.field("base", &self.base)
.field("code", &self.code)
.field("rank", &self.rank)
.field("type_id", &self.type_id.as_ref().map(|_| "TY"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not the following?

Suggested change
.field("type_id", &self.type_id.as_ref().map(|_| "TY"))
.field("type_id", &self.type_id)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The TypeId doesn't actually provide much (if any) useful information, since it looks something like TypeId(0x474ccf3b5db264ef53916706f7d7bb2c). I've updated this to actually print the type name (and, since I have the actual type name, it's now printed as part of the catcher on launch).

Comment on lines +162 to +167
/// A request callback, represented by the [`Fairing::on_request_filter()`] method,
/// is called just after a request is received, immediately after
/// pre-processing the request and running all `Request` fairings. This method
/// returns a `Result`, which can be used to terminate processing of a request,
/// bypassing the routing process. The error value must be a [`TypedError`], which
/// can then be caught by a typed catcher.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is very interesting. I apologize if I missed it, but what's the rationale for this approach as opposed to changing request fairings to optionally return an error and abort the request? It seems desirable to be able to abort a request before other request fairings are run, for example, or to be able to choose the order based on how you attach fairings.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I posted this earlier, but there is a safety issue at play here. Since the returned error can reference values in the Request, we need them to be immutable borrows (and afaik, there isn't a way to force a borrow to be immutable, beyond only providing an immutable reference in the first place). Basically all the catching logic relies on being able to read values from the request, so passing a mutable borrow to a filter fairing is a non-starter. The simple solution was to create a new Fairing type with the signature we need.

That being said, the decision to run filter fairings after normal request fairings was arbitrary. I'm not sure what the ideal solution is here, but I'd say an initial solution could simply be to run filter fairing first.

- import Left & Right in examples
- Consolidate function to single line
StaticInfo and Catcher now retain the error type's name, and it's
printed witht the mounting information on startup.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typed Error Catchers, Fairings
3 participants