From ff8db14d0dbad499643591e0576e1d00a497ac37 Mon Sep 17 00:00:00 2001 From: carter Date: Sun, 17 May 2026 15:52:35 -0600 Subject: [PATCH 1/4] fix string constants --- roslibrust_codegen/src/gen.rs | 43 +++++-- roslibrust_codegen/src/lib.rs | 3 +- roslibrust_codegen/src/parse/mod.rs | 115 +++++++++++++++++- roslibrust_codegen/src/parse/msg.rs | 4 +- .../test_package/msg/StringWithHash.msg | 17 +++ roslibrust_test/src/ros1.rs | 2 +- 6 files changed, 167 insertions(+), 17 deletions(-) create mode 100644 roslibrust_codegen/test_package/msg/StringWithHash.msg diff --git a/roslibrust_codegen/src/gen.rs b/roslibrust_codegen/src/gen.rs index ce358e10..8d856afb 100644 --- a/roslibrust_codegen/src/gen.rs +++ b/roslibrust_codegen/src/gen.rs @@ -325,24 +325,15 @@ fn generate_constant_field_definition( version: RosVersion, ) -> Result { let constant_name = format_ident!("r#{}", constant.constant_name); - let constant_rust_type = convert_ros_type_to_rust_type(version, &constant.constant_type) - .ok_or(Error::new(format!( - "A constant was detected {constant:?} for which no valid rust type was found." - )))?; - let constant_rust_type = if constant_rust_type == "::std::string::String" { - String::from("&'static str") - } else { - // Oof it's ugly in here - constant_rust_type.to_owned() - }; - let constant_rust_type = TokenStream::from_str(constant_rust_type.as_str()).map_err(|err| { + let constant_rust_type = convert_ros_constant_type_to_rust_type(version, &constant)?; + let constant_rust_type = TokenStream::from_str(constant_rust_type).map_err(|err| { Error::with( format!("Failed to parse {constant_rust_type} into valid rust syntax").as_str(), err, ) })?; let constant_value = ros_literal_to_rust_literal( - &constant.constant_type, + ros_type_for_constant_literal(&constant.constant_type), &constant.constant_value, &ArrayType::NotArray, version, @@ -350,6 +341,34 @@ fn generate_constant_field_definition( Ok(quote! { pub const #constant_name: #constant_rust_type = #constant_value; }) } +fn convert_ros_constant_type_to_rust_type( + version: RosVersion, + constant: &ConstantInfo, +) -> Result<&'static str, Error> { + if constant.constant_type == "&'static str" { + return Ok("&'static str"); + } + + convert_ros_type_to_rust_type(version, &constant.constant_type) + .map(|rust_type| { + if rust_type == "::std::string::String" { + "&'static str" + } else { + rust_type + } + }) + .ok_or(Error::new(format!( + "A constant was detected {constant:?} for which no valid rust type was found." + ))) +} + +fn ros_type_for_constant_literal(constant_type: &str) -> &str { + match constant_type { + "&'static str" => "string", + _ => constant_type, + } +} + pub fn generate_mod( pkg_name: String, struct_definitions: Vec, diff --git a/roslibrust_codegen/src/lib.rs b/roslibrust_codegen/src/lib.rs index a6a49146..e23da09a 100644 --- a/roslibrust_codegen/src/lib.rs +++ b/roslibrust_codegen/src/lib.rs @@ -506,11 +506,12 @@ impl FieldInfo { } /// Describes all information for a constant within a message -/// Note: Constants are not fully supported yet (waiting on codegen support) #[derive(Clone, Debug)] pub struct ConstantInfo { pub constant_type: String, + // Exact ros file format pub constant_name: String, + // Literal string contents of the rosmessage from the file pub constant_value: RosLiteral, } diff --git a/roslibrust_codegen/src/parse/mod.rs b/roslibrust_codegen/src/parse/mod.rs index 1ac4a1ae..1501eb37 100644 --- a/roslibrust_codegen/src/parse/mod.rs +++ b/roslibrust_codegen/src/parse/mod.rs @@ -126,7 +126,7 @@ fn parse_constant_field(line: &str, pkg: &Package) -> Result Result &str { if let Some(token) = line.find('#') { return &line[..token]; @@ -146,6 +148,21 @@ fn strip_comments(line: &str) -> &str { line } +/// Strips comments from a line, but respects string constant values. +/// For string constants, # characters within the value are NOT treated as comments per ROS spec. +fn strip_comments_respecting_string_constants(line: &str) -> &str { + let trimmed = line.trim_start(); + + // Check if this is a string/wstring constant (has both type declaration and = sign) + if (trimmed.starts_with("string ") || trimmed.starts_with("wstring ")) && line.contains('=') { + // This is a string constant - don't strip anything + return line; + } + + // For everything else (fields, non-string constants, comments), strip normally + strip_comments(line) +} + fn parse_field_type( type_str: &str, array_info: ArrayType, @@ -299,4 +316,100 @@ mod test { let parsed = parse_type(line, &pkg).unwrap(); assert_eq!(parsed.array_info, ArrayType::Unbounded); } + + #[test_log::test] + fn parse_constant_with_hash_in_value() { + use crate::parse::parse_constant_field; + + let pkg = Package { + name: "test_pkg".to_string(), + path: "./not_a_path".into(), + version: Some(RosVersion::ROS1), + }; + + // Test parsing a constant with # in the value + let line = "string HASH_IN_VALUE=foo # bar"; + let constant = parse_constant_field(line, &pkg).unwrap(); + + assert_eq!(constant.constant_name, "HASH_IN_VALUE"); + assert_eq!(constant.constant_type, "&'static str"); + // This should be "foo # bar" according to ROS spec + assert_eq!(constant.constant_value.inner, "foo # bar"); + } + + #[test_log::test] + fn parse_message_with_hash_in_string_constant() { + use crate::parse::parse_ros_message_file; + use std::path::Path; + + let pkg = Package { + name: "test_pkg".to_string(), + path: "./not_a_path".into(), + version: Some(RosVersion::ROS1), + }; + + let msg_content = r#"# Test message +string HASH_IN_VALUE=foo # bar +string NO_SPACES=test#value +string HASH_AT_START=#start +string HASH_AT_END=end# +string data +"#; + + let parsed = + parse_ros_message_file(msg_content, "TestMsg", &pkg, Path::new("test.msg")).unwrap(); + + assert_eq!(parsed.constants.len(), 4); + assert_eq!(parsed.fields.len(), 1); + + // Check all constants have correct values with # characters + assert_eq!(parsed.constants[0].constant_name, "HASH_IN_VALUE"); + assert_eq!(parsed.constants[0].constant_value.inner, "foo # bar"); + + assert_eq!(parsed.constants[1].constant_name, "NO_SPACES"); + assert_eq!(parsed.constants[1].constant_value.inner, "test#value"); + + assert_eq!(parsed.constants[2].constant_name, "HASH_AT_START"); + assert_eq!(parsed.constants[2].constant_value.inner, "#start"); + + assert_eq!(parsed.constants[3].constant_name, "HASH_AT_END"); + assert_eq!(parsed.constants[3].constant_value.inner, "end#"); + + // Check field is parsed correctly + assert_eq!(parsed.fields[0].field_name, "data"); + } + + #[test_log::test] + fn parse_non_string_constants_with_comments() { + use crate::parse::parse_ros_message_file; + use std::path::Path; + + let pkg = Package { + name: "test_pkg".to_string(), + path: "./not_a_path".into(), + version: Some(RosVersion::ROS1), + }; + + // Test that non-string constants still have comments stripped correctly + let msg_content = r#"# Test message +int32 INT_CONST=42 # this is a comment +float32 FLOAT_CONST=3.14 # another comment +bool BOOL_CONST=true # yet another comment +"#; + + let parsed = + parse_ros_message_file(msg_content, "TestMsg", &pkg, Path::new("test.msg")).unwrap(); + + assert_eq!(parsed.constants.len(), 3); + + // Non-string constants should have comments stripped + assert_eq!(parsed.constants[0].constant_name, "INT_CONST"); + assert_eq!(parsed.constants[0].constant_value.inner, "42"); + + assert_eq!(parsed.constants[1].constant_name, "FLOAT_CONST"); + assert_eq!(parsed.constants[1].constant_value.inner, "3.14"); + + assert_eq!(parsed.constants[2].constant_name, "BOOL_CONST"); + assert_eq!(parsed.constants[2].constant_value.inner, "true"); + } } diff --git a/roslibrust_codegen/src/parse/msg.rs b/roslibrust_codegen/src/parse/msg.rs index 85a10bbb..42f19545 100644 --- a/roslibrust_codegen/src/parse/msg.rs +++ b/roslibrust_codegen/src/parse/msg.rs @@ -1,4 +1,4 @@ -use crate::parse::{parse_constant_field, parse_field, strip_comments}; +use crate::parse::{parse_constant_field, parse_field, strip_comments_respecting_string_constants}; use crate::Error; use crate::{ConstantInfo, FieldInfo, Package, RosVersion}; use std::path::{Path, PathBuf}; @@ -69,7 +69,7 @@ pub fn parse_ros_message_file( let mut constants = vec![]; for line in data.lines() { - let line = strip_comments(line).trim(); + let line = strip_comments_respecting_string_constants(line).trim(); if line.is_empty() { // Comment only line skip continue; diff --git a/roslibrust_codegen/test_package/msg/StringWithHash.msg b/roslibrust_codegen/test_package/msg/StringWithHash.msg new file mode 100644 index 00000000..e0111943 --- /dev/null +++ b/roslibrust_codegen/test_package/msg/StringWithHash.msg @@ -0,0 +1,17 @@ +# Test message for string constants containing # characters +# According to ROS spec, # inside string constant values should NOT be treated as comment + +# This should have value "foo # bar" +string HASH_IN_VALUE=foo # bar + +# This should have value "test#value" +string NO_SPACES=test#value + +# This should have value "#start" +string HASH_AT_START=#start + +# This should have value "end#" +string HASH_AT_END=end# + +# Regular field +string data diff --git a/roslibrust_test/src/ros1.rs b/roslibrust_test/src/ros1.rs index 62d86ac3..fdc2c554 100644 --- a/roslibrust_test/src/ros1.rs +++ b/roslibrust_test/src/ros1.rs @@ -12803,7 +12803,7 @@ string frame_id"####; pub struct Constants {} impl ::roslibrust::RosMessageType for Constants { const ROS_TYPE_NAME: &'static str = "test_msgs/Constants"; - const MD5SUM: &'static str = "027df5f26b72c57b1e40902038ca3eec"; + const MD5SUM: &'static str = "ae9eec64f72349c84286c50355b978d8"; const DEFINITION: &'static str = r####"string TEST_STR="/topic" string TEST_STR_2 = '/topic_2' # Apparently unquoted strings are also valid? From 231717dc66111d47486a6c6ead6b5fe7d141ea4f Mon Sep 17 00:00:00 2001 From: carter Date: Sun, 17 May 2026 15:59:49 -0600 Subject: [PATCH 2/4] Code health cleanups around parsing logic --- roslibrust_codegen/src/gen.rs | 22 ++++++ roslibrust_codegen/src/lib.rs | 5 +- roslibrust_codegen/src/parse/mod.rs | 113 ++++++++++++++++++---------- roslibrust_codegen/src/parse/msg.rs | 19 +++-- 4 files changed, 106 insertions(+), 53 deletions(-) diff --git a/roslibrust_codegen/src/gen.rs b/roslibrust_codegen/src/gen.rs index 8d856afb..1aa5c12e 100644 --- a/roslibrust_codegen/src/gen.rs +++ b/roslibrust_codegen/src/gen.rs @@ -498,3 +498,25 @@ fn parse_ros_value( } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test_log::test] + fn string_constants_generate_static_str_type() { + let constant = ConstantInfo { + constant_type: "string".to_owned(), + constant_name: "RUNNING_STATE".to_owned(), + constant_value: RosLiteral::from("RUNNING".to_owned()), + }; + + let generated = generate_constant_field_definition(constant, RosVersion::ROS1) + .unwrap() + .to_string(); + + assert!(generated.contains("RUNNING_STATE")); + assert!(generated.contains("&'static str")); + assert!(generated.contains("= \"RUNNING\"")); + } +} diff --git a/roslibrust_codegen/src/lib.rs b/roslibrust_codegen/src/lib.rs index e23da09a..0e96429d 100644 --- a/roslibrust_codegen/src/lib.rs +++ b/roslibrust_codegen/src/lib.rs @@ -508,10 +508,11 @@ impl FieldInfo { /// Describes all information for a constant within a message #[derive(Clone, Debug)] pub struct ConstantInfo { + /// ROS type name as written in the message definition. pub constant_type: String, - // Exact ros file format + /// Constant name as written in the message definition. pub constant_name: String, - // Literal string contents of the rosmessage from the file + /// Literal value contents from the message definition. pub constant_value: RosLiteral, } diff --git a/roslibrust_codegen/src/parse/mod.rs b/roslibrust_codegen/src/parse/mod.rs index 1501eb37..e3555c34 100644 --- a/roslibrust_codegen/src/parse/mod.rs +++ b/roslibrust_codegen/src/parse/mod.rs @@ -1,6 +1,6 @@ use crate::utils::{Package, RosVersion}; use crate::{bail, ArrayType, Error}; -use crate::{ConstantInfo, FieldInfo, FieldType}; +use crate::{ConstantInfo, FieldInfo, FieldType, RosLiteral}; use std::collections::HashMap; mod action; @@ -75,35 +75,17 @@ pub fn convert_ros_type_to_rust_type(version: RosVersion, ros_type: &str) -> Opt } fn parse_field(line: &str, pkg: &Package, msg_name: &str) -> Result { - let mut splitter = line.split_whitespace(); let pkg_name = pkg.name.as_str(); - let field_type = splitter.next().ok_or(Error::new(format!( - "Did not find field_type on line: {line} while parsing {pkg_name}/{msg_name}" + let (field_type, remainder) = split_type_from_line(line).ok_or(Error::new(format!( + "Did not find field_type and field_name on line: {line} while parsing {pkg_name}/{msg_name}" )))?; let field_type = parse_type(field_type, pkg)?; - let field_name = splitter.next().ok_or(Error::new(format!( - "Did not find field_name on line: {line} while parsing {pkg_name}/{msg_name}" - )))?; + let (field_name, default) = parse_field_name_and_default(remainder).ok_or(Error::new( + format!("Did not find field_name on line: {line} while parsing {pkg_name}/{msg_name}"), + ))?; - let sep = line.find(' ').unwrap(); - // Determine if there is a default value for this field let default = if matches!(pkg.version, Some(RosVersion::ROS2)) { - // For ros2 packages only, check if there is a default value - let line_after_sep = line[sep + 1..].trim(); - match line_after_sep.find(' ') { - Some(def_start) => { - let remainder = line_after_sep[def_start..].trim(); - if remainder.is_empty() { - None - } else { - Some(remainder.to_owned().into()) - } - } - None => { - // No extra space separator found, not default was provided - None - } - } + default.map(RosLiteral::from) } else { None }; @@ -116,21 +98,16 @@ fn parse_field(line: &str, pkg: &Package, msg_name: &str) -> Result Result { - let sep = line.find(' ').ok_or( - Error::new(format!("Failed to find white space seperator ' ' while parsing constant information one line {line} for package {pkg:?}")) - )?; - let equal_after_sep = line[sep..].find('=').ok_or( + let (constant_type, remainder) = split_type_from_line(line).ok_or(Error::new(format!( + "Failed to find white space separator while parsing constant information on line {line} for package {pkg:?}" + )))?; + let equal = remainder.find('=').ok_or( Error::new(format!("Failed to find expected '=' while parsing constant information on line {line} for package {pkg:?}")) )?; - let mut constant_type = parse_type(line[..sep].trim(), pkg)?.field_type; - let constant_name = line[sep + 1..(equal_after_sep + sep)].trim().to_string(); - - // Handle the fact that string type should be different for constants than fields - if constant_type == "string" { - constant_type = "&'static str".to_string(); - } + let constant_type = parse_type(constant_type, pkg)?.field_type; + let constant_name = remainder[..equal].trim().to_string(); + let constant_value = remainder[equal + 1..].trim().to_string(); - let constant_value = line[sep + equal_after_sep + 1..].trim().to_string(); Ok(ConstantInfo { constant_type, constant_name, @@ -138,6 +115,29 @@ fn parse_constant_field(line: &str, pkg: &Package) -> Result Option<(&str, &str)> { + let type_end = line.find(char::is_whitespace)?; + let remainder = line[type_end..].trim_start(); + if remainder.is_empty() { + return None; + } + Some((&line[..type_end], remainder)) +} + +fn parse_field_name_and_default(remainder: &str) -> Option<(&str, Option)> { + match remainder.find(char::is_whitespace) { + Some(name_end) => { + let default = remainder[name_end..].trim(); + if default.is_empty() { + Some((&remainder[..name_end], None)) + } else { + Some((&remainder[..name_end], Some(default.to_owned()))) + } + } + None => Some((remainder, None)), + } +} + /// Looks for # comment character and sub-slices for characters preceding it /// Note: This should NOT be used for lines that contain string constants, /// as # characters within string constant values are part of the value. @@ -153,9 +153,7 @@ fn strip_comments(line: &str) -> &str { fn strip_comments_respecting_string_constants(line: &str) -> &str { let trimmed = line.trim_start(); - // Check if this is a string/wstring constant (has both type declaration and = sign) - if (trimmed.starts_with("string ") || trimmed.starts_with("wstring ")) && line.contains('=') { - // This is a string constant - don't strip anything + if is_string_constant_line(trimmed) { return line; } @@ -163,6 +161,17 @@ fn strip_comments_respecting_string_constants(line: &str) -> &str { strip_comments(line) } +fn is_string_constant_line(line: &str) -> bool { + let Some((type_name, remainder)) = split_type_from_line(line) else { + return false; + }; + (type_name == "string" + || type_name == "wstring" + || type_name.starts_with("string<=") + || type_name.starts_with("wstring<=")) + && remainder.contains('=') +} + fn parse_field_type( type_str: &str, array_info: ArrayType, @@ -332,11 +341,33 @@ mod test { let constant = parse_constant_field(line, &pkg).unwrap(); assert_eq!(constant.constant_name, "HASH_IN_VALUE"); - assert_eq!(constant.constant_type, "&'static str"); + assert_eq!(constant.constant_type, "string"); // This should be "foo # bar" according to ROS spec assert_eq!(constant.constant_value.inner, "foo # bar"); } + #[test_log::test] + fn parse_bounded_string_constant_with_hash_in_value() { + use crate::parse::parse_ros_message_file; + use std::path::Path; + + let pkg = Package { + name: "test_pkg".to_string(), + path: "./not_a_path".into(), + version: Some(RosVersion::ROS2), + }; + + let msg_content = r#"string<=32 HASH_IN_VALUE='foo # bar' +string data +"#; + + let parsed = + parse_ros_message_file(msg_content, "TestMsg", &pkg, Path::new("test.msg")).unwrap(); + + assert_eq!(parsed.constants[0].constant_type, "string"); + assert_eq!(parsed.constants[0].constant_value.inner, "'foo # bar'"); + } + #[test_log::test] fn parse_message_with_hash_in_string_constant() { use crate::parse::parse_ros_message_file; diff --git a/roslibrust_codegen/src/parse/msg.rs b/roslibrust_codegen/src/parse/msg.rs index 42f19545..02f14430 100644 --- a/roslibrust_codegen/src/parse/msg.rs +++ b/roslibrust_codegen/src/parse/msg.rs @@ -1,4 +1,7 @@ -use crate::parse::{parse_constant_field, parse_field, strip_comments_respecting_string_constants}; +use crate::parse::{ + parse_constant_field, parse_field, split_type_from_line, + strip_comments_respecting_string_constants, +}; use crate::Error; use crate::{ConstantInfo, FieldInfo, Package, RosVersion}; use std::path::{Path, PathBuf}; @@ -74,15 +77,11 @@ pub fn parse_ros_message_file( // Comment only line skip continue; } - // Determine if we're looking at a constant or a field - let sep = line.find(' ').ok_or( - Error::new( - format!("Found an invalid ros field line, no space delinting type from name: {line} in {}\n{data}", - path.display()) - ) - )?; - let equal_after_sep = line[sep..].find('='); - if equal_after_sep.is_some() { + let (_, remainder) = split_type_from_line(line).ok_or(Error::new(format!( + "Found an invalid ros field line, no space delimiting type from name: {line} in {}\n{data}", + path.display() + )))?; + if remainder.contains('=') { // Since we found an equal sign after a space, this must be a constant constants.push(parse_constant_field(line, package)?) } else { From 60611d25a487186b4158ac642745d1b26b2aa400 Mon Sep 17 00:00:00 2001 From: carter Date: Sun, 17 May 2026 16:07:06 -0600 Subject: [PATCH 3/4] Fix updated hash (old was confirmed incorrect) --- roslibrust_test/src/ros1.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/roslibrust_test/src/ros1.rs b/roslibrust_test/src/ros1.rs index fdc2c554..62d86ac3 100644 --- a/roslibrust_test/src/ros1.rs +++ b/roslibrust_test/src/ros1.rs @@ -12803,7 +12803,7 @@ string frame_id"####; pub struct Constants {} impl ::roslibrust::RosMessageType for Constants { const ROS_TYPE_NAME: &'static str = "test_msgs/Constants"; - const MD5SUM: &'static str = "ae9eec64f72349c84286c50355b978d8"; + const MD5SUM: &'static str = "027df5f26b72c57b1e40902038ca3eec"; const DEFINITION: &'static str = r####"string TEST_STR="/topic" string TEST_STR_2 = '/topic_2' # Apparently unquoted strings are also valid? From bf5599f16b30d44023be9f4a4acd1fb5b60e04e2 Mon Sep 17 00:00:00 2001 From: carter Date: Sun, 17 May 2026 16:09:07 -0600 Subject: [PATCH 4/4] update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 58ae375c..189eaac5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - @DomWilliamsEE fix ros1 subscribers not automatically unsubscribing from the master when dropped. +- Incorrect handling of string constants in ros message files around comments. ### Changed