Always include whitespace in string literals

[sloretz]

Alternative to #677
Fixes #676

rosidl_parser randomly ignores whitespace at the start of string literals. This example matches the string literal as Token('__ANON_6', ' e') or Token('__ANON_6', 'e') when run multiple times; however, stripping the whitespace is incorrect.

from rosidl_parser.parser import get_ast_from_idl_string

idl_str ='''module MRE {
const string foo = "    e";
};'''
print(get_ast_from_idl_string(idl_str))

I think the cause is a combination of ignoring whitespace

rosidl/rosidl_parser/rosidl_parser/grammar.lark

Line 18 in 0250ae8

%ignore WS

and the way the string_literal rule is constructed

rosidl/rosidl_parser/rosidl_parser/grammar.lark

Line 77 in 0250ae8

string_literal: "\"\"" | "\"" /(\\\"|[^"])+/ "\""

The string_literal rule has two parts. The first part matches an empty string "", and the second part matches a non-empty string. The second part says to match three things. First match a quote ". Next match at least one of: an escaped quote \", or a character that is not a quote. Finally match another quote ". I think the problem is the second rule being divided into three parts. It ambiguous if the parser should ignore the whitespace between the first quote and the string content, or if it should include the whitespace via the not-a-quote regex.

Lark has a built in rule for exactly this situation: https://github.com/lark-parser/lark/blob/953171821ed307f700fddf27f3fcb9483346bd46/lark/grammars/common.lark#L26-L29

We could %import common.ESCAPED_STRING for string_literal, but whide_string_literal probably has the same problem. If I defined wide_string_literal as "L" ESCAPED_STRING then it would also match L "white space between L and first quote", which is also incorrect. I've chosen to copy the rules from lark into our grammar instead.

There's a stackoverlow post explaining how lark's ESCAPED_STRING rule works: https://stackoverflow.com/a/61374198

[sloretz]

@pablogs9 this seems to resolve the issue on my machine. Mind checking if it works for your use case?

[pablogs9]

@sloretz LGTM, thanks a lot for this.

Are you going to backport to humble?

[sloretz]

I added tests for whitespace at the start and end of the string. With rolling as it is before this PR I only see the whitespace at the start of the string and wide string tests fail. I left the whitespace at the end of the string tests in since there's no harm in testing it. All tests pass with this PR.

[sloretz]

Are you going to backport to humble?

I think this change is backportable to humble.

[sloretz]

CI (build: --packages-above-and-dependencies rosidl_parser test: --packages-above rosidl_parser)

• Linux
• Linux-aarch64
• Windows

[sloretz]

Linux CI failure is known to be flaky: ros2/rosbag2#862. CI LGTM!

[wjwwood]

lgtm

code changes are arcane, but the test cases make sense to me.

+1 for backporting, if anyone was relying on this behavior it was a bug and therefore I think it's ok to break them.

[sloretz]

@Mergifyio backport humble galactic foxy

[mergify]

backport humble galactic foxy

✅ Backports have been created

• #689 Always include whitespace in string literals (backport #688) has been created for branch humble
• #690 Always include whitespace in string literals (backport #688) has been created for branch galactic
• #691 Always include whitespace in string literals (backport #688) has been created for branch foxy