Add std::error::Error impls to Error enums of parameter module

[alluring-mushroom]

Adds error impls to the error types used by rclrs parameters.

See #412.

[jhdcs]

Answering your questions that you have listed here: #412

"It occured to me that good error messages that are displayed to an end user for parameters should likely include the parameter in question: its name and also possibly the description used in ParameterBuilder. This would require altering the Error struct to store the ParameterBuilder, or at least the relevant information. How do you feel about that?"

I think that adding in the name and description of the parameter would be a good idea. Also, if applicable, reporting the value might be useful. Especially for an "Out Of Range" or "Type Mismatch" error.

"Do you think it is a good idea to include ros2 links in these messages?"

I think that would be good, though we would have to keep an eye on the links to avoid link rot. For example, we would probably have to modify the links for each new version of ROS 2 that's supported. Not necessarily a dealbreaker, but something to keep in mind.

"I'm not sure about the change in rclrs/src/parameter/value.rs , as that code was fairly dense. I wasn't immediatly sure if RmwParameterConversionError would be user facing and would need an Error impl, originally I had only found the Errors in rclrs/src/parameter.rs"

I'll need to do some digging on this one to figure out what's going on. Usually rmw stuff isn't user-facing - it deals with the underlying communication methods between nodes. However, it's not fully my area of expertise, so hopefully one of the other maintainers/contributors can chime in to explain what's happening...

[luca-della-vedova]

Just for curiosity, what would be the feeling on introducing a dependency to make this a bit less verbose such as thiserror?
But in general thanks for the contribution! I agree with @jhdcs that we could improve the errors to include a value, but that could be tricky when we want to print a generic type and don't want to make the error itself generic, so I understand if it is out of scope of this PR

[alluring-mushroom]

I think the best gain for thiserror here is that the macro describing the variant is next to the variant, and this keeps more things clear. I will leave that up to everyone here though.

I have struggled a bit with including the value. As you say, it requires the error to be generic, but I have run into another issue.
Many of the dependencies of the parameter types aren't Debug. Thus, many places need the derive. This includes vendored code. I could manually implement Debug, but that felt sketchy.
When I work on this again in the next couple of days, I'll push code and add references to this/another comment.

I can see two alternatives to include the data in a message.

1. Include a String in the variant, and stringify the name description and value when the error is generated. This means no generics
2. Include the name, description and value in the error, and combine in the display impl. Don't use the Parameter types directly. Requires generics, but can use references. Also allows error handling code to use these (maybe unnecessary?)

I figure I might just finish the PR with nice messages so we have Error impls, and then start a new one with one of these three solutions to improve the error messages.

[luca-della-vedova]

Yap fully agree also with the split into another PR! I personally feel the cost of an additional (small) dependency is worth the clarity of less typing and, as you said, keeping the display message in the same place where the error is defined but I'll leave one of the main maintainers to comment on that

[jhdcs]

While I usually think it's a good idea to try and limit the number of dependencies for a core library, I think that thiserror is a small enough size with a large enough benefit that it might be warranted for inclusion. If it makes contributing quality code easier for people, I think we can certainly take a look at adding it into the project.

How much work would need to be done to transform our current method of creating errors into using thiserror? Because if we do include it, we should probably use it library-wide for consistency.

[alluring-mushroom]

This is what a single error conversion looks like, for one of the ones I added:
diff

--- a/rclrs/src/parameter.rs
+++ b/rclrs/src/parameter.rs
@@ -18,6 +18,8 @@ use std::{
     sync::{Arc, Mutex, RwLock, Weak},
 };

+use thiserror::Error;
+
 // This module implements the core logic of parameters in rclrs.
 // The implementation is fairly different from the existing ROS 2 client libraries. A detailed
 // explanation of the core differences and why they have been implemented is available at:
@@ -614,28 +616,21 @@ pub struct Parameters<'a> {
 }

 /// Describes errors that can be generated when trying to set a parameter's value.
-#[derive(Debug)]
+#[derive(Error, Debug)]
 pub enum ParameterValueError {
+    #[error("parameter value was out of the parameter's range")]
     /// Parameter value was out of the parameter's range.
     OutOfRange,
     /// Parameter was stored in a static type and an operation on a different type was attempted.
+    #[error(
+        "parameter is stored in a static type and an operation on a different type was attempted"
+    )]
     TypeMismatch,
     /// A write on a read-only parameter was attempted.
+    #[error("a write on a read-only parameter was attempted")]
     ReadOnly,
 }

-impl std::fmt::Display for ParameterValueError {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        match self {
-            ParameterValueError::OutOfRange => write!(f, "parameter value was out of the parameter's range"),
-            ParameterValueError::TypeMismatch => write!(f, "parameter was stored in a static type and an operation on a different type was attempted"),
-            ParameterValueError::ReadOnly => write!(f, "a write on a read-only parameter was attempted"),
-        }
-    }
-}
-
-impl std::error::Error for ParameterValueError {}
-
 /// Error that can be generated when doing operations on parameters.
 #[derive(Debug)]
 pub enum DeclarationError {

Unfortunately I learned that derive macros cannot generate doc comments, so we will still need those, but at least they are next to each other.

There aren't that many errors that we would need to convert I think:

rg "impl.*Error for"
rclrs/src/parameter/value.rs
395:impl std::error::Error for RmwParameterConversionError {}

rclrs/src/dynamic_message/error.rs
45:impl Error for DynamicMessageError {

rclrs/src/error.rs
74:impl Error for RclErrorMsg {}
76:impl Error for RclrsError {
317:impl Error for RclReturnCode {}

rclrs/src/parameter.rs
637:impl std::error::Error for ParameterValueError {}
678:impl std::error::Error for DeclarationError {}

rosidl_runtime_rs/src/sequence.rs
493:impl std::error::Error for SequenceExceedsBoundsError {}

rosidl_runtime_rs/src/string.rs
505:impl std::error::Error for StringExceedsBoundsError {}

I've taken this out of draft mode, as I figured I would merge this change before submitting another PR for the thiserror change, but I can include it all in this one if you like

[luca-della-vedova]

Imho this is fairly straightforward and uncontroversial and I can see the argument in favor of leaving the cleanup to a separate PR and just have a consistent implementation first

[esteve]

@alluring-mushroom thanks!