Fix string constants issue

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
 

roslibrust_codegen/src/gen.rs

@@ -325,31 +325,50 @@ fn generate_constant_field_definition(
     version: RosVersion,
 ) -> Result<TokenStream, Error> {
     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,
     )?;
     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<TokenStream>,
@@ -479,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\""));
+    }
+}

roslibrust_codegen/src/lib.rs

@@ -506,11 +506,13 @@ 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 {
+    /// ROS type name as written in the message definition.
     pub constant_type: String,
+    /// Constant name as written in the message definition.
     pub constant_name: String,
+    /// Literal value contents from the message definition.
     pub constant_value: RosLiteral,
 }
 

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<FieldInfo, Error> {
-    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,36 +98,80 @@ fn parse_field(line: &str, pkg: &Package, msg_name: &str) -> Result<FieldInfo, E
 }
 
 fn parse_constant_field(line: &str, pkg: &Package) -> Result<ConstantInfo, Error> {
-    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,
         constant_value: constant_value.into(),
     })
 }
 
+fn split_type_from_line(line: &str) -> 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<String>)> {
+    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.
 fn strip_comments(line: &str) -> &str {
     if let Some(token) = line.find('#') {
         return &line[..token];
     }
     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();
+
+    if is_string_constant_line(trimmed) {
+        return line;
+    }
+
+    // For everything else (fields, non-string constants, comments), strip normally
+    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,
@@ -299,4 +325,122 @@ 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, "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;
+        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");
+    }
 }