ronantakizawa/codereview-bench huggingface dataset:
Repository: tokio-rs/axum
File: axum-extra/src/response/erased_json.rs (Rust)

Code before:
use std::sync::Arc;

use axum::{
    http::{header, HeaderValue, StatusCode},
    response::{IntoResponse, Response},
};
use bytes::{BufMut, Bytes, BytesMut};
use serde::Serialize;

/// A response type that holds a JSON in serialized form.
///
/// This allows returning a borrowing type from a handler, or returning different response
/// types as JSON from different branches inside a handler.
///
/// Like [`axum::Json`],
/// if the [`Serialize`] implementation decides to fail
/// or if a map with non-string keys is used,
/// a 500 response will be issued
/// whose body is the error message in UTF-8.
///
/// This can be constructed using [`new`](ErasedJson::new)
/// or the [`json!`](crate::json) macro.
///
/// # Example
///
/// ```rust
/// # use axum::{response::IntoResponse};
/// # use axum_extra::response::ErasedJson;
/// async fn handler() -> ErasedJson {
///     # let condition = true;
///     # let foo = ();
///     # let bar = vec![()];
///     // ...
///
///     if condition {
///         ErasedJson::new(&foo)
///     } else {
///         ErasedJson::new(&bar)
///     }
/// }
/// ```

Review comment (bug):
```suggestion
/// if the [`Serialize`] implementation fails
```

Code after:
use std::sync::Arc;

use axum::{
    http::{header, HeaderValue, StatusCode},
    response::{IntoResponse, Response},
};
use bytes::{BufMut, Bytes, BytesMut};
use serde::Serialize;

/// A response type that holds a JSON in serialized form.
///
/// This allows returning a borrowing type from a handler, or returning different response
/// types as JSON from different branches inside a handler.
///
/// Like [`axum::Json`],
/// if the [`Serialize`] implementation fails
/// or if a map with non-string keys is used,
/// a 500 response will be issued
/// whose body is the error message in UTF-8.
///
/// This can be constructed using [`new`](ErasedJson::new)
/// or the [`json!`](crate::json) macro.
///
/// # Example
///
/// ```rust
/// # use axum::{response::IntoResponse};
/// # use axum_extra::response::ErasedJson;
/// async fn handler() -> ErasedJson {
///     # let condition = true;
///     # let foo = ();
///     # let bar = vec![()];
///     // ...
///
///     if condition {
///         ErasedJson::new(&foo)
///     } else {
///         ErasedJson::new(&bar)
///     }
/// }
/// ```
Repository: tokio-rs/axum
File: axum-extra/src/response/error_response.rs (Rust)

Code before:
use axum_core::response::{IntoResponse, Response};
use http::StatusCode;
use std::error::Error;
use std::io::Write;
use tracing::error;

/// Convenience response to create an error response from a non-IntoResponse error
///
/// This provides a method to quickly respond with an error that does not implement
/// the IntoResponse trait itself. This type should only be used for debugging purposes or internal
/// facing applications, as it includes the full error chain with descriptions,
/// thus leaking information that could possibly be sensitive.
///
/// ```rust
/// use axum_extra::response::InternalServerError;
/// use axum_core::response::IntoResponse;
/// # use std::io::{Error, ErrorKind};
/// # fn try_thing() -> Result<(), Error> {
/// #   Err(Error::new(ErrorKind::Other, "error"))
/// # }
///
/// async fn maybe_error() -> Result<String, InternalServerError<Error>> {
///     try_thing().map_err(InternalServerError)?;
///     // do something on success
///     # Ok(String::from("ok"))
/// }
/// ```
#[derive(Debug)]
pub struct InternalServerError<T>(pub T);

impl<T: Error> IntoResponse for InternalServerError<T> {
    fn into_response(self) -> Response {

Review comment (suggestion):
```suggestion
/// Convenience response to create an error response from a non-[`IntoResponse`] error
```


Code after:
use axum_core::response::{IntoResponse, Response};
use http::StatusCode;
use std::error::Error;
use tracing::error;

/// Convenience response to create an error response from a non-[`IntoResponse`] error
///
/// This provides a method to quickly respond with an error that does not implement
/// the `IntoResponse` trait itself. This type should only be used for debugging purposes or internal
/// facing applications, as it includes the full error chain with descriptions,
/// thus leaking information that could possibly be sensitive.
///
/// ```rust
/// use axum_extra::response::InternalServerError;
/// use axum_core::response::IntoResponse;
/// # use std::io::{Error, ErrorKind};
/// # fn try_thing() -> Result<(), Error> {
/// #   Err(Error::new(ErrorKind::Other, "error"))
/// # }
///
/// async fn maybe_error() -> Result<String, InternalServerError<Error>> {
///     try_thing().map_err(InternalServerError)?;
///     // do something on success
///     # Ok(String::from("ok"))
/// }
/// ```
#[derive(Debug)]
pub struct InternalServerError<T>(pub T);

impl<T: Error + 'static> IntoResponse for InternalServerError<T> {
    fn into_response(self) -> Response {
        error!(error = &self.0 as &dyn Error);
Repository: tokio-rs/axum
File: axum-extra/src/response/error_response.rs (Rust)

Code before:
use axum_core::response::{IntoResponse, Response};
use http::StatusCode;
use std::error::Error;
use std::io::Write;
use tracing::error;

/// Convenience response to create an error response from a non-IntoResponse error
///
/// This provides a method to quickly respond with an error that does not implement
/// the IntoResponse trait itself. This type should only be used for debugging purposes or internal
/// facing applications, as it includes the full error chain with descriptions,
/// thus leaking information that could possibly be sensitive.
///
/// ```rust
/// use axum_extra::response::InternalServerError;
/// use axum_core::response::IntoResponse;
/// # use std::io::{Error, ErrorKind};
/// # fn try_thing() -> Result<(), Error> {
/// #   Err(Error::new(ErrorKind::Other, "error"))
/// # }
///
/// async fn maybe_error() -> Result<String, InternalServerError<Error>> {
///     try_thing().map_err(InternalServerError)?;
///     // do something on success
///     # Ok(String::from("ok"))
/// }
/// ```
#[derive(Debug)]
pub struct InternalServerError<T>(pub T);

impl<T: Error> IntoResponse for InternalServerError<T> {
    fn into_response(self) -> Response {
        let mut body = Vec::new();
        write!(body, "{}", self.0).unwrap();
        let mut e: &dyn Error = &self.0;

Review comment (bug):
```suggestion
/// the `IntoResponse` trait itself. This type should only be used for debugging purposes or internal
```


Code after:
use axum_core::response::{IntoResponse, Response};
use http::StatusCode;
use std::error::Error;
use tracing::error;

/// Convenience response to create an error response from a non-[`IntoResponse`] error
///
/// This provides a method to quickly respond with an error that does not implement
/// the `IntoResponse` trait itself. This type should only be used for debugging purposes or internal
/// facing applications, as it includes the full error chain with descriptions,
/// thus leaking information that could possibly be sensitive.
///
/// ```rust
/// use axum_extra::response::InternalServerError;
/// use axum_core::response::IntoResponse;
/// # use std::io::{Error, ErrorKind};
/// # fn try_thing() -> Result<(), Error> {
/// #   Err(Error::new(ErrorKind::Other, "error"))
/// # }
///
/// async fn maybe_error() -> Result<String, InternalServerError<Error>> {
///     try_thing().map_err(InternalServerError)?;
///     // do something on success
///     # Ok(String::from("ok"))
/// }
/// ```
#[derive(Debug)]
pub struct InternalServerError<T>(pub T);

impl<T: Error + 'static> IntoResponse for InternalServerError<T> {
    fn into_response(self) -> Response {
        error!(error = &self.0 as &dyn Error);
        (
            StatusCode::INTERNAL_SERVER_ERROR,
            "An error occurred while processing your request.",
Repository: tokio-rs/axum
File: axum-extra/src/response/error_response.rs (Rust)

Code before:
/// use axum_extra::response::InternalServerError;
/// use axum_core::response::IntoResponse;
/// # use std::io::{Error, ErrorKind};
/// # fn try_thing() -> Result<(), Error> {
/// #   Err(Error::new(ErrorKind::Other, "error"))
/// # }
///
/// async fn maybe_error() -> Result<String, InternalServerError<Error>> {
///     try_thing().map_err(InternalServerError)?;
///     // do something on success
///     # Ok(String::from("ok"))
/// }
/// ```
#[derive(Debug)]
pub struct InternalServerError<T>(pub T);

impl<T: Error> IntoResponse for InternalServerError<T> {
    fn into_response(self) -> Response {
        let mut error: Vec<String> = Vec::new();
        error.push(format!("{}", self.0));
        let mut e: &dyn Error = &self.0;
        while let Some(new_e) = e.source() {
            e = new_e;
            error.push(format!(": {e}"))
        }
        error!("Internal server error: {}", error);
        (
            StatusCode::INTERNAL_SERVER_ERROR,
            "An error occurred while processing your request.",
        )
            .into_response()
    }
}

#[cfg(test)]
mod tests {
    use super::*;
    use std::io::{Error, ErrorKind};

    #[test]
    fn internal_server_error() {
        let response = InternalServerError(Error::new(ErrorKind::Other, "Test")).into_response();
        assert_eq!(response.status(), StatusCode::INTERNAL_SERVER_ERROR);
    }
}


Review comment (suggestion):
Tracing has built-in support for `dyn Error`s as values you can capture as fields. I think it would be good to ude that here and add an `error` field, instead of string-interpolating the error into the message. 

Code after:
/// use axum_core::response::IntoResponse;
/// # use std::io::{Error, ErrorKind};
/// # fn try_thing() -> Result<(), Error> {
/// #   Err(Error::new(ErrorKind::Other, "error"))
/// # }
///
/// async fn maybe_error() -> Result<String, InternalServerError<Error>> {
///     try_thing().map_err(InternalServerError)?;
///     // do something on success
///     # Ok(String::from("ok"))
/// }
/// ```
#[derive(Debug)]
pub struct InternalServerError<T>(pub T);

impl<T: Error + 'static> IntoResponse for InternalServerError<T> {
    fn into_response(self) -> Response {
        error!(error = &self.0 as &dyn Error);
        (
            StatusCode::INTERNAL_SERVER_ERROR,
            "An error occurred while processing your request.",
        )
            .into_response()
    }
}

#[cfg(test)]
mod tests {
    use super::*;
    use std::io::{Error, ErrorKind};

    #[test]
    fn internal_server_error() {
        let response = InternalServerError(Error::new(ErrorKind::Other, "Test")).into_response();
        assert_eq!(response.status(), StatusCode::INTERNAL_SERVER_ERROR);
    }
}
Repository: tokio-rs/axum
File: axum/src/extract/ws.rs (Rust)

Code before:
    ///
    /// // auto-deref as arg
    /// a(&data);
    ///
    /// // deref to str methods
    /// assert_eq!(data.len(), 6);
    /// ```
    #[inline]
    fn deref(&self) -> &Self::Target {
        self.as_str()
    }
}

impl std::fmt::Display for Utf8Bytes {
    #[inline]
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        f.write_str(self.as_str())
    }
}

impl TryFrom<Bytes> for Utf8Bytes {
    type Error = std::str::Utf8Error;

    #[inline]
    fn try_from(bytes: Bytes) -> Result<Self, Self::Error> {
        std::str::from_utf8(&bytes)?;
        Ok(Self(bytes.try_into()?))
    }
}

impl TryFrom<Vec<u8>> for Utf8Bytes {
    type Error = std::str::Utf8Error;

    #[inline]
    fn try_from(v: Vec<u8>) -> Result<Self, Self::Error> {
        Ok(Self(v.try_into()?))
    }
}

impl From<String> for Utf8Bytes {
    #[inline]
    fn from(s: String) -> Self {
        Self(s.into())
    }
}

impl From<&str> for Utf8Bytes {
    #[inline]
    fn from(s: &str) -> Self {
        Self(s.into())
    }

Review comment (suggestion):
This check is done by the upstream TryFrom impl

```suggestion
```

Code after:
    /// // auto-deref as arg
    /// a(&data);
    ///
    /// // deref to str methods
    /// assert_eq!(data.len(), 6);
    /// ```
    #[inline]
    fn deref(&self) -> &Self::Target {
        self.as_str()
    }
}

impl std::fmt::Display for Utf8Bytes {
    #[inline]
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        f.write_str(self.as_str())
    }
}

impl TryFrom<Bytes> for Utf8Bytes {
    type Error = std::str::Utf8Error;

    #[inline]
    fn try_from(bytes: Bytes) -> Result<Self, Self::Error> {
        Ok(Self(bytes.try_into()?))
    }
}

impl TryFrom<Vec<u8>> for Utf8Bytes {
    type Error = std::str::Utf8Error;

    #[inline]
    fn try_from(v: Vec<u8>) -> Result<Self, Self::Error> {
        Ok(Self(v.try_into()?))
    }
}

impl From<String> for Utf8Bytes {
    #[inline]
    fn from(s: String) -> Self {
        Self(s.into())
    }
}

impl From<&str> for Utf8Bytes {
    #[inline]
    fn from(s: &str) -> Self {
        Self(s.into())
    }
}
Repository: tokio-rs/axum
File: axum-core/src/macros.rs (Rust)

Code before:
                self.to_string()
            }

            /// Get the status code used for this rejection.
            pub fn status(&self) -> http::StatusCode {
                http::StatusCode::$status
            }
        }

        impl $crate::response::IntoResponse for $name {
            fn into_response(self) -> $crate::response::Response {
                let status = self.status();
                let body_text = self.body_text();

                $crate::__log_rejection!(
                    rejection_type = $name,
                    body_text = body_text,
                    status = status,
                );
                (status, body_text).into_response()
            }
        }

        impl std::fmt::Display for $name {
            fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
                write!(f, concat!($body, ": {}"), self.0)
            }
        }

        impl std::error::Error for $name {
            fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
                Some(&self.0)
            }
        }
    };
}

/// Private API.
#[doc(hidden)]
#[macro_export]
macro_rules! __composite_rejection {
    (
        $(#[$m:meta])*
        pub enum $name:ident {
            $($variant:ident),+
            $(,)?
        }
    ) => {
        $(#[$m])*
        #[derive(Debug)]
        #[non_exhaustive]

Review comment (suggestion):
I'd slightly prefer `f.write_str` to not rely on inlining as much, but that probably affects other places too and can be a separate PR once again. 

Code after:
                self.to_string()
            }

            /// Get the status code used for this rejection.
            pub fn status(&self) -> http::StatusCode {
                http::StatusCode::$status
            }
        }

        impl $crate::response::IntoResponse for $name {
            fn into_response(self) -> $crate::response::Response {
                let status = self.status();
                let body_text = self.body_text();

                $crate::__log_rejection!(
                    rejection_type = $name,
                    body_text = body_text,
                    status = status,
                );
                (status, body_text).into_response()
            }
        }

        impl std::fmt::Display for $name {
            fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
                f.write_str($body)?;
                f.write_str(": ")?;
                self.0.fmt(f)
            }
        }

        impl std::error::Error for $name {
            fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
                Some(&self.0)
            }
        }
    };
}

