rosidl icon indicating copy to clipboard operation
rosidl copied to clipboard

Single quote characters in comment strings not escaped in IDL

Open rokel opened this issue 5 years ago • 2 comments

Bug report

Required Info:

  • Operating System: Ubuntu 20.04
  • Installation type: binaries
  • Version or commit hash: foxy
  • DDS implementation: Fast-RTPS
  • Client library (if applicable): N/A

Steps to reproduce issue

  1. Use rosidl_adapter.parser.parse_message_file on a .msg that has a single quote character in a comment, e.g. Time.msg
  2. Resulting MessageSpecification contains annotations['comment'] fields with unescaped single quote in.
  3. Convert to IDL string using str()

Expected behavior

The eProsima xtypes::idl::parse() function can parse this IDL string.

Actual behavior

The eProsima xtypes::idl::parse() function fails to parse this IDL string.

Additional information

I'm not certain whether rosidl_adapter should be escaping single quotes in comment strings, like it currently does for double quotes. The OMG IDL spec 4.2 seems a bit ambiguous - section 7.2.6.2.2 mentions escaping single quotes in character literals, which make up string literals, and presumably text in annotations also follow those rules?

rokel avatar Jan 15 '21 12:01 rokel

You are right, the specification is a bit ambiguous on this. Section 7.2.2 says that "Comments may contain alphabetic, digit, graphic, space, horizontal tab, vertical tab, form feed, and newline characters". Table 7-4 lists an apostrophe as a "graphic" character, and while they show the curled version, I think we can reasonably assume the ' version is allowed as well.

Thus, I'd be for making this work. Do you have an end-to-end example of how this currently fails? Even better, if you could make a pull request to fix it, I'd be happy to review it.

clalancette avatar Jan 30 '21 14:01 clalancette

Thinking about this more I'm not sure it is ambiguous - 7.2.6.2.2 states single quotes are non-graphic and must be escaped, as opposed to apostrophes:

Nongraphic characters shall be represented using escape sequences as defined below in Table 7-9

So I think the xtypes parser is failing correctly here. From my reading of the rosidl_generator_cpp code I think adding the escape sequence here should fix it:

https://github.com/ros2/rosidl/blob/681e833cdb60f97c10a2891d05545c24eb7c1c4b/rosidl_generator_cpp/rosidl_generator_cpp/init.py#L243-L246

And presumably the same in rosidl_generator_c. But have not yet been able to setup a working dev env to test that.

rokel avatar Feb 09 '21 17:02 rokel