diff --git a/slicec/src/slice_options.rs b/slicec/src/slice_options.rs index f0272466..14bfd75a 100644 --- a/slicec/src/slice_options.rs +++ b/slicec/src/slice_options.rs @@ -23,7 +23,7 @@ pub struct SliceOptions { /// Ex: '--generator /path/to/my/generator' /// /// Each code-generator can be provided with arbitrary string arguments using the following syntax: - /// '/path/to/my/generator;arg1=value1;arg2 = value2;arg3;...' + /// '/path/to/my/generator,arg1=value1,arg2 = value2,arg3,...' /// Leading and trailing whitespace is stripped from arguments and their values. Argument values are optional. #[arg(short = 'G', long = "generator", num_args = 1, action = Append, value_name = "GENERATOR", value_parser = plugin_parser, verbatim_doc_comment)] pub generators: Vec, @@ -62,7 +62,6 @@ This AST is encoded with Slice, and then output, to be consumed by other tools." fn plugin_parser<'a>(s: &str) -> Result { // Helper enum to track what element the parser is currently parsing. - #[derive(PartialEq, Eq)] enum State { Path, Key, @@ -82,22 +81,14 @@ fn plugin_parser<'a>(s: &str) -> Result { let mut char_iter = s.chars().peekable(); while let Some(c) = char_iter.next() { match c { - // The next character after this is being escaped, add it directly to the buffer without parsing. - '\\' => match char_iter.next() { - Some(escaped_char) => string_buffer.push(escaped_char), - None => return Err("unterminated escape sequence (for a literal '\\' character, use '\\\\')"), - }, - - ';' => { - if string_buffer.is_empty() { - match state { - State::Path => return Err("missing plugin path (ex: 'PATH;KEY=VALUE')"), - State::Key => return Err("missing argument key (ex: 'PATH;KEY=VALUE')"), - State::Value => return Err("missing argument value (ex: 'PATH;KEY=VALUE' or 'PATH;KEY')"), - } - } + // If the current character is a backslash, and the next character is either ',' or '=', + // this is an escaping backslash. We eat the backslash, and write the next character as-is. + '\\' if matches!(char_iter.peek(), Some(',' | '=')) => { + string_buffer.push(char_iter.next().unwrap()); + } - // We only handle ';' if there's more characters after it. If it's a trailing ';' we ignore it. + ',' => { + // We only handle ',' if there's more characters after it. If it's a trailing ',' we ignore it. if char_iter.peek().is_some() { // Add a '(key=value)' argument pair, and re-target `string_buffer` to point at the key's buffer. plugin_args.push(Default::default()); @@ -109,13 +100,6 @@ fn plugin_parser<'a>(s: &str) -> Result { '=' => match state { State::Path => string_buffer.push('='), // '=' has no special meaning in the plugin path. State::Key => { - if string_buffer.is_empty() { - return Err("missing argument key (ex: 'PATH;KEY=VALUE')"); - } - if matches!(char_iter.peek(), None | Some(';')) { - return Err("missing argument value (ex: 'PATH;KEY=VALUE' or 'PATH;KEY')"); - } - // Re-target `string_buffer` to point at the argument value's buffer (instead of the key). string_buffer = &mut plugin_args.last_mut().unwrap().1; state = State::Value; @@ -131,11 +115,21 @@ fn plugin_parser<'a>(s: &str) -> Result { // Trim any leading/trailing whitespace from the path and arguments. let path = plugin_path.trim().to_owned(); - let args = plugin_args + let args: Vec<_> = plugin_args .into_iter() .map(|(key, value)| (key.trim().to_owned(), value.trim().to_owned())) .collect(); + // Validate that the plugin path, and any argument keys are non-empty (empty argument values are fine). + if path.is_empty() { + return Err("missing plugin path (ex: 'PATH,KEY=VALUE')"); + } + for arg in &args { + if arg.0.is_empty() { + return Err("missing argument key (ex: 'PATH,KEY=VALUE')"); + } + } + Ok(Plugin { path, args }) } diff --git a/slicec/tests/command_line_tests.rs b/slicec/tests/command_line_tests.rs index 51cce743..43b49836 100644 --- a/slicec/tests/command_line_tests.rs +++ b/slicec/tests/command_line_tests.rs @@ -10,10 +10,11 @@ use test_case::test_case; mod test_helpers; -#[test] -fn plugins_can_have_no_arguments() { +#[test_case("foo"; "without trailing separator")] +#[test_case("foo,"; "with trailing separator")] +fn plugins_can_have_no_arguments(value: &str) { // Arrange - let input = ["", "--generator", "foo"]; + let input = ["", "--generator", value]; // Act let result = SliceOptions::try_parse_from(input); @@ -45,9 +46,9 @@ fn equals_sign_has_no_special_meaning_in_plugin_path() { } #[test] -fn path_characters_can_be_escaped() { +fn backslash_escapes_special_characters_in_path() { // Arrange - let input = ["", "--generator", "C:\\\\my\\;folder\\=cool/\\a"]; + let input = ["", "--generator", "C:\\my\\,folder\\=cool/\\a"]; // Act let result = SliceOptions::try_parse_from(input); @@ -57,14 +58,55 @@ fn path_characters_can_be_escaped() { assert_eq!(parsed_options.generators.len(), 1); let generator_plugin = &parsed_options.generators[0]; - assert_eq!(generator_plugin.path, "C:\\my;folder=cool/a"); + assert_eq!(generator_plugin.path, "C:\\my,folder=cool/\\a"); assert!(generator_plugin.args.is_empty()); } #[test] -fn plugins_can_have_a_single_argument() { +fn backslash_escapes_special_characters_in_arguments() { + // Arrange + let input = ["", "--generator", "C:\\plugin,my\\=name\\,=my\\=value\\,,"]; + + // Act + let result = SliceOptions::try_parse_from(input); + + // Assert + let parsed_options = result.unwrap(); + assert_eq!(parsed_options.generators.len(), 1); + + let generator_plugin = &parsed_options.generators[0]; + assert_eq!(generator_plugin.path, "C:\\plugin"); + assert_eq!(generator_plugin.args.len(), 1); + + assert_eq!(generator_plugin.args[0].0, "my=name,"); + assert_eq!(generator_plugin.args[0].1, "my=value,"); +} + +#[test] +fn backslash_does_not_escape_normal_characters() { + // Arrange + let input = ["", "--generator", "C:\\Users\\Me\\_plugin,na\\me=\\value\\"]; + + // Act + let result = SliceOptions::try_parse_from(input); + + // Assert + let parsed_options = result.unwrap(); + assert_eq!(parsed_options.generators.len(), 1); + + let generator_plugin = &parsed_options.generators[0]; + assert_eq!(generator_plugin.path, "C:\\Users\\Me\\_plugin"); + assert_eq!(generator_plugin.args.len(), 1); + + assert_eq!(generator_plugin.args[0].0, "na\\me"); + assert_eq!(generator_plugin.args[0].1, "\\value\\"); +} + +#[test_case("C:\\my_plugin,arg1=val1"; "without trailing separator")] +#[test_case("C:\\my_plugin,arg1=val1,"; "with trailing separator")] +fn plugins_can_have_a_single_argument(value: &str) { // Arrange - let input = ["", "--generator", "C:\\\\my_plugin;arg1=val1"]; + let input = ["", "--generator", value]; // Act let result = SliceOptions::try_parse_from(input); @@ -84,7 +126,7 @@ fn plugins_can_have_a_single_argument() { #[test] fn plugins_arguments_have_whitespace_trimmed() { // Arrange - let input = ["", "--generator", "/path/to/plugin; arg1 = val1 "]; + let input = ["", "--generator", "/path/to/plugin, arg1 = val1 "]; // Act let result = SliceOptions::try_parse_from(input); @@ -101,10 +143,12 @@ fn plugins_arguments_have_whitespace_trimmed() { assert_eq!(generator_plugin.args[0].1, "val1"); } -#[test] -fn plugins_arguments_can_omit_a_value() { +#[test_case("../plugin/path,foo,bar"; "without trailing equals")] +#[test_case("../plugin/path,foo=,bar="; "with trailing equals")] +#[test_case("../plugin/path,foo,bar=,"; "with trailing equals and separator")] +fn plugins_arguments_can_omit_a_value(value: &str) { // Arrange - let input = ["", "--generator", "../plugin/path;foo;bar"]; + let input = ["", "--generator", value]; // Act let result = SliceOptions::try_parse_from(input); @@ -126,7 +170,7 @@ fn plugins_arguments_can_omit_a_value() { #[test] fn plugins_can_have_multiple_arguments() { // Arrange - let input = ["", "--generator", "foo;arg1=val1;arg2;arg3 = val3"]; + let input = ["", "--generator", "foo,arg1=val1,arg2, arg3 = val3 "]; // Act let result = SliceOptions::try_parse_from(input); @@ -147,9 +191,10 @@ fn plugins_can_have_multiple_arguments() { assert_eq!(generator_plugin.args[2].1, "val3"); } -#[test_case("foo;arg="; "without trailing argument")] -#[test_case("foo;arg1=;arg2"; "with trailing argument")] -fn argument_value_must_appear_after_equals(generator_arg: &str) { +#[test_case(" "; "without trailing equals")] +#[test_case(" ,"; "with trailing separator")] +#[test_case(","; "only trailing separator")] +fn plugin_path_must_be_present(generator_arg: &str) { // Arrange let input = ["", "--generator", generator_arg]; @@ -161,15 +206,12 @@ fn argument_value_must_appear_after_equals(generator_arg: &str) { assert_eq!(parsing_error.kind(), ErrorKind::ValueValidation); let error_message = parsing_error.source().unwrap().to_string(); - assert_eq!( - error_message, - "missing argument value (ex: 'PATH;KEY=VALUE' or 'PATH;KEY')", - ); + assert_eq!(error_message, "missing plugin path (ex: 'PATH,KEY=VALUE')"); } -#[test_case("foo;="; "trailing equals")] -#[test_case("foo;=val"; "trailing equals with value")] -#[test_case("foo;;"; "trailing semicolon")] +#[test_case("foo,="; "trailing equals")] +#[test_case("foo,=val"; "trailing equals with value")] +#[test_case("foo,,"; "trailing separator")] fn argument_key_must_be_present(generator_arg: &str) { // Arrange let input = ["", "--generator", generator_arg]; @@ -182,13 +224,13 @@ fn argument_key_must_be_present(generator_arg: &str) { assert_eq!(parsing_error.kind(), ErrorKind::ValueValidation); let error_message = parsing_error.source().unwrap().to_string(); - assert_eq!(error_message, "missing argument key (ex: 'PATH;KEY=VALUE')"); + assert_eq!(error_message, "missing argument key (ex: 'PATH,KEY=VALUE')"); } #[test] fn double_equals_is_disallowed() { // Arrange - let input = ["", "--generator", "/paths/are=/=okay;arg1=key=1"]; + let input = ["", "--generator", "/paths/are=/=okay,arg1=key=1"]; // Act let result = SliceOptions::try_parse_from(input);