/// Private API.
#[doc(hidden)]
#[macro_export]
macro_rules! __composite_rejection {
    (
        $(#[$m:meta])*
        pub enum $name:ident {
            $($variant:ident),+
            $(,)?
        }
    ) => {
        $(#[$m])*
Repository: tokio-rs/axum
File: axum/src/routing/path_router.rs (Rust)

Code before:
/// use axum::vpath;
///
/// let handler = async {};
/// let router = axum::Router::<()>::new()
///     .route(vpath!("invalid_path"), get(root))
///     .to_owned();
///
/// async fn root() {}
/// ```
///
/// This one will compile without problems:
///
/// ```no_run
/// use axum::routing::*;
/// use axum::vpath;
///
/// let router = axum::Router::<()>::new()
///     .route(vpath!("/valid_path"), get(root))
///     .to_owned();
///
/// async fn root() {}
/// ```
///
macro_rules! vpath {
    ($e:expr) => {
        const { __private_validate_static_path($e) }
    };
}

#[rustversion::before(1.80)]
#[macro_export]
/// The compiler is too old for this macro to do anything useful.
/// Compile time path evaluation will work on rustc >= 1.80
macro_rules! vpath {
    ($e:expr) => {
        $e
    };
}

fn validate_path(v7_checks: bool, path: &str) -> Result<(), &'static str> {
    if path.is_empty() {
        return Err("Paths must start with a `/`. Use \"/\" for root routes");
    } else if !path.starts_with('/') {
        return Err("Paths must start with a `/`");
    }

    if v7_checks {
        validate_v07_paths(path)?;
    }

    Ok(())

Review comment (suggestion):
```suggestion
        const { $crate::routing::__private_validate_static_path($e) }
```

Code after:
                Some(Err(
                    "Path segments must not start with `*`. For wildcard capture, use \
                `{*wildcard}`. If you meant to literally match a segment starting with \
                an asterisk, call `without_v07_checks` on the router.",
                ))
            } else {
                None
            }
        })
        .unwrap_or(Ok(()))
}

