add .source() support to client error enums and clean up some clien…

…t error definitions

Summary:
It'd be helpful to be able to call `.source()` on a Thrift client error, as that would make writing dynamic retry policies far easier and allow them to be used for multiple Thrift client methods. Normally to enable this, we just need to add the `#[source]` attribute (from thiserror) to the inner value of our error enums. But this results in bad error chaining and duplicated error messages when printing out the error chain (say with `anyhow`).

I think the best solution here is to take a page out of [anyhow](https://docs.rs/anyhow/latest/anyhow/)'s book, specifically the `{:#}` semantics for `Display`. This way, we can omit printing the cause when printing with `{}` (and thus avoiding duplicate error messages when printing with `anyhow`), but one can choose to retain the behavior from before with `{:#}`.

Aside from this, there's also a few cleanups we need to do with how Thrift client errors are laid out to support `.source()`, mainly:
1. Add `#[derive(thiserror::Error)]` to `ApplicationException`
2. This then requires us to remove the `impl From<ApplicationException> for Error` in `errors.rs` as it conflicts with `anyhow`'s existing blanket impl of `From<T: std::error::Error>`
3. Removing facebook#2 causes issues in `unionimpl.mustache` which does rely on that impl. However, `unionimpl`'s usage just returns a `ProtocolError` that wraps an `ApplicationException` that has a `ProtocolError` as an error code... so just flatten that to a new `ProtocolError` variant
4. It appears that this `unionimpl` was the only time we used the `ProtocolError::ApplicationException` variant, so just remove that. Feels weird to wrap an `ApplicationException` inside of a `ProtocolError` anyways.

inspired by https://sabrinajewson.org/blog/errors#guidelines-for-good-errors.

Reviewed By: edward-shen

Differential Revision: D44808129

fbshipit-source-id: 051c4c499bc2aa309e6319e6da59ffb3788a55a3

src/application_exception.rs

@@ -53,9 +53,18 @@ pub enum ApplicationExceptionErrorCode {
 
 const TAPPLICATION_EXCEPTION_ERROR_CODE: &str = "ApplicationExceptionErrorCode";
 
-/// ApplicationException is *not* actually an error type in the Rust sense, but
-/// a Thrift-specific serializable
-#[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd, Hash, Default)]
+#[derive(
+    Debug,
+    Clone,
+    Eq,
+    PartialEq,
+    Ord,
+    PartialOrd,
+    Hash,
+    Default,
+    thiserror::Error
+)]
+#[error("{type_:?}: {message}")]
 pub struct ApplicationException {
     pub message: String,
     pub type_: ApplicationExceptionErrorCode,

src/dep_tests/exceptions.rs

@@ -50,7 +50,7 @@ fn test_should_use_message_field_override() -> Result<()> {
         ..Default::default()
     };
     assert_eq!(
-        format!("TestExceptionMsgOverride: {}: {:?}", err.message, err),
+        format!("TestExceptionMsgOverride: {} ({:?})", err.message, err),
         format!("{}", err)
     );
 
@@ -65,7 +65,7 @@ fn test_should_use_message_field_override_optional() -> Result<()> {
     };
     assert_eq!(
         format!(
-            "TestExceptionMsgOverrideOptional: Some(\"{}\"): {:?}",
+            "TestExceptionMsgOverrideOptional: Some(\"{}\") ({:?})",
             err.message.as_ref().unwrap(),
             err
         ),
@@ -77,7 +77,7 @@ fn test_should_use_message_field_override_optional() -> Result<()> {
         ..Default::default()
     };
     assert_eq!(
-        format!("TestExceptionMsgOverrideOptional: None: {:?}", err),
+        format!("TestExceptionMsgOverrideOptional: None ({:?})", err),
         format!("{}", err)
     );
 

src/errors.rs

@@ -49,33 +49,15 @@ pub enum ProtocolError {
     InvalidValue,
     #[error("Unexpected trailing data after the end of a value")]
     TrailingData,
-    #[error("Application exception: {0:?}")]
-    ApplicationException(ApplicationException),
-}
-
-impl From<ApplicationException> for Error {
-    fn from(exn: ApplicationException) -> Error {
-        ProtocolError::ApplicationException(exn).into()
-    }
+    #[error("Unwanted extra union {0} field ty {1:?} id {2}")]
+    UnwantedExtraUnionField(String, TType, i32),
 }
 
 /// Error value returned by functions that do not throw any user-defined exceptions.
 #[derive(Debug, Error)]
 pub enum NonthrowingFunctionError {
-    #[error("Application exception: {0:?}")]
-    ApplicationException(ApplicationException),
-    #[error("{0}")]
-    ThriftError(Error),
-}
-
-impl From<Error> for NonthrowingFunctionError {
-    fn from(err: Error) -> Self {
-        NonthrowingFunctionError::ThriftError(err)
-    }
-}
-
-impl From<ApplicationException> for NonthrowingFunctionError {
-    fn from(ae: ApplicationException) -> Self {
-        NonthrowingFunctionError::ApplicationException(ae)
-    }
+    #[error(transparent)]
+    ApplicationException(#[from] ApplicationException),
+    #[error(transparent)]
+    ThriftError(#[from] Error),
 }