Extract message path from codegen

Cargo.toml

@@ -5,6 +5,7 @@ members = [
     "rosrust_msg",
     "rosrust_actionlib",
     "rosrust_diagnostics",
+    "ros_msg_parser",
     "examples",
 ]
 
@@ -14,4 +15,5 @@ members = [
 "rosrust_msg" = { path = "rosrust_msg" }
 "rosrust_actionlib" = { path = "rosrust_actionlib" }
 "rosrust_diagnostics" = { path = "rosrust_diagnostics" }
+"ros_msg_parser" = { path = "ros_msg_parser" }
 "examples" = { path = "examples" }

ros_msg_parser/Cargo.toml

@@ -0,0 +1,13 @@
+[package]
+edition = "2018"
+authors = ["Adnan Ademovic <adnanademovic100@gmail.com>"]
+description = "MSG parser for ROS"
+license = "MIT"
+name = "ros_msg_parser"
+repository = "https://github.com/adnanademovic/rosrust"
+version = "0.1.0"
+
+[dependencies]
+lazy_static = "1.4.0"
+regex = "1.5.4"
+thiserror = "1.0.26"

ros_msg_parser/src/error.rs

@@ -0,0 +1,7 @@
+#[derive(thiserror::Error, Debug)]
+pub enum Error {
+    #[error("message path `{name}` is invalid, {reason}")]
+    InvalidMessagePath { name: String, reason: String },
+}
+
+pub type Result<T> = std::result::Result<T, Error>;

ros_msg_parser/src/lib.rs

@@ -0,0 +1,5 @@
+mod error;
+mod message_path;
+
+pub use error::{Error, Result};
+pub use message_path::MessagePath;

rosrust_codegen/src/message_path.rs → ros_msg_parser/src/message_path.rs

@@ -1,5 +1,4 @@
-use crate::error::{Error, ErrorKind, Result};
-use error_chain::bail;
+use crate::{Error, Result};
 use lazy_static::lazy_static;
 use regex::Regex;
 use std::convert::TryFrom;
@@ -8,55 +7,63 @@ use std::hash::Hash;
 
 #[derive(Clone, Debug, Hash, PartialEq, Eq)]
 pub struct MessagePath {
-    pub package: String,
-    pub name: String,
+    package: String,
+    name: String,
+}
+
+fn is_valid_package_name(package: &str) -> bool {
+    lazy_static! {
+        static ref RE_PACKAGE_CORRECT_CHAR_SET_AND_LENGTH: Regex =
+            Regex::new("^[a-z][a-z0-9_]+$").unwrap();
+        static ref RE_PACKAGE_CONSECUTIVE_UNDERSCORE: Regex = Regex::new("__").unwrap();
+    }
+    RE_PACKAGE_CORRECT_CHAR_SET_AND_LENGTH.is_match(package)
+        && !RE_PACKAGE_CONSECUTIVE_UNDERSCORE.is_match(package)
 }
 
 impl MessagePath {
-    pub fn new(package: impl Into<String>, name: impl Into<String>) -> Self {
-        Self {
-            package: package.into(),
-            name: name.into(),
+    /// Create full message path, with naming rules checked
+    ///
+    /// Naming rules are based on [REP 144](https://www.ros.org/reps/rep-0144.html).
+    pub fn new(package: impl Into<String>, name: impl Into<String>) -> Result<Self> {
+        let package = package.into();
+        let name = name.into();
+        if !is_valid_package_name(&package) {
+            return Err(Error::InvalidMessagePath  {
+                name: format!("{}/{}",package,name),
+                  reason: "package name needs to follow REP 144 rules (https://www.ros.org/reps/rep-0144.html)".into(),
+            });
         }
+        Ok(Self { package, name })
     }
 
     fn from_combined(input: &str) -> Result<Self> {
-        let mut parts = input.splitn(2, '/');
-        let package = match parts.next() {
-            Some(v) => v,
-            None => bail!("Package string constains no parts: {}", input),
-        };
-        let name = match parts.next() {
-            Some(v) => v,
-            None => bail!(
-                "Package string needs to be in package/name format: {}",
-                input
-            ),
-        };
-        let output = Self::new(package, name);
-        output.validate()?;
-        Ok(output)
+        let parts = input.splitn(2, '/').collect::<Vec<&str>>();
+        match parts[..] {
+            [package, name] => Self::new(package, name),
+            _ => Err(Error::InvalidMessagePath {
+                name: input.into(),
+                reason: "string needs to be in package/name format".into(),
+            }),
+        }
     }
 
-    /// Perform package name validity checks
-    ///
-    /// Based on [REP 144](https://www.ros.org/reps/rep-0144.html).
-    fn has_valid_package_name(&self) -> bool {
-        RE_PACKAGE_CORRECT_CHAR_SET_AND_LENGTH.is_match(&self.package)
-            && !RE_PACKAGE_CONSECUTIVE_UNDERSCORE.is_match(&self.package)
+    pub fn package(&self) -> &str {
+        &self.package
     }
 
-    pub fn validate(&self) -> Result<()> {
-        if !self.has_valid_package_name() {
-            bail!(ErrorKind::PackageNameInvalid(self.package.clone()));
-        }
-        Ok(())
+    pub fn name(&self) -> &str {
+        &self.name
+    }
+
+    pub fn into_inner(self) -> (String, String) {
+        (self.package, self.name)
     }
 }
 
 impl Display for MessagePath {
     fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
-        write!(f, "{}/{}", self.package, self.name)
+        write!(f, "{}/{}", self.package(), self.name())
     }
 }
 
@@ -68,20 +75,10 @@ impl<'a> TryFrom<&'a str> for MessagePath {
     }
 }
 
-lazy_static! {
-    static ref RE_PACKAGE_CORRECT_CHAR_SET_AND_LENGTH: Regex =
-        Regex::new("^[a-z][a-z0-9_]+$").unwrap();
-    static ref RE_PACKAGE_CONSECUTIVE_UNDERSCORE: Regex = Regex::new("__").unwrap();
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;
 
-    fn is_valid_package_name(package: &str) -> bool {
-        MessagePath::new(package, "anything").has_valid_package_name()
-    }
-
     #[test]
     fn package_names_must_be_at_least_two_characters() {
         assert!(is_valid_package_name("foo"));

rosrust_codegen/Cargo.toml

@@ -18,5 +18,9 @@ proc-macro2 = "1.0.28"
 md-5 = "0.9.1"
 hex = "0.4.3"
 
+[dependencies.ros_msg_parser]
+path = "../ros_msg_parser"
+version = "0.1.0"
+
 [lib]
 proc-macro = true

rosrust_codegen/src/alerts.rs

@@ -0,0 +1,2 @@
+pub static MESSAGE_NAME_SHOULD_BE_VALID: &str =
+    "Message name should be valid, please report this as a bug";

rosrust_codegen/src/genmsg.rs

@@ -1,8 +1,8 @@
-use crate::error::Result;
+use crate::error::{Result, ResultExt};
 use crate::helpers;
 use crate::helpers::MessageMap;
-use crate::message_path::MessagePath;
 use crate::output_layout;
+use ros_msg_parser::MessagePath;
 use std::collections::{HashMap, HashSet};
 use std::convert::TryInto;
 
@@ -12,7 +12,6 @@ pub fn depend_on_messages(
     messages: &[&str],
 ) -> Result<output_layout::Layout> {
     let message_map = message_names_to_message_map(ignore_bad_messages, folders, messages)?;
-    validate_message_paths(&message_map)?;
     message_map_to_layout(&message_map)
 }
 
@@ -25,34 +24,25 @@ fn message_names_to_message_map(
         .iter()
         .copied()
         .map(TryInto::try_into)
-        .collect::<Result<Vec<MessagePath>>>()?;
+        .collect::<ros_msg_parser::Result<Vec<MessagePath>>>()
+        .chain_err(|| "Failed to parse all message paths")?;
     helpers::get_message_map(ignore_bad_messages, folders, &message_pairs)
 }
 
-fn validate_message_paths(message_map: &MessageMap) -> Result<()> {
-    for message in message_map.messages.keys() {
-        message.validate()?;
-    }
-    for message in message_map.services.keys() {
-        message.validate()?;
-    }
-    Ok(())
-}
-
 fn message_map_to_layout(message_map: &MessageMap) -> Result<output_layout::Layout> {
     let mut output = output_layout::Layout {
         packages: Vec::new(),
     };
-    let hashes = helpers::calculate_md5(&message_map)?;
+    let hashes = helpers::calculate_md5(message_map)?;
     let packages = message_map
         .messages
         .keys()
-        .map(|message| message.package.clone())
+        .map(|message| message.package().into())
         .chain(
             message_map
                 .services
                 .keys()
-                .map(|message| message.package.clone()),
+                .map(|message| message.package().into()),
         )
         .collect::<HashSet<String>>();
     for package in packages {
@@ -64,11 +54,11 @@ fn message_map_to_layout(message_map: &MessageMap) -> Result<output_layout::Layo
         let names = message_map
             .messages
             .iter()
-            .filter(|&(message, _value)| &message.package == &package)
-            .map(|(message, value)| (message.name.clone(), value.source.clone()))
+            .filter(|&(message, _value)| message.package() == package)
+            .map(|(message, value)| (message.name().into(), value.source.clone()))
             .collect::<HashMap<String, String>>();
         for (name, source) in names {
-            let key = MessagePath::new(&package, name);
+            let key = MessagePath::new(&package, name).chain_err(|| "Invalid message path")?;
             let message = message_map
                 .messages
                 .get(&key)
@@ -92,12 +82,12 @@ fn message_map_to_layout(message_map: &MessageMap) -> Result<output_layout::Layo
         let names = message_map
             .services
             .iter()
-            .filter(|&(message, _value)| &message.package == &package)
-            .map(|(message, value)| (message.name.clone(), value.source.clone()))
+            .filter(|&(message, _value)| message.package() == package)
+            .map(|(message, value)| (message.name().into(), value.source.clone()))
             .collect::<HashMap<String, String>>();
         for (name, source) in names {
             let md5sum = hashes
-                .get(&MessagePath::new(&package, &name))
+                .get(&MessagePath::new(&package, &name).chain_err(|| "Invalid message path")?)
                 .expect("Internal implementation contains mismatch in map keys")
                 .clone();
             let msg_type = format!("{}/{}", package, name);

rosrust_codegen/src/helpers.rs

@@ -1,10 +1,10 @@
+use crate::alerts::MESSAGE_NAME_SHOULD_BE_VALID;
 use crate::error::{ErrorKind, Result, ResultExt};
-use crate::message_path::MessagePath;
 use crate::msg::{Msg, Srv};
 use error_chain::bail;
 use lazy_static::lazy_static;
 use regex::RegexBuilder;
-use std;
+use ros_msg_parser::MessagePath;
 use std::collections::{HashMap, HashSet, LinkedList};
 use std::fs::{read_dir, File};
 use std::path::{Path, PathBuf};
@@ -29,8 +29,14 @@ pub fn calculate_md5(message_map: &MessageMap) -> Result<HashMap<MessagePath, St
         }
     }
     for message in message_map.services.keys() {
-        let key_req = MessagePath::new(&message.package, format!("{}Req", message.name));
-        let key_res = MessagePath::new(&message.package, format!("{}Res", message.name));
+        let key_req = MessagePath::new(message.package(), format!("{}Req", message.name()))
+            .chain_err(|| {
+                "Failed to create service request message! This is unexpected and probably a reportable bug."
+            })?;
+        let key_res = MessagePath::new(message.package(), format!("{}Res", message.name()))
+            .chain_err(|| {
+                "Failed to create service response message! This is unexpected and probably a reportable bug."
+            })?;
         let req = match representations.get(&key_req) {
             Some(v) => v,
             None => bail!("Message map does not contain all needed elements"),
@@ -64,7 +70,7 @@ pub fn generate_message_definition<S: std::hash::BuildHasher>(
     let mut handled_messages = HashSet::<MessagePath>::new();
     let mut result = message.source.clone();
     let mut pending = message
-        .dependencies()
+        .dependencies()?
         .into_iter()
         .collect::<LinkedList<_>>();
     while let Some(value) = pending.pop_front() {
@@ -79,7 +85,7 @@ pub fn generate_message_definition<S: std::hash::BuildHasher>(
             Some(msg) => msg,
             None => bail!("Message map does not contain all needed elements"),
         };
-        for dependency in message.dependencies() {
+        for dependency in message.dependencies()? {
             pending.push_back(dependency);
         }
         result += &message.source;
@@ -128,16 +134,16 @@ pub fn get_message_map(
             message_path,
         )? {
             MessageCase::Message(message) => {
-                for dependency in &message.dependencies() {
+                for dependency in &message.dependencies()? {
                     pending.push(dependency.clone());
                 }
                 messages.insert(message.path.clone(), message);
             }
             MessageCase::Service(service, req, res) => {
-                for dependency in &req.dependencies() {
+                for dependency in &req.dependencies()? {
                     pending.push(dependency.clone());
                 }
-                for dependency in &res.dependencies() {
+                for dependency in &res.dependencies()? {
                     pending.push(dependency.clone());
                 }
                 messages.insert(req.path.clone(), req);
@@ -182,7 +188,7 @@ fn identify_message_or_service(filename: &Path) -> Option<(MessagePath, PathBuf,
         _ => return None,
     };
     Some((
-        MessagePath::new(package.to_str()?, message.to_str()?),
+        MessagePath::new(package.to_str()?, message.to_str()?).ok()?,
         filename.into(),
         message_type,
     ))
@@ -201,15 +207,15 @@ lazy_static! {
 fn generate_in_memory_messages() -> HashMap<MessagePath, &'static str> {
     let mut output = HashMap::new();
     output.insert(
-        MessagePath::new("rosgraph_msgs", "Clock"),
+        MessagePath::new("rosgraph_msgs", "Clock").expect(MESSAGE_NAME_SHOULD_BE_VALID),
         include_str!("msg_examples/rosgraph_msgs/msg/Clock.msg"),
     );
     output.insert(
-        MessagePath::new("rosgraph_msgs", "Log"),
+        MessagePath::new("rosgraph_msgs", "Log").expect(MESSAGE_NAME_SHOULD_BE_VALID),
         include_str!("msg_examples/rosgraph_msgs/msg/Log.msg"),
     );
     output.insert(
-        MessagePath::new("std_msgs", "Header"),
+        MessagePath::new("std_msgs", "Header").expect(MESSAGE_NAME_SHOULD_BE_VALID),
         include_str!("msg_examples/std_msgs/msg/Header.msg"),
     );
     output
@@ -225,8 +231,8 @@ fn get_message_or_service(
 ) -> Result<MessageCase> {
     use std::io::Read;
 
-    let package = &message.package;
-    let name = &message.name;
+    let package = message.package();
+    let name = message.name();
 
     if let Some(full_path) = message_locations.get(&message) {
         if let Ok(mut f) = File::open(full_path) {
@@ -250,12 +256,16 @@ fn get_message_or_service(
                 v => bail!("Service {} is split into {} parts", message, v.len()),
             };
             let req = create_message(
-                MessagePath::new(package, format!("{}Req", name)),
+                MessagePath::new(package, format!("{}Req", name)).chain_err(|| {
+                        "Failed to create service request message! This is unexpected and probably a reportable bug."
+                })?  ,
                 req,
                 ignore_bad_messages,
             )?;
             let res = create_message(
-                MessagePath::new(package, format!("{}Res", name)),
+                MessagePath::new(package, format!("{}Res", name)).chain_err(|| {
+                    "Failed to create service response message! This is unexpected and probably a reportable bug."
+                })?,
                 res,
                 ignore_bad_messages,
             )?;