Skip to content

fn type_name_of_val<T> is non-deterministic #243

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
rimutaka opened this issue Jul 22, 2020 · 6 comments
Closed

fn type_name_of_val<T> is non-deterministic #243

rimutaka opened this issue Jul 22, 2020 · 6 comments
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@rimutaka
Copy link
Contributor

It seems that function fn type_name_of_val in lib.rs always returns the same value:

alloc::boxed::Box<dyn std::error::Error+core::marker::Sync+core::marker::Send>

which is propagated to the response and CloudWatch.

That value is not user-friendly and is hard to parse downstream.

Other parts of the code return values like InvalidEventDataError, which is friendly and consistent with what other runtimes do.

Proposal

  1. Confirm if fn type_name_of_val is in fact non-deterministic.
  2. Find a way to allow the handler code set the error type, e.g. Error logging and examples #242 (comment)
  3. In the interim, replace the function with a constant value, e.g. "HandlerError" for the next release.

Making later changes to this part of the code will break compatibility for the handler code, client applications processing the lambda response and monitoring applications parsing CloudWatch message. I would prefer us to make an interim change to minimize that impact.

If there is no consensus on this, we may just advise the developers to ignore this field for now and rely on the structured data inside Diagnostic.error_message.

@Veetaha
Copy link
Contributor

Veetaha commented Jul 22, 2020

As for me enforcing the error shape is quite limiting in AWS api (as well as e.g. the shape of messages (enforcing JSON, XML instead of using raw bytes (hello protobuf), sorry for offtopic...).

We might try not hardcoding HandlerError string, but actually use the dynamic type name of the object.
As to implementation, here is what comes to my mind (playground link):

// user code

#[derive(Debug)]
struct FooError {
    foo: i32,
}

todo_impl_display!(FooError); // imagine std::fmt::Display impl here
impl Error for FooError {}

#[derive(Debug)]
enum EnumedError {
    A, B,
}
todo_impl_display!(EnumedError);
impl Error for EnumedError {}
// Nope: This will confict with the blanket impl
// impl lambda_runtime::HanderError for EnumedError {
//     fn type_name(&self) -> &'static str {
//         match self {
//             EnumedError::A => "A",
//             EnumedError::B => "B"
//         }
//     }
// }

fn main() {
    let err = lambda();
    println!("type_name: {}", (*err).type_name());
}

fn lambda() -> Box<dyn lambda_runtime::HanderError> {
    if 2 + 2 == 4 {
        Box::new(FooError { foo: 42 })
    } else {
        Box::new(EnumedError::A)
    }
}

// lambda_runtime/lib.rs
use std::error::Error;

trait HandlerError: Error + Send + Sync {
    fn type_name(&self) -> &'static str {
        std::any::type_name::<Self>()
    }
}
impl<T> HandlerError for T where T: Error + Send + Sync {}

The problem with this implementation is that even thou we can provide the automatic type name of the error we cannot guarantee its semver stability, because as from std::type_name() docs:

The returned string must not be considered to be a unique identifier of a
type as multiple types may map to the same type name
Similarly, there is no
guarantee that all parts of a type will appear in the returned string: for
example, lifetime specifiers are currently not included. In addition, the
output may change between versions of the compiler.

And we cannot allow the user to override HandlerError trait impl. Even thou rust-lang is planning to add specialized implementations in future.
For this one we can have a workaround. Since most users don't want to implement HandlerError by hand and/or will use the error type API only for internal diagnostics we might just add a feature to conditionally compile lambda_runtime crate with a blanket impl for HandlerError or not. Another option would be to provide a macro impl_type_name!(UserError) that automatically generates an implementation of HandlerError for UserEror that uses std::any::type_name::<UserError>() in its body.

So at the end of the day, the blocker for nice implementation is the specialization feature (rust-lang/rust#31844) which has been unstable for ages and unknown to ever become stable...

cc @davidbarsky @softprops

@Veetaha
Copy link
Contributor

Veetaha commented Jul 22, 2020

Hmm, an idea has just come to my head suddenly.
We might just provide a proc macro derive for HandlerError:

use lambda_runtime::HandlerError;

#[derive(HandlerError)]
#[error_type = "USER_ERROR"] // this bit is optional (uses std::any::type_name::<T>() or literally struct Identifier by default?)
struct UserError {
    why_i_failed: String
}

@davidbarsky
Copy link
Contributor

Hmm, an idea has just come to my head suddenly. We might just provide a proc macro derive for HandlerError:

We used to that in the previous iteration of this runtime, lots of folks complained. This current type_name-based was a reaction to that approach.

As for your previous, insightful, comment, those are all good points. I've tried to address portions of your comment in #241 (comment), but I recognize that I didn't fully address it.

@Veetaha
Copy link
Contributor

Veetaha commented Jul 23, 2020

Yup, I use what is currently at crates.io but there is no proc macro derive for LambdaErrorExt there (or I might be missing something). I think proc macro derive would just cut the boilerplate enough to allow for convenience in general case. Also we might have a

/// ErrorType generated by it is
/// std::any::type_name() of the underlying error
/// No semver guarantees, use it only for diagnostics
struct DiagnosticError {
    err: Box<dyn Error + Send + Sync>,
    kind: &'static str,
}

impl<T> From<T> for DiagnosticError
where
    T: Error + Send + Sync
{
    fn from(err: T) -> DiagnosticError {
        DiagnosticError {
            err: Box::new(err),
            kind: std::any::type_name::<T>()
         }
    }
}
impl HandlerErrror for DiagnosticError {
    fn error_type(&self) -> String { // we might use fmt::Formatter here instead
        self.kind.to_owned()
    }
}

This way users will have this utility DiagnosticError for general simple cases, but they might use proc macro derive or implement HandlerErrror manually if they rally need to

@jkshtj jkshtj added the bug Something isn't working label Jan 21, 2021
@calavera calavera added the help wanted Extra attention is needed label Feb 19, 2022
@calavera
Copy link
Contributor

👋 hey folks this issue has been stale for two years. I've decided to close it because the runtime has changed significantly enough and this is not a significant issue. If you feel strongly about keeping it open let me know, happy to discuss further.

@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for the maintainers of this repository to see.
If you need more assistance, please open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants