From 7c9cac1cd1b021efe171a0a411d01ee1276a9e9c Mon Sep 17 00:00:00 2001 From: Mathieu David Date: Sat, 25 Jul 2026 15:38:41 +0200 Subject: [PATCH 1/3] fix(parameters): validate values against the declared Rust type A ParameterKind does not uniquely identify a Rust type. Several types can share one, and a value of the right kind can still be unrepresentable in the type a parameter was declared with. validate_parameter_setting only compared kind discriminants and Parameters::set only compared T::kind(), while every getter assumes the stored value converts back: self.value.read().unwrap().clone().try_into().ok().unwrap() So a remote SetParameters call could store a value that passes validation and then panic the node on the next read. This is reachable today by anyone implementing the public ParameterVariant trait for a type whose conversion from ParameterValue is partial, and becomes unavoidable once narrow integer and enum parameter types exist. Capture the declared type's check as a fn pointer at declaration time and run it on both write paths, before the range check and before the custom validate callback (whose type-erasure wrapper depends on the value being convertible). The check is the type's own TryFrom, so there is nothing extra for a type to implement and no way for it to accept a value that a later read would then reject. ParameterValueError::Invalid carries the reason the conversion gave back to the caller, which the parameter service puts in the response so the operator who made the call can see why the value was refused. --- rclrs/src/node.rs | 6 + rclrs/src/parameter.rs | 267 ++++++++++++++++++++++++++++++----- rclrs/src/parameter/value.rs | 2 +- 3 files changed, 236 insertions(+), 39 deletions(-) diff --git a/rclrs/src/node.rs b/rclrs/src/node.rs index 8fdccad7..dee1ffff 100644 --- a/rclrs/src/node.rs +++ b/rclrs/src/node.rs @@ -1420,6 +1420,12 @@ impl NodeState { self.parameter.declare(name.into()) } + /// Access to this node's parameter interface, for the parameter implementation itself. + #[cfg(test)] + pub(crate) fn parameter_interface(&self) -> &ParameterInterface { + &self.parameter + } + /// Enables usage of undeclared parameters for this node. /// /// Returns a [`Parameters`] struct that can be used to get and set all parameters. diff --git a/rclrs/src/parameter.rs b/rclrs/src/parameter.rs index 1ff3415a..5b5ad601 100644 --- a/rclrs/src/parameter.rs +++ b/rclrs/src/parameter.rs @@ -278,9 +278,9 @@ type DiscriminatorFunction<'a, T> = Box) -> Option /// Wraps a typed validate callback into a type-erased one that operates on `ParameterValue`. /// -/// The `expect` here is safe: this callback is only invoked from -/// `validate_parameter_setting` which checks the type discriminant first, -/// and from `Parameters::set()` which checks `T::kind() == param.kind`. +/// The `expect` here is safe: this callback is only invoked from `validate_parameter_setting` +/// and `Parameters::set()`. Both of them run the declaration's `type_check`, which is this same +/// conversion, before reaching it. fn wrap_validate_callback( callback: Arc Result<(), String> + Send + Sync>, ) -> ValidateCallback { @@ -325,11 +325,15 @@ impl TryFrom> for OptionalParameter builder.interface.store_parameter( builder.name.clone(), - T::kind(), - DeclaredValue::Optional(value.clone()), - builder.options.into(), - type_erased_validate, - Some(change_tx.clone()), + DeclaredStorage { + value: DeclaredValue::Optional(value.clone()), + kind: T::kind(), + options: builder.options.into(), + type_check: type_check_of::, + validate: type_erased_validate, + on_change: None, + change_tx: Some(change_tx.clone()), + }, ); Ok(OptionalParameter { name: builder.name, @@ -412,11 +416,15 @@ impl TryFrom> for MandatoryParamete builder.interface.store_parameter( builder.name.clone(), - T::kind(), - DeclaredValue::Mandatory(value.clone()), - builder.options.into(), - type_erased_validate, - Some(change_tx.clone()), + DeclaredStorage { + value: DeclaredValue::Mandatory(value.clone()), + kind: T::kind(), + options: builder.options.into(), + type_check: type_check_of::, + validate: type_erased_validate, + on_change: None, + change_tx: Some(change_tx.clone()), + }, ); Ok(MandatoryParameter { name: builder.name, @@ -519,11 +527,17 @@ impl TryFrom> for ReadOnlyParameter let value = initial_value.into(); builder.interface.store_parameter( builder.name.clone(), - T::kind(), - DeclaredValue::ReadOnly(value.clone()), - builder.options.into(), - None, - None, + DeclaredStorage { + value: DeclaredValue::ReadOnly(value.clone()), + kind: T::kind(), + options: builder.options.into(), + type_check: type_check_of::, + // A read-only parameter never changes, so it needs neither the validate + // callback nor a change notification channel. + validate: None, + on_change: None, + change_tx: None, + }, ); Ok(ReadOnlyParameter { name: builder.name, @@ -537,10 +551,31 @@ impl TryFrom> for ReadOnlyParameter type ValidateCallback = Arc Result<(), String> + Send + Sync>; type OnChangeCallback = Arc) + Send + Sync>; +/// Checks that a value is usable for a parameter declared as `T`. +/// +/// A [`ParameterKind`] does not uniquely identify a Rust type: several types can share one, and a +/// value of the right kind can still be unrepresentable (an integer that does not fit a [`u16`], a +/// string that is not a known enum variant). Parameters are declared with a fixed type, so such a +/// value has to be rejected when it arrives, or reading the parameter back would panic. +/// +/// Captured as a function pointer at declaration time so that the parameter map can enforce the +/// declared Rust type without knowing what it is. The conversion the type already has to provide +/// *is* the check, so there is nothing for a type to implement beyond [`TryFrom`], and no way for +/// it to accept a value that a later read would then reject. +fn type_check_of(value: &ParameterValue) -> Result<(), String> { + T::try_from(value.clone()) + .map(|_| ()) + .map_err(|err| err.to_string()) +} + struct DeclaredStorage { value: DeclaredValue, kind: ParameterKind, options: ParameterOptionsStorage, + /// Enforces the Rust type the parameter was declared with. The [`kind`](Self::kind) alone + /// is not enough: a value can have the right kind and still be unrepresentable in `T`. + /// See [`type_check_of`]. + type_check: fn(&ParameterValue) -> Result<(), String>, validate: Option, on_change: Option, change_tx: Option>, @@ -552,6 +587,7 @@ impl Debug for DeclaredStorage { .field("value", &self.value) .field("kind", &self.kind) .field("options", &self.options) + .field("type_check", &"..") .field("validate", &self.validate.as_ref().map(|_| "..")) .field("on_change", &self.on_change.as_ref().map(|_| "..")) .field("change_tx", &"..") @@ -622,6 +658,15 @@ impl ParameterMap { == std::mem::discriminant(&value.kind()) || matches!(storage.kind, ParameterKind::Dynamic) { + // The kind matching is not sufficient. The parameter was declared with a + // concrete Rust type, and a value of the right kind can still be + // unrepresentable in it. Reject those here rather than letting a later + // read of the parameter fail. + if let Err(reason) = (storage.type_check)(&value) { + return Err(format!( + "Parameter value is not valid for this parameter's type: {reason}" + )); + } if !storage.options.ranges.in_range(&value) { return Err("Parameter value is out of range".into()); } @@ -946,6 +991,10 @@ pub enum ParameterValueError { OutOfRange, /// Parameter was stored in a static type and an operation on a different type was attempted. TypeMismatch, + /// The value had the right [`ParameterKind`] for this parameter but was not valid for the + /// Rust type it was declared with, e.g. an integer that does not fit the declared integer + /// type or a string that is not a known enum variant. + Invalid(String), /// A write on a read-only parameter was attempted. ReadOnly, /// A custom validation callback rejected the value. @@ -957,6 +1006,10 @@ impl std::fmt::Display for ParameterValueError { 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"), + // The reason is a whole sentence written by the type's own conversion, which reports + // this variant as its error. Callers that go on to add their own context, such as the + // parameter service, would repeat a prefix added here. + ParameterValueError::Invalid(reason) => write!(f, "{reason}"), ParameterValueError::ReadOnly => write!(f, "a write on a read-only parameter was attempted"), ParameterValueError::ValidationFailed(reason) => write!(f, "custom validation rejected the value: {reason}"), } @@ -1034,6 +1087,9 @@ impl Parameters<'_> { /// * `Ok(())` if setting was successful. /// * [`Err(ParameterValueError::TypeMismatch)`] if the type of the requested value is different /// from the parameter's type. + /// * [`Err(ParameterValueError::Invalid)`] if the requested value shares its + /// [`ParameterKind`] with the parameter's type but cannot be represented in it, e.g. + /// setting an `i64` of `70000` on a parameter declared as [`u16`]. /// * [`Err(ParameterValueError::OutOfRange)`] if the requested value is out of the parameter's /// range. /// * [`Err(ParameterValueError::ReadOnly)`] if the parameter is read only. @@ -1054,6 +1110,10 @@ impl Parameters<'_> { ParameterStorage::Declared(param) => { if T::kind() == param.kind { let value = value.into(); + // `T` here is the type of the value being set, which is not + // necessarily the type the parameter was declared with. The two only + // have to share a kind, so enforce the declared type as well. + (param.type_check)(&value).map_err(ParameterValueError::Invalid)?; if !param.options.ranges.in_range(&value) { return Err(ParameterValueError::OutOfRange); } @@ -1197,26 +1257,12 @@ impl ParameterInterface { Ok(selection) } - fn store_parameter( - &self, - name: Arc, - kind: ParameterKind, - value: DeclaredValue, - options: ParameterOptionsStorage, - validate: Option, - change_tx: Option>, - ) { - self.parameter_map.lock().unwrap().storage.insert( - name, - ParameterStorage::Declared(DeclaredStorage { - options, - value, - kind, - validate, - on_change: None, - change_tx, - }), - ); + fn store_parameter(&self, name: Arc, storage: DeclaredStorage) { + self.parameter_map + .lock() + .unwrap() + .storage + .insert(name, ParameterStorage::Declared(storage)); } pub(crate) fn allow_undeclared(&self) { @@ -2230,4 +2276,149 @@ mod tests { param.set(75).unwrap(); 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 for ParameterValue { + fn from(value: Switch) -> Self { + ParameterValue::String( + match value { + Switch::On => "on", + Switch::Off => "off", + } + .into(), + ) + } + } + + impl TryFrom for Switch { + type Error = ParameterValueError; + + fn try_from(value: ParameterValue) -> Result { + 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 rmw_string(value: &str) -> RmwParameterValue { + RmwParameterValue { + type_: ParameterType::PARAMETER_STRING, + string_value: value.into(), + ..Default::default() + } + } + + #[test] + fn test_service_path_rejects_value_of_right_kind_but_wrong_type() { + let node = Context::default() + .create_basic_executor() + .create_node(&format!("param_test_node_{}", line!())) + .unwrap(); + let param: MandatoryParameter = node + .declare_parameter("switch") + .default(Switch::On) + .mandatory() + .unwrap(); + + let map = node.parameter_interface().parameter_map.lock().unwrap(); + + // A valid value is accepted. + assert!(map + .validate_parameter_setting("switch", rmw_string("off")) + .is_ok()); + + // "banana" is a perfectly good string, so the parameter kind matches. It is not a + // Switch, though, and accepting it would make the next `get()` panic. + // + // Asserted in full because this string is what an operator sees come back from + // `ros2 param set`, and because the reason travels through a `Display` that used to + // prepend a duplicate of the context added here. + let err = map + .validate_parameter_setting("switch", rmw_string("banana")) + .unwrap_err(); + assert_eq!( + err, + "Parameter value is not valid for this parameter's type: \ + unknown Switch 'banana', expected one of: on, off" + ); + + // A genuine kind mismatch still reports as one. + let err = map + .validate_parameter_setting( + "switch", + RmwParameterValue { + type_: ParameterType::PARAMETER_INTEGER, + integer_value: 42, + ..Default::default() + }, + ) + .unwrap_err(); + assert!(err.contains("different type"), "unexpected reason: {err}"); + + drop(map); + + // Nothing was applied, and reading the parameter still works. + assert_eq!(param.get(), Switch::On); + } + + #[test] + fn test_undeclared_set_rejects_value_of_right_kind_but_wrong_type() { + let node = Context::default() + .create_basic_executor() + .create_node(&format!("param_test_node_{}", line!())) + .unwrap(); + let param: MandatoryParameter = node + .declare_parameter("switch") + .default(Switch::On) + .mandatory() + .unwrap(); + + // `Parameters::set` only requires the value's kind to match the parameter's, so this + // `Arc` is accepted as far as the kind check goes even though the parameter was + // declared as a `Switch`. + let err = node + .use_undeclared_parameters() + .set::>("switch", "banana".into()) + .unwrap_err(); + assert!( + matches!(err, ParameterValueError::Invalid(_)), + "expected Invalid, got {err:?}" + ); + // The reason reaches the caller once, not wrapped in a restatement of itself. + assert_eq!( + err.to_string(), + "unknown Switch 'banana', expected one of: on, off" + ); + assert_eq!(param.get(), Switch::On); + + // A value that is valid for the declared type goes through. + node.use_undeclared_parameters() + .set::>("switch", "off".into()) + .unwrap(); + assert_eq!(param.get(), Switch::Off); + } } diff --git a/rclrs/src/parameter/value.rs b/rclrs/src/parameter/value.rs index e646b30e..725301aa 100644 --- a/rclrs/src/parameter/value.rs +++ b/rclrs/src/parameter/value.rs @@ -137,7 +137,7 @@ impl From]>> for ParameterValue { /// A trait that describes a value that can be converted into a parameter. pub trait ParameterVariant: - Into + Clone + TryFrom + 'static + Into + Clone + TryFrom + 'static { /// The type used to describe the range of this parameter. type Range: Into + Default + Clone; From 3f27dccdf88eb884e50973baeabae81d9df39859 Mon Sep 17 00:00:00 2001 From: Mathieu David Date: Sat, 25 Jul 2026 15:40:24 +0200 Subject: [PATCH 2/3] feat(parameters): derive descriptor constraints from the parameter type additional_constraints in a parameter descriptor was only ever what the declaration passed to .constraints(). For a type with a closed set of valid values, such as a string-backed enum, that meant every declaration site restating the type's rules, with nothing keeping the text in sync as variants are added -- and an empty descriptor field when the call was forgotten, leaving operators no way to discover what a value may be. ParameterVariant::type_constraints() lets a type describe itself once. A declaration that sets no constraints of its own inherits it, so `ros2 param describe` reports the valid values for free. An explicit .constraints() still takes precedence for rules that belong to one declaration rather than to the type. --- rclrs/src/parameter.rs | 135 +++++++++++++++++++++------------ rclrs/src/parameter/service.rs | 45 ++++++++++- rclrs/src/parameter/value.rs | 13 ++++ 3 files changed, 145 insertions(+), 48 deletions(-) diff --git a/rclrs/src/parameter.rs b/rclrs/src/parameter.rs index 5b5ad601..30db40a1 100644 --- a/rclrs/src/parameter.rs +++ b/rclrs/src/parameter.rs @@ -50,9 +50,17 @@ struct ParameterOptionsStorage { impl From> for ParameterOptionsStorage { fn from(opts: ParameterOptions) -> Self { + // A declaration that says nothing about constraints inherits whatever the parameter's + // type has to say about itself, so that introspection is useful without every + // declaration site restating the type's own rules. + let constraints = if opts.constraints.is_empty() { + T::type_constraints().unwrap_or(opts.constraints) + } else { + opts.constraints + }; Self { description: opts.description, - constraints: opts.constraints, + constraints, ranges: opts.ranges.into(), } } @@ -1270,6 +1278,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 for ParameterValue { + fn from(value: Switch) -> Self { + ParameterValue::String( + match value { + Switch::On => "on", + Switch::Off => "off", + } + .into(), + ) + } + } + + impl TryFrom for Switch { + type Error = ParameterValueError; + + fn try_from(value: ParameterValue) -> Result { + 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> { + Some("one of: on, off".into()) + } + } +} + #[cfg(test)] mod tests { use super::*; @@ -2277,52 +2363,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 for ParameterValue { - fn from(value: Switch) -> Self { - ParameterValue::String( - match value { - Switch::On => "on", - Switch::Off => "off", - } - .into(), - ) - } - } - - impl TryFrom for Switch { - type Error = ParameterValueError; - - fn try_from(value: ParameterValue) -> Result { - 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 { diff --git a/rclrs/src/parameter/service.rs b/rclrs/src/parameter/service.rs index 79ea85e9..2017c8d9 100644 --- a/rclrs/src/parameter/service.rs +++ b/rclrs/src/parameter/service.rs @@ -35,7 +35,7 @@ pub struct ParameterService { set_parameters_atomically_service: Service, } -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 = node + .declare_parameter("inherited") + .default(Switch::On) + .mandatory() + .unwrap(); + // An explicit constraint still wins, for rules specific to this declaration. + let _explicit: MandatoryParameter = 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 = 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"); diff --git a/rclrs/src/parameter/value.rs b/rclrs/src/parameter/value.rs index 725301aa..32218c1a 100644 --- a/rclrs/src/parameter/value.rs +++ b/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> { + None + } } impl TryFrom for bool { From 4f098d45114b152eaab24b467b5a47cc92340299 Mon Sep 17 00:00:00 2001 From: Mathieu David Date: Sat, 25 Jul 2026 15:46:20 +0200 Subject: [PATCH 3/3] feat(parameters): support common Rust types as parameters A parameter could only be declared with the Rust type that most directly represents one of the nine ROS 2 parameter types, so a list of names had to be handled as Arc<[Arc]> and a port number as i64 even where the application would otherwise use Vec and u16. Add ParameterVariant for String, PathBuf, Vec, Vec, Vec, Vec, Vec, f32 and the integer types that fit in an i64 (i8, i16, i32, u8, u16, u32). Conversions back from a stored value can be partial -- not every i64 is a u16 -- which the type check added in the previous commit rejects before it can be stored, so such a parameter can never hold a value of the wrong type. Every scalar type also has a Vec form, using whichever ROS 2 array type holds the scalar's representation, so a Vec is an integer array whose elements are each checked against the range of a u16. Reading an element goes through the scalar type's own conversion, so what an element may be is decided in one place, and a rejection says which element was at fault. Vec is the one exception: a sequence of bytes is a ROS 2 byte array rather than an integer array. Ranges are expressed in the parameter's own type: range = 1024..=49151 on a u16 parameter means what it appears to. A bound the declaration leaves open is filled in from the type's own limits, so the narrowing reaches the descriptor's IntegerRange where `ros2 param describe` and rqt_reconfigure can read it, rather than only being discovered by having a value rejected. DurationSecs and DurationMillis carry a Duration as a double or an integer. Duration itself is not a parameter type, since the unit it would be stored in is not something to leave implicit in a robotics configuration. Both wrappers deref to Duration and report their unit in the descriptor's constraints. u64, usize, i128 and u128 are deliberately unsupported: every way of storing a value above i64::MAX in a ROS 2 parameter silently produces a number the application did not ask for. --- rclrs/src/parameter.rs | 2 + rclrs/src/parameter/service.rs | 74 ++- rclrs/src/parameter/std_types.rs | 765 +++++++++++++++++++++++++++++++ 3 files changed, 840 insertions(+), 1 deletion(-) create mode 100644 rclrs/src/parameter/std_types.rs diff --git a/rclrs/src/parameter.rs b/rclrs/src/parameter.rs index 30db40a1..5b5b0828 100644 --- a/rclrs/src/parameter.rs +++ b/rclrs/src/parameter.rs @@ -1,11 +1,13 @@ mod override_map; mod range; mod service; +mod std_types; mod value; pub(crate) use override_map::*; pub use range::*; use service::*; +pub use std_types::*; pub use value::*; use ros_env::rcl_interfaces::msg::rmw::{ParameterType, ParameterValue as RmwParameterValue}; diff --git a/rclrs/src/parameter/service.rs b/rclrs/src/parameter/service.rs index 2017c8d9..9b7c46f7 100644 --- a/rclrs/src/parameter/service.rs +++ b/rclrs/src/parameter/service.rs @@ -833,6 +833,79 @@ mod tests { Ok(()) } + /// A remote caller cannot put a value into a parameter that the type it was declared with + /// cannot hold, even when the value's ROS 2 type is the right one. Were it able to, the + /// node would panic the next time it read the parameter. + #[test] + fn test_set_parameters_service_enforces_the_declared_rust_type() -> Result<(), RclrsError> { + let mut executor = Context::default().create_basic_executor(); + let node = executor + .create_node(NodeOptions::new("node").namespace("narrow")) + .unwrap(); + let port: MandatoryParameter = node + .declare_parameter("port") + .default(8080) + .mandatory() + .unwrap(); + + let client_node = executor + .create_node(NodeOptions::new("client").namespace("narrow")) + .unwrap(); + let set_client = + client_node.create_client::("/narrow/node/set_parameters")?; + + let set_client_inner = Arc::clone(&set_client); + let ready = client_node + .notify_on_graph_change_with_period(Duration::from_millis(1), move || { + set_client_inner.service_is_ready().unwrap() + }); + executor + .spin(SpinOptions::default().until_promise_resolved(ready)) + .first_error()?; + + let integer = |value: i64| RmwParameter { + name: "port".into(), + value: RmwParameterValue { + type_: ParameterType::PARAMETER_INTEGER, + integer_value: value, + ..Default::default() + }, + }; + let request = SetParameters_Request { + // 70000 is a valid ROS 2 integer, so the kind matches, but it is not a u16. + parameters: seq![integer(70000), integer(-1), integer(9000)], + }; + + let callback_ran = Arc::new(AtomicBool::new(false)); + let callback_ran_inner = Arc::clone(&callback_ran); + let promise = set_client + .call_then(&request, move |response: SetParameters_Response| { + assert_eq!(response.results.len(), 3); + assert!(!response.results[0].successful); + // Asserted in full: this is the text `ros2 param set` prints back at whoever + // made the call, so it is part of the interface rather than a detail. + let reason = response.results[0].reason.to_string(); + assert_eq!( + reason, + "Parameter value is not valid for this parameter's type: \ + 70000 is out of range for u16, which accepts 0..=65535" + ); + assert!(!response.results[1].successful, "negative port"); + assert!(response.results[2].successful, "9000 is a valid u16"); + callback_ran_inner.store(true, Ordering::Release); + }) + .unwrap(); + + executor + .spin(SpinOptions::default().until_promise_resolved(promise)) + .first_error()?; + assert!(callback_ran.load(Ordering::Acquire)); + + // Only the valid value was applied, and reading the parameter does not panic. + assert_eq!(port.get(), 9000); + 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. @@ -870,7 +943,6 @@ mod tests { .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"), ""); diff --git a/rclrs/src/parameter/std_types.rs b/rclrs/src/parameter/std_types.rs new file mode 100644 index 00000000..053a672d --- /dev/null +++ b/rclrs/src/parameter/std_types.rs @@ -0,0 +1,765 @@ +//! [`ParameterVariant`] implementations for common Rust types. +//! +//! ROS 2 has nine parameter types, and [`value`](super::value) implements [`ParameterVariant`] +//! for the Rust type that represents each of them most directly, such as `i64`, `f64`, +//! `Arc` and `Arc<[i64]>`. Those are the right types for the parameter machinery, but they +//! are not always the types an application would otherwise use. +//! +//! This module widens the set of types a parameter can be declared with, so that a `Vec` +//! or a `u16` can be used where the ROS 2 representation happens to be a string array or an +//! integer. Conversions in this direction can be partial: not every `i64` fits a `u16`. Values +//! that do not fit are rejected by the type's [`TryFrom`] before they are ever stored, so a +//! parameter declared as `u16` can never hold something that is not one. +//! +//! Every scalar type here has a `Vec` form, which uses whichever ROS 2 array type holds the +//! scalar's representation. A `Vec` is therefore an integer array whose elements are each +//! checked against the range of a `u16`, and the rejection message says which element was at +//! fault. The one exception is `Vec`, which is a ROS 2 *byte* array rather than an integer +//! array, since that is what ROS 2 has a byte array for. +//! +//! # Unsupported integer types +//! +//! `u64`, `usize`, `i128` and `u128` are deliberately absent. A parameter's value has to be +//! representable in ROS 2, whose integer type is `i64`, and every way of handling a value above +//! `i64::MAX` silently produces a number the application did not ask for, whether by saturating, +//! by wrapping, or by panicking inside an infallible `From`. Use `i64`, or `u32` when the value +//! must be unsigned and small. + +use std::{ops::Deref, path::PathBuf, sync::Arc, time::Duration}; + +use crate::{ + parameter::{ParameterRange, ParameterRanges}, + ParameterKind, ParameterValue, ParameterValueError, ParameterVariant, +}; + +/// Implements the three traits needed to declare a parameter as `$t`, where `$t` maps onto the +/// ROS 2 integer type but holds a narrower range of values. +macro_rules! impl_narrow_integer { + ($($t:ty),* $(,)?) => { $( + impl From<$t> for ParameterValue { + fn from(value: $t) -> Self { + ParameterValue::Integer(value.into()) + } + } + + impl TryFrom for $t { + type Error = ParameterValueError; + + fn try_from(value: ParameterValue) -> Result { + match value { + ParameterValue::Integer(v) => <$t>::try_from(v).map_err(|_| { + ParameterValueError::Invalid(format!( + "{v} is out of range for {}, which accepts {}..={}", + stringify!($t), + <$t>::MIN, + <$t>::MAX, + )) + }), + _ => Err(ParameterValueError::TypeMismatch), + } + } + } + + // Ranges are written in the field's own type, so `range = 1024..=65535` on a `u16` + // means what it appears to. `ParameterRanges` stores the ROS 2 representation. + // + // A bound the declaration leaves open is still bounded by the type, so it is filled in + // from the type's own limits. That puts the narrowing into the descriptor's + // `IntegerRange`, where `ros2 param describe` and rqt_reconfigure can read it, instead + // of leaving them to discover it by having a value rejected. + impl From> for ParameterRanges { + fn from(range: ParameterRange<$t>) -> Self { + ParameterRange:: { + lower: Some(range.lower.map_or(i64::from(<$t>::MIN), i64::from)), + upper: Some(range.upper.map_or(i64::from(<$t>::MAX), i64::from)), + step: range.step.map(i64::from), + } + .into() + } + } + + impl ParameterVariant for $t { + type Range = ParameterRange<$t>; + + fn kind() -> ParameterKind { + ParameterKind::Integer + } + + fn type_constraints() -> Option> { + Some(format!("{}..={}", <$t>::MIN, <$t>::MAX).into()) + } + } + )* }; +} + +impl_narrow_integer!(i8, i16, i32, u8, u16, u32); + +impl From for ParameterValue { + fn from(value: f32) -> Self { + ParameterValue::Double(value.into()) + } +} + +impl TryFrom for f32 { + type Error = ParameterValueError; + + fn try_from(value: ParameterValue) -> Result { + match value { + ParameterValue::Double(v) => { + let narrowed = v as f32; + // Narrowing is lossy in the mantissa, which is inherent to asking for an `f32`, + // but a finite value must not silently become an infinity. + if narrowed.is_finite() || !v.is_finite() { + Ok(narrowed) + } else { + Err(ParameterValueError::Invalid(format!( + "{v} is out of range for f32" + ))) + } + } + _ => Err(ParameterValueError::TypeMismatch), + } + } +} + +impl From> for ParameterRanges { + fn from(range: ParameterRange) -> Self { + ParameterRange:: { + lower: range.lower.map(f64::from), + upper: range.upper.map(f64::from), + step: range.step.map(f64::from), + } + .into() + } +} + +impl ParameterVariant for f32 { + type Range = ParameterRange; + + fn kind() -> ParameterKind { + ParameterKind::Double + } +} + +impl From for ParameterValue { + fn from(value: String) -> Self { + ParameterValue::String(value.into()) + } +} + +impl TryFrom for String { + type Error = ParameterValueError; + + fn try_from(value: ParameterValue) -> Result { + match value { + ParameterValue::String(v) => Ok(v.to_string()), + _ => Err(ParameterValueError::TypeMismatch), + } + } +} + +impl ParameterVariant for String { + type Range = (); + + fn kind() -> ParameterKind { + ParameterKind::String + } +} + +/// Note that a path which is not valid UTF-8 cannot be represented as a ROS 2 string and is +/// converted lossily, since ROS 2 parameter strings are UTF-8. +impl From for ParameterValue { + fn from(value: PathBuf) -> Self { + ParameterValue::String(value.to_string_lossy().as_ref().into()) + } +} + +impl TryFrom for PathBuf { + type Error = ParameterValueError; + + fn try_from(value: ParameterValue) -> Result { + match value { + ParameterValue::String(v) => Ok(PathBuf::from(v.as_ref())), + _ => Err(ParameterValueError::TypeMismatch), + } + } +} + +impl ParameterVariant for PathBuf { + type Range = (); + + fn kind() -> ParameterKind { + ParameterKind::String + } +} + +/// Implements the traits needed to declare a parameter as `Vec<$item>`, for the array kinds +/// whose ROS 2 representation is `Arc<[$item]>`. +macro_rules! impl_vec_of { + ($(($item:ty, $variant:ident, $kind:ident)),* $(,)?) => { $( + impl From> for ParameterValue { + fn from(value: Vec<$item>) -> Self { + ParameterValue::$variant(value.into()) + } + } + + impl TryFrom for Vec<$item> { + type Error = ParameterValueError; + + fn try_from(value: ParameterValue) -> Result { + match value { + ParameterValue::$variant(v) => Ok(v.to_vec()), + _ => Err(ParameterValueError::TypeMismatch), + } + } + } + + impl ParameterVariant for Vec<$item> { + type Range = (); + + fn kind() -> ParameterKind { + ParameterKind::$kind + } + } + )* }; +} + +impl_vec_of!( + (u8, ByteArray, ByteArray), + (bool, BoolArray, BoolArray), + (i64, IntegerArray, IntegerArray), + (f64, DoubleArray, DoubleArray), +); + +/// Reports which element of an array failed to convert, since "out of range for u16" is not much +/// help on its own when the array has forty entries. +fn element_error(index: usize, error: ParameterValueError) -> ParameterValueError { + let reason = match error { + ParameterValueError::Invalid(reason) => reason, + other => other.to_string(), + }; + ParameterValueError::Invalid(format!("element {index}: {reason}")) +} + +/// Implements the traits needed to declare a parameter as `Vec<$item>`, for an item type whose +/// ROS 2 representation is `$scalar` and which therefore lives in the `$array` array type. +/// +/// Reading an element back goes through the item type's own [`TryFrom`], so whatever an element of +/// that type may be is decided in exactly one place, and the error says which element was at fault. +macro_rules! impl_vec_of_convertible { + ($(($item:ty, $array:ident, $scalar:ident, $kind:ident, $to_element:expr)),* $(,)?) => { $( + impl From> for ParameterValue { + fn from(value: Vec<$item>) -> Self { + ParameterValue::$array(value.into_iter().map($to_element).collect()) + } + } + + impl TryFrom for Vec<$item> { + type Error = ParameterValueError; + + fn try_from(value: ParameterValue) -> Result { + match value { + ParameterValue::$array(items) => items + .iter() + .enumerate() + .map(|(index, item)| { + <$item>::try_from(ParameterValue::$scalar(item.clone())) + .map_err(|error| element_error(index, error)) + }) + .collect(), + _ => Err(ParameterValueError::TypeMismatch), + } + } + } + + impl ParameterVariant for Vec<$item> { + type Range = (); + + fn kind() -> ParameterKind { + ParameterKind::$kind + } + + fn type_constraints() -> Option> { + <$item as ParameterVariant>::type_constraints() + .map(|constraints| format!("every element {constraints}").into()) + } + } + )* }; +} + +// `Vec` is deliberately absent: ROS 2 has a byte array type, so a sequence of bytes is that +// rather than an integer array. It is implemented above, alongside the other array types that need +// no element conversion. +impl_vec_of_convertible!( + (i8, IntegerArray, Integer, IntegerArray, i64::from), + (i16, IntegerArray, Integer, IntegerArray, i64::from), + (i32, IntegerArray, Integer, IntegerArray, i64::from), + (u16, IntegerArray, Integer, IntegerArray, i64::from), + (u32, IntegerArray, Integer, IntegerArray, i64::from), + (f32, DoubleArray, Double, DoubleArray, f64::from), + ( + PathBuf, + StringArray, + String, + StringArray, + |path: PathBuf| { Arc::from(path.to_string_lossy().as_ref()) } + ), + ( + DurationSecs, + DoubleArray, + Double, + DoubleArray, + |value: DurationSecs| { value.0.as_secs_f64() } + ), + ( + DurationMillis, + IntegerArray, + Integer, + IntegerArray, + |value: DurationMillis| { whole_millis(value.0) } + ), +); + +impl From> for ParameterValue { + fn from(value: Vec) -> Self { + ParameterValue::StringArray(value.into_iter().map(Arc::from).collect()) + } +} + +impl TryFrom for Vec { + type Error = ParameterValueError; + + fn try_from(value: ParameterValue) -> Result { + match value { + ParameterValue::StringArray(v) => Ok(v.iter().map(|s| s.to_string()).collect()), + _ => Err(ParameterValueError::TypeMismatch), + } + } +} + +impl ParameterVariant for Vec { + type Range = (); + + fn kind() -> ParameterKind { + ParameterKind::StringArray + } +} + +/// A [`Duration`] parameter expressed in seconds, as a ROS 2 double. +/// +/// [`Duration`] itself is not a parameter type, because the unit it would be stored in is not +/// something to leave implicit in a robotics configuration. Choose the unit at the point of +/// declaration by using this or [`DurationMillis`]. Both dereference to [`Duration`], and the +/// unit is reported in the parameter descriptor's constraints. +/// +/// Ranges are expressed in seconds, e.g. `range = 0.0..=5.0`. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct DurationSecs(pub Duration); + +/// A [`Duration`] parameter expressed in whole milliseconds, as a ROS 2 integer. +/// +/// See [`DurationSecs`] for why the unit is part of the type. Sub-millisecond precision is lost. +/// Ranges are expressed in milliseconds, e.g. `range = 0..=5000`. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct DurationMillis(pub Duration); + +macro_rules! impl_duration_wrapper { + ($($t:ident),* $(,)?) => { $( + impl Deref for $t { + type Target = Duration; + + fn deref(&self) -> &Duration { + &self.0 + } + } + + impl From for $t { + fn from(value: Duration) -> Self { + Self(value) + } + } + + impl From<$t> for Duration { + fn from(value: $t) -> Self { + value.0 + } + } + )* }; +} + +impl_duration_wrapper!(DurationSecs, DurationMillis); + +impl From for ParameterValue { + fn from(value: DurationSecs) -> Self { + ParameterValue::Double(value.0.as_secs_f64()) + } +} + +impl TryFrom for DurationSecs { + type Error = ParameterValueError; + + fn try_from(value: ParameterValue) -> Result { + match value { + ParameterValue::Double(v) => { + Duration::try_from_secs_f64(v) + .map(DurationSecs) + .map_err(|_| { + ParameterValueError::Invalid(format!( + "{v} is not a valid duration in seconds; must be finite and not negative" + )) + }) + } + _ => Err(ParameterValueError::TypeMismatch), + } + } +} + +impl ParameterVariant for DurationSecs { + type Range = ParameterRange; + + fn kind() -> ParameterKind { + ParameterKind::Double + } + + fn type_constraints() -> Option> { + Some("a duration in seconds".into()) + } +} + +/// The whole milliseconds of a duration, as a ROS 2 integer. +fn whole_millis(value: Duration) -> i64 { + let millis = value.as_millis(); + debug_assert!( + millis <= i64::MAX as u128, + "duration of {millis} ms cannot be represented as a ROS 2 integer parameter", + ); + i64::try_from(millis).unwrap_or(i64::MAX) +} + +impl From for ParameterValue { + fn from(value: DurationMillis) -> Self { + ParameterValue::Integer(whole_millis(value.0)) + } +} + +impl TryFrom for DurationMillis { + type Error = ParameterValueError; + + fn try_from(value: ParameterValue) -> Result { + match value { + ParameterValue::Integer(v) => u64::try_from(v) + .map(|millis| DurationMillis(Duration::from_millis(millis))) + .map_err(|_| { + ParameterValueError::Invalid(format!( + "{v} is not a valid duration in milliseconds; must not be negative" + )) + }), + _ => Err(ParameterValueError::TypeMismatch), + } + } +} + +impl ParameterVariant for DurationMillis { + type Range = ParameterRange; + + fn kind() -> ParameterKind { + ParameterKind::Integer + } + + fn type_constraints() -> Option> { + Some("a duration in milliseconds".into()) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::*; + + /// Asserts that a value survives a round trip through `ParameterValue`. + fn round_trip(value: T) + where + T: ParameterVariant + PartialEq + std::fmt::Debug, + { + let stored: ParameterValue = value.clone().into(); + let Ok(recovered) = T::try_from(stored) else { + panic!("{value:?} did not convert back from its ParameterValue"); + }; + assert_eq!(recovered, value); + } + + #[test] + fn test_round_trips() { + round_trip(String::from("hello")); + round_trip(PathBuf::from("/dev/ttyUSB0")); + + round_trip(vec![String::from("a"), String::from("b")]); + round_trip(vec![PathBuf::from("/dev/a"), PathBuf::from("/dev/b")]); + round_trip(vec![1i64, 2, 3]); + round_trip(vec![1.5f64, 2.5]); + round_trip(vec![true, false]); + round_trip(vec![1u8, 2, 3]); + round_trip(vec![-5i8, 5]); + round_trip(vec![-5000i16, 5000]); + round_trip(vec![-70000i32, 70000]); + round_trip(vec![0u16, 65535]); + round_trip(vec![0u32, 4_000_000_000]); + round_trip(vec![1.5f32, -2.5]); + round_trip(vec![DurationSecs(Duration::from_millis(250))]); + round_trip(vec![DurationMillis(Duration::from_millis(30))]); + + round_trip(1.5f32); + round_trip(-5i8); + round_trip(-5000i16); + round_trip(-70000i32); + round_trip(200u8); + round_trip(65535u16); + round_trip(4_000_000_000u32); + + round_trip(DurationSecs(Duration::from_millis(1500))); + round_trip(DurationMillis(Duration::from_millis(1500))); + } + + #[test] + fn test_narrow_integers_reject_out_of_range_values() { + // The kind is right and the value is a perfectly good i64, but it is not a u16. + let err = u16::try_from(ParameterValue::Integer(70000)).unwrap_err(); + assert!( + matches!(&err, ParameterValueError::Invalid(reason) + if reason.contains("70000") && reason.contains("0..=65535")), + "unhelpful reason: {err}" + ); + + assert!(u16::try_from(ParameterValue::Integer(-1)).is_err()); + assert!(i8::try_from(ParameterValue::Integer(128)).is_err()); + assert!(u8::try_from(ParameterValue::Integer(256)).is_err()); + assert!(i32::try_from(ParameterValue::Integer(i64::MAX)).is_err()); + + // Boundaries are accepted. + assert_eq!( + u16::try_from(ParameterValue::Integer(65535)).unwrap(), + 65535 + ); + assert_eq!(i8::try_from(ParameterValue::Integer(-128)).unwrap(), -128); + } + + #[test] + fn test_f32_rejects_values_it_cannot_represent() { + assert!(f32::try_from(ParameterValue::Double(1e300)).is_err()); + assert!(f32::try_from(ParameterValue::Double(-1e300)).is_err()); + + // Non-finite values pass through unchanged rather than being rejected as out of range. + assert!(f32::try_from(ParameterValue::Double(f64::INFINITY)) + .unwrap() + .is_infinite()); + assert!(f32::try_from(ParameterValue::Double(f64::NAN)) + .unwrap() + .is_nan()); + } + + #[test] + fn test_durations_reject_invalid_values() { + assert!(DurationSecs::try_from(ParameterValue::Double(-1.0)).is_err()); + assert!(DurationSecs::try_from(ParameterValue::Double(f64::NAN)).is_err()); + assert!(DurationMillis::try_from(ParameterValue::Integer(-1)).is_err()); + } + + /// An array of a narrower type holds the same values as the scalar type does, and says which + /// element is at fault when one of them does not fit. + #[test] + fn test_arrays_check_every_element() { + let too_big = ParameterValue::IntegerArray(vec![1, 2, 70000].into()); + let err = Vec::::try_from(too_big).unwrap_err(); + assert!( + matches!(&err, ParameterValueError::Invalid(reason) + if reason.contains("element 2") && reason.contains("0..=65535")), + "the reason should name the element: {err}" + ); + + assert!(Vec::::try_from(ParameterValue::IntegerArray(vec![-1].into())).is_err()); + assert!(Vec::::try_from(ParameterValue::DoubleArray(vec![1e300].into())).is_err()); + assert!( + Vec::::try_from(ParameterValue::IntegerArray(vec![-1].into())).is_err() + ); + + // An array of the wrong ROS 2 type is a different problem from a bad element. + assert!(matches!( + Vec::::try_from(ParameterValue::DoubleArray(vec![1.0].into())), + Err(ParameterValueError::TypeMismatch) + )); + } + + /// A sequence of bytes is a ROS 2 byte array, not an integer array, which is why `Vec` is + /// the one array type that does not follow its scalar. + #[test] + fn test_byte_arrays_are_not_integer_arrays() { + assert_eq!(Vec::::kind(), ParameterKind::ByteArray); + assert_eq!(Vec::::kind(), ParameterKind::IntegerArray); + assert_eq!(u8::kind(), ParameterKind::Integer); + } + + #[test] + fn test_arrays_report_the_constraints_of_their_elements() { + assert_eq!( + Vec::::type_constraints().as_deref(), + Some("every element 0..=65535") + ); + assert_eq!( + Vec::::type_constraints().as_deref(), + Some("every element a duration in milliseconds") + ); + // An element type with nothing to say about itself leaves the array with nothing either. + assert_eq!(Vec::::type_constraints(), None); + } + + #[test] + fn test_declaring_parameters_with_ergonomic_types() { + let node = Context::default() + .create_basic_executor() + .create_node("std_types") + .unwrap(); + + let name: MandatoryParameter = node + .declare_parameter("name") + .default("robot".to_string()) + .mandatory() + .unwrap(); + assert_eq!(name.get(), "robot"); + + let wheels: MandatoryParameter> = node + .declare_parameter("wheels") + .default(vec!["left".to_string(), "right".to_string()]) + .mandatory() + .unwrap(); + assert_eq!(wheels.get(), vec!["left", "right"]); + + let device: MandatoryParameter = node + .declare_parameter("device") + .default(PathBuf::from("/dev/ttyUSB0")) + .mandatory() + .unwrap(); + assert_eq!(device.get(), PathBuf::from("/dev/ttyUSB0")); + + let timeout: MandatoryParameter = node + .declare_parameter("timeout") + .default(DurationSecs(Duration::from_millis(2500))) + .mandatory() + .unwrap(); + assert_eq!(timeout.get().as_millis(), 2500); + + let ports: MandatoryParameter> = node + .declare_parameter("ports") + .default(vec![8080, 9090]) + .mandatory() + .unwrap(); + assert_eq!(ports.get(), vec![8080, 9090]); + } + + /// A `Vec` parameter cannot be made to hold an integer array that is not one, even by a + /// caller going through the untyped interface with a well-formed array of `i64`. + #[test] + fn test_narrow_arrays_cannot_be_corrupted() { + let node = Context::default() + .create_basic_executor() + .create_node("std_types_arrays") + .unwrap(); + + let ports: MandatoryParameter> = node + .declare_parameter("ports") + .default(vec![8080u16]) + .mandatory() + .unwrap(); + + let err = node + .use_undeclared_parameters() + .set::>("ports", vec![1, 70000]) + .unwrap_err(); + assert!(matches!(err, ParameterValueError::Invalid(_)), "{err}"); + assert_eq!(ports.get(), vec![8080]); + } + + /// Ranges on a narrow integer parameter are expressed in that integer's own type, and are + /// enforced against the ROS 2 representation. + #[test] + fn test_narrow_integer_ranges() { + let node = Context::default() + .create_basic_executor() + .create_node("std_types_ranges") + .unwrap(); + + let port: MandatoryParameter = node + .declare_parameter("port") + .default(8080) + .range(ParameterRange { + lower: Some(1024), + upper: Some(49151), + step: None, + }) + .mandatory() + .unwrap(); + + assert_eq!(port.get(), 8080); + assert!(port.set(80u16).is_err(), "below the range"); + assert!(port.set(50000u16).is_err(), "above the range"); + port.set(9000u16).unwrap(); + assert_eq!(port.get(), 9000); + } + + /// The narrowing is reported as the descriptor's `IntegerRange`, which is what rqt and + /// `ros2 param describe` read, and not only as free-text constraints. + #[test] + fn test_narrow_integers_report_their_range_in_the_descriptor() { + use crate::parameter::test_support::parameter_descriptor; + + let node = Context::default() + .create_basic_executor() + .create_node("std_types_descriptor") + .unwrap(); + + // No range on the declaration: the type is the only thing that bounds the value. + let _implied: MandatoryParameter = node + .declare_parameter("implied") + .default(8080) + .mandatory() + .unwrap(); + // A half-open range keeps its declared bound and takes the type's for the other end. + let _half_open: MandatoryParameter = node + .declare_parameter("half_open") + .default(8080) + .range(ParameterRange { + lower: Some(1024), + upper: None, + step: None, + }) + .mandatory() + .unwrap(); + let _signed: MandatoryParameter = node + .declare_parameter("signed") + .default(0) + .mandatory() + .unwrap(); + + let bounds = |name| { + let descriptor = parameter_descriptor(&node, name); + let range = descriptor + .integer_range + .first() + .expect("a narrow integer is always bounded, so it always has a range"); + (range.from_value, range.to_value) + }; + assert_eq!(bounds("implied"), (0, 65535)); + assert_eq!(bounds("half_open"), (1024, 65535)); + assert_eq!(bounds("signed"), (-128, 127)); + + // An `i64` is bounded by nothing narrower than the ROS 2 type, so it still reports no + // range unless the declaration asked for one. + let _plain: MandatoryParameter = node + .declare_parameter("plain") + .default(0) + .mandatory() + .unwrap(); + assert!(parameter_descriptor(&node, "plain") + .integer_range + .is_empty()); + } +}