impl<S, const IS_FALLBACK: bool> PathRouter<S, IS_FALLBACK>
where
    S: Clone + Send + Sync + 'static,
{
    pub(super) fn without_v07_checks(&mut self) {
        self.v7_checks = false;
    }

    pub(super) fn route(
        &mut self,
        path: &str,
        method_router: MethodRouter<S>,
    ) -> Result<(), Cow<'static, str>> {
        validate_path(self.v7_checks, path)?;

        let endpoint = if let Some((route_id, Endpoint::MethodRouter(prev_method_router))) = self
            .node
            .path_to_route_id
            .get(path)
            .and_then(|route_id| self.routes.get(route_id).map(|svc| (*route_id, svc)))
        {
            // if we're adding a new `MethodRouter` to a route that already has one just
            // merge them. This makes `.route("/", get(_)).route("/", post(_))` work
            let service = Endpoint::MethodRouter(
                prev_method_router
                    .clone()
                    .merge_for_path(Some(path), method_router),
            );
            self.routes.insert(route_id, service);
            return Ok(());
        } else {
            Endpoint::MethodRouter(method_router)
        };

        let id = self.next_route_id();
        self.set_node(path, id)?;
        self.routes.insert(id, endpoint);

        Ok(())
Repository: tokio-rs/axum
File: axum/src/routing/path_router.rs (Rust)

Code before:
        self.replace_endpoint("/", endpoint.clone());
        self.replace_endpoint(FALLBACK_PARAM_PATH, endpoint);
    }
}

// Validates a path in compile time, used with the vpath macro.
// Allow dead code needed because this is only used within the vpath macro
// and if there's no call to it, then, there's no users of it.
#[allow(dead_code)]
#[doc(hidden)]
pub const fn __private_validate_static_path(path: &'static str) -> &'static str {
    if path.is_empty() {
        panic!("Paths must start with a `/`. Use \"/\" for root routes")
    }
    if path.as_bytes()[0] != b'/' {
        panic!("Paths must start with /");
    }
    path
}

#[rustversion::since(1.80)]
#[macro_export]
/// This macro checks that your string is the correct format to be used as a route,
/// and fails in compilation time instead of runtime, if it's not.
///
/// This example will stop the compialtion:
///
/// ```compile_fail
/// use axum::routing::*;
/// use axum::vpath;
///
/// let handler = async {};
/// let router = axum::Router::<()>::new()
///     .route(vpath!("invalid_path"), get(root))
///     .to_owned();
///
/// async fn root() {}
/// ```
///
/// This one will compile without problems:
///
/// ```no_run
/// use axum::routing::*;
/// use axum::vpath;
///
/// let router = axum::Router::<()>::new()
///     .route(vpath!("/valid_path"), get(root))
///     .to_owned();
///
/// async fn root() {}
/// ```

Review comment (suggestion):
```suggestion
/// This example will stop the compilation:
```

Code after:
        self.replace_endpoint("/", endpoint.clone());
        self.replace_endpoint(FALLBACK_PARAM_PATH, endpoint);
    }
}

fn validate_path(v7_checks: bool, path: &str) -> Result<(), &'static str> {
    if path.is_empty() {
        return Err("Paths must start with a `/`. Use \"/\" for root routes");
    } else if !path.starts_with('/') {
        return Err("Paths must start with a `/`");
    }

    if v7_checks {
        validate_v07_paths(path)?;
    }

    Ok(())
}

fn validate_v07_paths(path: &str) -> Result<(), &'static str> {
    path.split('/')
        .find_map(|segment| {
            if segment.starts_with(':') {
                Some(Err(
                    "Path segments must not start with `:`. For capture groups, use \
                `{capture}`. If you meant to literally match a segment starting with \
                a colon, call `without_v07_checks` on the router.",
                ))
            } else if segment.starts_with('*') {
                Some(Err(
                    "Path segments must not start with `*`. For wildcard capture, use \
                `{*wildcard}`. If you meant to literally match a segment starting with \
                an asterisk, call `without_v07_checks` on the router.",
                ))
            } else {
                None
            }
        })
        .unwrap_or(Ok(()))
}

impl<S, const IS_FALLBACK: bool> PathRouter<S, IS_FALLBACK>
where
    S: Clone + Send + Sync + 'static,
{
    pub(super) fn without_v07_checks(&mut self) {
        self.v7_checks = false;
    }

    pub(super) fn route(
        &mut self,