feat(parameters): derive descriptor constraints from the parameter type

rclrs/src/parameter.rs

@@ -66,6 +66,23 @@ impl Default for ParameterOptions {
     }
 }
 
+/// Fills in the constraints a declaration did not give, from whatever the conversion has to say.
+///
+/// Introspection through `ros2 param describe` is only useful if the descriptor says what a value
+/// may be, and a type that has rules of its own should not need every declaration site to restate
+/// them. An explicit `constraints` on the declaration always wins.
+fn resolve_constraints<T: 'static>(
+    mut options: ParameterOptions,
+    conversion: &ParameterConversion<T>,
+) -> ParameterOptions {
+    if options.constraints.is_empty() {
+        if let Some(constraints) = conversion.constraints() {
+            options.constraints = constraints;
+        }
+    }
+    options
+}
+
 #[derive(Clone, Debug)]
 enum DeclaredValue {
     Mandatory(Arc<RwLock<ParameterValue>>),
@@ -359,7 +376,7 @@ impl<T: 'static> TryFrom<ParameterBuilder<'_, T>> for OptionalParameter<T> {
             DeclaredStorage {
                 value: DeclaredValue::Optional(value.clone()),
                 kind: builder.conversion.kind(),
-                options: builder.options,
+                options: resolve_constraints(builder.options, &builder.conversion),
                 type_check: type_check_of(builder.conversion.clone()),
                 validate: type_erased_validate,
                 on_change: None,
@@ -452,7 +469,7 @@ impl<T: 'static> TryFrom<ParameterBuilder<'_, T>> for MandatoryParameter<T> {
             DeclaredStorage {
                 value: DeclaredValue::Mandatory(value.clone()),
                 kind: builder.conversion.kind(),
-                options: builder.options,
+                options: resolve_constraints(builder.options, &builder.conversion),
                 type_check: type_check_of(builder.conversion.clone()),
                 validate: type_erased_validate,
                 on_change: None,
@@ -566,7 +583,7 @@ impl<T: 'static> TryFrom<ParameterBuilder<'_, T>> for ReadOnlyParameter<T> {
             DeclaredStorage {
                 value: DeclaredValue::ReadOnly(value.clone()),
                 kind: builder.conversion.kind(),
-                options: builder.options,
+                options: resolve_constraints(builder.options, &builder.conversion),
                 type_check: type_check_of(builder.conversion.clone()),
                 // A read-only parameter never changes, so it needs neither the validate
                 // callback nor a change notification channel.
@@ -1359,6 +1376,84 @@ impl ParameterInterface {
     }
 }
 
+/// Shared support for the parameter tests in this module and its children.
+#[cfg(test)]
+pub(crate) mod test_support {
+    use super::*;
+    use crate::Node;
+    use ros_env::rcl_interfaces::{
+        msg::rmw::ParameterDescriptor, srv::rmw::DescribeParameters_Request,
+    };
+    use rosidl_runtime_rs::{seq, Sequence};
+
+    /// The descriptor a node reports for one of its parameters, as `ros2 param describe` gets it.
+    pub(crate) fn parameter_descriptor(node: &Node, name: &str) -> ParameterDescriptor {
+        let map = node.parameter_interface().parameter_map.lock().unwrap();
+        let response = crate::parameter::service::describe_parameters(
+            DescribeParameters_Request {
+                names: seq![name.into()],
+            },
+            &map,
+        );
+        response
+            .descriptors
+            .into_iter()
+            .next()
+            .expect("a descriptor is returned for every requested name")
+    }
+
+    /// A string-backed parameter type, of the kind a user can define today by implementing the
+    /// public [`ParameterVariant`] trait. Its conversion from [`ParameterValue`] is *partial*:
+    /// a value can be a `String`, and so pass any check based on [`ParameterKind`], and still
+    /// not be a valid `Switch`.
+    #[derive(Clone, Debug, PartialEq)]
+    pub(crate) enum Switch {
+        On,
+        Off,
+    }
+
+    impl From<Switch> for ParameterValue {
+        fn from(value: Switch) -> Self {
+            ParameterValue::String(
+                match value {
+                    Switch::On => "on",
+                    Switch::Off => "off",
+                }
+                .into(),
+            )
+        }
+    }
+
+    impl TryFrom<ParameterValue> for Switch {
+        type Error = ParameterValueError;
+
+        fn try_from(value: ParameterValue) -> Result<Self, Self::Error> {
+            match value {
+                ParameterValue::String(s) => match s.as_ref() {
+                    "on" => Ok(Switch::On),
+                    "off" => Ok(Switch::Off),
+                    other => Err(ParameterValueError::Invalid(format!(
+                        "unknown Switch '{other}', expected one of: on, off"
+                    ))),
+                },
+                _ => Err(ParameterValueError::TypeMismatch),
+            }
+        }
+    }
+
+    impl ParameterVariant for Switch {
+        type Range = ();
+
+        fn kind() -> ParameterKind {
+            ParameterKind::String
+        }
+
+        fn type_constraints() -> Option<Arc<str>> {
+            Some("one of: on, off".into())
+        }
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -2366,52 +2461,7 @@ mod tests {
         assert_eq!(sub.get(), 75);
     }
 
-    /// A string-backed parameter type, of the kind a user can define today by implementing the
-    /// public `ParameterVariant` trait. Its conversion from `ParameterValue` is *partial*: a
-    /// value can be a `String`, and so pass any check based on `ParameterKind`, and still not
-    /// be a valid `Switch`.
-    #[derive(Clone, Debug, PartialEq)]
-    enum Switch {
-        On,
-        Off,
-    }
-
-    impl From<Switch> for ParameterValue {
-        fn from(value: Switch) -> Self {
-            ParameterValue::String(
-                match value {
-                    Switch::On => "on",
-                    Switch::Off => "off",
-                }
-                .into(),
-            )
-        }
-    }
-
-    impl TryFrom<ParameterValue> for Switch {
-        type Error = ParameterValueError;
-
-        fn try_from(value: ParameterValue) -> Result<Self, Self::Error> {
-            match value {
-                ParameterValue::String(s) => match s.as_ref() {
-                    "on" => Ok(Switch::On),
-                    "off" => Ok(Switch::Off),
-                    other => Err(ParameterValueError::Invalid(format!(
-                        "unknown Switch '{other}', expected one of: on, off"
-                    ))),
-                },
-                _ => Err(ParameterValueError::TypeMismatch),
-            }
-        }
-    }
-
-    impl ParameterVariant for Switch {
-        type Range = ();
-
-        fn kind() -> ParameterKind {
-            ParameterKind::String
-        }
-    }
+    use super::test_support::Switch;
 
     fn rmw_string(value: &str) -> RmwParameterValue {
         RmwParameterValue {

rclrs/src/parameter/conversion.rs

@@ -41,6 +41,9 @@ use super::{ParameterKind, ParameterValue, ParameterVariant};
 /// ```
 pub struct ParameterConversion<T> {
     kind: ParameterKind,
+    /// What a value of this conversion may be, in words, for the descriptor's
+    /// `additional_constraints`. Nothing enforces it, and a declaration that states its own wins.
+    constraints: Option<Arc<str>>,
     to_value: Arc<dyn Fn(&T) -> ParameterValue + Send + Sync>,
     from_value: Arc<dyn Fn(ParameterValue) -> Result<T, String> + Send + Sync>,
 }
@@ -51,6 +54,7 @@ impl<T> Clone for ParameterConversion<T> {
     fn clone(&self) -> Self {
         Self {
             kind: self.kind,
+            constraints: self.constraints.clone(),
             to_value: Arc::clone(&self.to_value),
             from_value: Arc::clone(&self.from_value),
         }
@@ -84,6 +88,7 @@ macro_rules! conversion_constructor {
         ) -> ParameterConversion<T> {
             ParameterConversion {
                 kind: ParameterKind::$variant,
+                constraints: ::core::option::Option::None,
                 to_value: Arc::new(move |value| ParameterValue::$variant(to(value))),
                 from_value: Arc::new(move |value| match value {
                     ParameterValue::$variant(wire) => from(wire).map_err(|err| err.to_string()),
@@ -125,6 +130,21 @@ impl<T: 'static> ParameterConversion<T> {
         self.kind
     }
 
+    /// Describes in words what a value of this conversion may be.
+    ///
+    /// Reported as the descriptor's `additional_constraints` by any declaration that does not
+    /// state constraints of its own, so that `ros2 param describe` can say what a value has to
+    /// satisfy when the rule is not one a range can express.
+    pub fn with_constraints(mut self, constraints: impl Into<Arc<str>>) -> Self {
+        self.constraints = Some(constraints.into());
+        self
+    }
+
+    /// The constraints this conversion describes, if any.
+    pub fn constraints(&self) -> Option<Arc<str>> {
+        self.constraints.clone()
+    }
+
     /// Converts a value into the parameter value that represents it.
     pub fn to_value(&self, value: &T) -> ParameterValue {
         (self.to_value)(value)
@@ -145,6 +165,7 @@ impl<T: ParameterVariant> ParameterConversion<T> {
     pub fn of_variant() -> Self {
         Self {
             kind: T::kind(),
+            constraints: T::type_constraints(),
             to_value: Arc::new(|value: &T| value.clone().into()),
             from_value: Arc::new(|value| T::try_from(value).map_err(|err| err.to_string())),
         }

rclrs/src/parameter/service.rs

@@ -35,7 +35,7 @@ pub struct ParameterService {
     set_parameters_atomically_service: Service<SetParametersAtomically>,
 }
 
-fn describe_parameters(
+pub(crate) fn describe_parameters(
     req: DescribeParameters_Request,
     map: &ParameterMap,
 ) -> DescribeParameters_Response {
@@ -833,6 +833,49 @@ mod tests {
         Ok(())
     }
 
+    /// A parameter's own type can describe what it accepts, so that `ros2 param describe`
+    /// reports it without the declaration having to restate it, and without that restatement
+    /// drifting from the type as it gains variants.
+    #[test]
+    fn test_descriptor_constraints_come_from_the_parameter_type() {
+        use crate::parameter::test_support::{parameter_descriptor, Switch};
+
+        let node = Context::default()
+            .create_basic_executor()
+            .create_node("describe_type_constraints")
+            .unwrap();
+
+        // No `.constraints()` on either declaration: the type supplies the text.
+        let _inherited: MandatoryParameter<Switch> = node
+            .declare_parameter("inherited")
+            .default(Switch::On)
+            .mandatory()
+            .unwrap();
+        // An explicit constraint still wins, for rules specific to this declaration.
+        let _explicit: MandatoryParameter<Switch> = node
+            .declare_parameter("explicit")
+            .default(Switch::On)
+            .constraints("must be off on tuesdays")
+            .mandatory()
+            .unwrap();
+        // Types that say nothing about themselves are unaffected.
+        let _plain: MandatoryParameter<i64> = node
+            .declare_parameter("plain")
+            .default(1)
+            .mandatory()
+            .unwrap();
+
+        let constraints = |name| {
+            parameter_descriptor(&node, name)
+                .additional_constraints
+                .to_string()
+        };
+
+        assert_eq!(constraints("inherited"), "one of: on, off");
+        assert_eq!(constraints("explicit"), "must be off on tuesdays");
+        assert_eq!(constraints("plain"), "");
+    }
+
     #[test]
     fn test_describe_get_types_parameters_service() -> Result<(), RclrsError> {
         let (mut executor, _test, client_node) = construct_test_nodes("describe");

rclrs/src/parameter/value.rs

@@ -144,6 +144,19 @@ pub trait ParameterVariant:
 
     /// Returns the `ParameterKind` of the implemented type.
     fn kind() -> ParameterKind;
+
+    /// Human-readable constraints that are inherent to this type, such as the set of variants
+    /// a string-backed enum accepts.
+    ///
+    /// Used for the parameter descriptor's `additional_constraints` field when the declaration
+    /// does not set its own with [`ParameterBuilder::constraints`], so that introspection
+    /// through `ros2 param describe` can report what a value of this type may be without every
+    /// declaration site having to restate it.
+    ///
+    /// [`ParameterBuilder::constraints`]: crate::ParameterBuilder::constraints
+    fn type_constraints() -> Option<Arc<str>> {
+        None
+    }
 }
 
 impl TryFrom<ParameterValue> for bool {