Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow ? and * parameters to be omitted in tail position #815

Merged
merged 2 commits into from
Aug 15, 2024
Merged

Allow ? and * parameters to be omitted in tail position #815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a1c9bbe
allow ? and * parameters to be omitted in tail position
zslayton Aug 15, 2024
1c0456c
adds unit tests for explicit empty stream
zslayton Aug 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/lazy/expanded/template.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,13 @@ impl Parameter {
pub fn is_variadic(&self) -> bool {
!matches!(self.cardinality, ParameterCardinality::ExactlyOne)
}
/// Returns true if a text e-expression can omit this argument.
///
/// Arguments for parameters with a cardinality of zero-or-one (`?`) or zero-or-more (`*`) can
/// be omitted if there are no other arguments being passed.
pub fn can_be_omitted(&self) -> bool {
matches!(self.cardinality, ParameterCardinality::ZeroOrOne | ParameterCardinality::ZeroOrMore)
}
}

/// The encoding used to serialize and deserialize the associated parameter.
Expand Down
44 changes: 37 additions & 7 deletions src/lazy/text/buffer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1142,7 +1142,7 @@ impl<'top> TextBufferView<'top> {
Some(arg) => arg_expr_cache.push(arg),
None => {
for param in &signature_params[index..] {
if param.rest_syntax_policy() == RestSyntaxPolicy::NotAllowed {
if !param.can_be_omitted() {
return fatal_parse_error(
self,
format!(
Expand Down Expand Up @@ -1242,16 +1242,24 @@ impl<'top> TextBufferView<'top> {
}
}

pub fn match_empty_arg_group(self) -> IonMatchResult<'top> {
recognize(pair(tag("(:"), whitespace_and_then(tag(")"))))(self)
Comment on lines -1245 to -1246
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ The original impl was just returning the matched slice. Now there's a concrete type that represents an argument group, so we instantiate one of those and return it.

pub fn match_empty_arg_group(
self,
parameter: &'top Parameter,
) -> IonParseResult<'top, EExpArg<'top, TextEncoding_1_1>> {
recognize(pair(tag("(:"), whitespace_and_then(tag(")"))))
.map(|matched_expr| {
let arg_group = TextEExpArgGroup::new(parameter, matched_expr, &[]);
EExpArg::new(parameter, EExpArgExpr::ArgGroup(arg_group))
})
.parse(self)
}

pub fn match_zero_or_one(
self,
parameter: &'top Parameter,
) -> IonParseResult<'top, Option<EExpArg<'top, TextEncoding_1_1>>> {
whitespace_and_then(alt((
Self::match_empty_arg_group.map(|_| None),
Self::parser_with_arg(Self::match_empty_arg_group, parameter).map(Some),
// TODO: Match a non-empty arg group and turn it into a failure with a helpful error message
Self::match_sexp_value_1_1.map(|maybe_expr| {
maybe_expr.map(|expr| {
Expand Down Expand Up @@ -1285,7 +1293,7 @@ impl<'top> TextBufferView<'top> {
self,
parameter: &'top Parameter,
) -> IonParseResult<'top, Option<EExpArg<'top, TextEncoding_1_1>>> {
if self.match_empty_arg_group().is_ok() {
if self.match_empty_arg_group(parameter).is_ok() {
return Err(nom::Err::Failure(IonParseError::Invalid(
InvalidInputError::new(self).with_description(format!(
"parameter '{}' is one-or-more (`+`) and cannot accept an empty stream",
Expand Down Expand Up @@ -2660,8 +2668,8 @@ mod tests {
}

macro_rules! matcher_tests_with_macro {
($parser:ident $macro_src:literal $($expect:ident: [$($input:literal),+$(,)?]),+$(,)?) => {
mod $parser {
($mod_name:ident $parser:ident $macro_src:literal $($expect:ident: [$($input:literal),+$(,)?]),+$(,)?) => {
mod $mod_name {
Comment on lines -2663 to +2672
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ This macro creates a module with a bunch of nested unit tests. However, the module name was also the name of the parser. This meant you couldn't write two sets of unit tests that used the same parser entry point. I added another parameter ($mod_name) to this macro to allow the module name to be specified separately.

use super::*;
$(
#[test]
Expand Down Expand Up @@ -2967,6 +2975,7 @@ mod tests {
}

matcher_tests_with_macro! {
parsing_sexps
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗺️ In the next few tests, I added a distinct module name.

match_sexp_1_1
"(macro foo (x*) null)"
expect_match: [
Expand Down Expand Up @@ -3001,6 +3010,7 @@ mod tests {
}

matcher_tests_with_macro! {
parsing_lists
match_list_1_1
"(macro foo (x*) null)"
expect_match: [
Expand All @@ -3015,6 +3025,7 @@ mod tests {
}

matcher_tests_with_macro! {
parsing_eexps
match_e_expression
"(macro foo (x*) null)"
expect_match: [
Expand Down Expand Up @@ -3046,6 +3057,25 @@ mod tests {
]
}

matcher_tests_with_macro! {
allow_omitting_trailing_optionals
match_e_expression
"(macro foo (a b+ c? d*) null)"
expect_match: [
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth adding an expectation that the explicit empty argument group (:) also works?

"(:foo 1 2)",
"(:foo 1 2 3)",
"(:foo 1 2 3 4)",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"(:foo 1 2 3 4)",
"(:foo 1 2 3 4)",
"(:foo 1 2 (: 3) 4)",
"(:foo 1 2 3 (: 4))",

...unless these are already covered elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The text reader doesn't have non-empty arg group support yet.

"(:foo 1 2 3 4 5 6)", // implicit rest
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"(:foo 1 2 3 4 5 6)", // implicit rest
"(:foo 1 2 3 4 5 6)", // implicit rest
"(:foo 1 2 (: 3) 4 5 6)", // implicit rest
"(:foo 1 2 3 (: 4 5 6))",

...unless these are already covered elsewhere.

"(:foo 1 2 3 (:))", // explicit empty stream
"(:foo 1 2 (:) 4)",
"(:foo 1 2 (:) (:))",
],
expect_mismatch: [
"(:foo 1)",
"(:foo)",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"(:foo)",
"(:foo)",
"(:foo 1 2 3 4 (: 5) 6)", // final argument must be rest or expression group, but not a mix of both

...unless this is already covered elsewhere.

]
}

#[rstest]
#[case::empty("(:)")]
#[case::empty_with_extra_spacing("(: )")]
Expand Down
Loading