diff --git a/src/cst/mod.rs b/src/cst/mod.rs index 4142327..46c36e9 100644 --- a/src/cst/mod.rs +++ b/src/cst/mod.rs @@ -4062,6 +4062,22 @@ value3: true } } + #[test] + fn missing_comma_between_array_elements() { + build_cst("[1 2]"); + + // but is strict when strict + let options = crate::ParseOptions { + allow_missing_commas: false, + ..Default::default() + }; + assert_eq!( + CstRootNode::parse("[1 2]", &options).err().unwrap().to_string(), + "Expected comma on line 1 column 3" + ); + CstRootNode::parse("[1, 2]", &options).unwrap(); + } + #[track_caller] fn build_cst(text: &str) -> CstRootNode { CstRootNode::parse(text, &crate::ParseOptions::default()).unwrap() diff --git a/src/parse_to_ast.rs b/src/parse_to_ast.rs index bbe6783..3a8d8af 100644 --- a/src/parse_to_ast.rs +++ b/src/parse_to_ast.rs @@ -414,13 +414,24 @@ fn parse_array<'a>(context: &mut Context<'a>) -> Result, ParseError> { } // skip the comma - if let Some(Token::Comma) = context.scan()? { - let comma_range = context.create_range_from_last_token(); - if let Some(Token::CloseBracket) = context.scan()? - && !context.allow_trailing_commas - { - return Err(context.create_error_for_range(comma_range, ParseErrorKind::TrailingCommasNotAllowed)); + let after_value_end = context.last_token_end; + match context.scan()? { + Some(Token::Comma) => { + let comma_range = context.create_range_from_last_token(); + if let Some(Token::CloseBracket) = context.scan()? + && !context.allow_trailing_commas + { + return Err(context.create_error_for_range(comma_range, ParseErrorKind::TrailingCommasNotAllowed)); + } + } + Some(token) if !context.allow_missing_commas && token.is_value_start() => { + let range = Range { + start: after_value_end, + end: after_value_end, + }; + return Err(context.create_error_for_range(range, ParseErrorKind::ExpectedComma)); } + _ => {} } } @@ -582,25 +593,25 @@ mod tests { #[track_caller] fn assert_has_strict_error(text: &str, message: &str) { - let result = parse_to_ast( - text, - &Default::default(), - &ParseOptions { - allow_comments: false, - allow_loose_object_property_names: false, - allow_trailing_commas: false, - allow_missing_commas: false, - allow_single_quoted_strings: false, - allow_hexadecimal_numbers: false, - allow_unary_plus_numbers: false, - }, - ); + let result = parse_to_ast(text, &Default::default(), &strict_options()); match result { Ok(_) => panic!("Expected error, but did not find one."), Err(err) => assert_eq!(err.to_string(), message), } } + fn strict_options() -> ParseOptions { + ParseOptions { + allow_comments: false, + allow_loose_object_property_names: false, + allow_trailing_commas: false, + allow_missing_commas: false, + allow_single_quoted_strings: false, + allow_hexadecimal_numbers: false, + allow_unary_plus_numbers: false, + } + } + #[test] fn it_should_not_include_tokens_by_default() { let result = parse_to_ast("{}", &Default::default(), &Default::default()).unwrap(); @@ -742,6 +753,67 @@ mod tests { } } + #[test] + fn missing_comma_between_array_elements() { + let text = "[1 2]"; + let result = parse_to_ast(text, &Default::default(), &Default::default()).unwrap(); + let value = result.value.unwrap(); + let elements = &value.as_array().unwrap().elements; + assert_eq!(elements.len(), 2); + + // but is strict when strict + assert_has_strict_error(text, "Expected comma on line 1 column 3"); + assert_has_strict_error("[01]", "Expected comma on line 1 column 3"); + assert_has_strict_error(r#"["a" "b"]"#, "Expected comma on line 1 column 5"); + assert_has_strict_error("[true false]", "Expected comma on line 1 column 6"); + assert_has_strict_error("[null null]", "Expected comma on line 1 column 6"); + assert_has_strict_error("[[1] [2]]", "Expected comma on line 1 column 5"); + assert_has_strict_error(r#"[{"a":1} {"b":2}]"#, "Expected comma on line 1 column 9"); + assert_has_strict_error("[1 2", "Expected comma on line 1 column 3"); + + // these are not missing commas + assert_has_strict_error("[1", "Unterminated array on line 1 column 1"); + assert_has_strict_error("[1 a ]", "Unexpected word on line 1 column 4"); + + for text in ["[]", "[ ]", "[1,2]", "[1 , 2]", "[[1],[2]]", r#"[{"a":1},{"b":2}]"#] { + parse_to_ast(text, &Default::default(), &strict_options()).unwrap(); + } + } + + #[test] + fn missing_comma_not_allowed_with_trailing_commas_allowed_in_array() { + let options = ParseOptions { + allow_missing_commas: false, + allow_trailing_commas: true, + ..Default::default() + }; + for text in ["[]", "[1,]", "[1, 2,]", "[[1,],[2,],]"] { + parse_to_ast(text, &Default::default(), &options).unwrap(); + } + } + + #[test] + fn missing_comma_with_comment_between_array_elements() { + // when comments are allowed but missing commas are not, + // should still detect the missing comma after the comment is skipped + let result = parse_to_ast( + r#"[ + 1 // comment here + 2 +]"#, + &Default::default(), + &ParseOptions { + allow_comments: true, + allow_missing_commas: false, + ..Default::default() + }, + ); + match result { + Ok(_) => panic!("Expected error, but did not find one."), + Err(err) => assert_eq!(err.to_string(), "Expected comma on line 2 column 4"), + } + } + #[test] fn it_should_error_when_arrays_are_deeply_nested() { // Deeply nested arrays cause a stack overflow when recursion depth is not limited diff --git a/src/parse_to_value.rs b/src/parse_to_value.rs index 24a3a89..774c31c 100644 --- a/src/parse_to_value.rs +++ b/src/parse_to_value.rs @@ -343,24 +343,25 @@ mod tests { #[track_caller] fn assert_has_strict_error(text: &str, message: &str) { - let result = parse_to_value( - text, - &ParseOptions { - allow_comments: false, - allow_loose_object_property_names: false, - allow_trailing_commas: false, - allow_missing_commas: false, - allow_single_quoted_strings: false, - allow_hexadecimal_numbers: false, - allow_unary_plus_numbers: false, - }, - ); + let result = parse_to_value(text, &strict_options()); match result { Ok(_) => panic!("Expected error, but did not find one."), Err(err) => assert_eq!(err.to_string(), message), } } + fn strict_options() -> ParseOptions { + ParseOptions { + allow_comments: false, + allow_loose_object_property_names: false, + allow_trailing_commas: false, + allow_missing_commas: false, + allow_single_quoted_strings: false, + allow_hexadecimal_numbers: false, + allow_unary_plus_numbers: false, + } + } + #[test] fn it_should_error_when_has_multiple_values() { assert_has_error( @@ -500,6 +501,67 @@ mod tests { } } + #[test] + fn missing_comma_between_array_elements() { + let text = "[1 2]"; + let value = parse_to_value(text, &Default::default()).unwrap().unwrap(); + assert_eq!( + value, + JsonValue::Array(vec![JsonValue::Number("1"), JsonValue::Number("2")].into()) + ); + + // but is strict when strict + assert_has_strict_error(text, "Expected comma on line 1 column 3"); + assert_has_strict_error("[01]", "Expected comma on line 1 column 3"); + assert_has_strict_error(r#"["a" "b"]"#, "Expected comma on line 1 column 5"); + assert_has_strict_error("[true false]", "Expected comma on line 1 column 6"); + assert_has_strict_error("[null null]", "Expected comma on line 1 column 6"); + assert_has_strict_error("[[1] [2]]", "Expected comma on line 1 column 5"); + assert_has_strict_error(r#"[{"a":1} {"b":2}]"#, "Expected comma on line 1 column 9"); + assert_has_strict_error("[1 2", "Expected comma on line 1 column 3"); + + // these are not missing commas + assert_has_strict_error("[1", "Unterminated array on line 1 column 3"); + assert_has_strict_error("[1 a ]", "Unexpected word on line 1 column 4"); + + for text in ["[]", "[ ]", "[1,2]", "[1 , 2]", "[[1],[2]]", r#"[{"a":1},{"b":2}]"#] { + parse_to_value(text, &strict_options()).unwrap(); + } + } + + #[test] + fn missing_comma_not_allowed_with_trailing_commas_allowed_in_array() { + let options = ParseOptions { + allow_missing_commas: false, + allow_trailing_commas: true, + ..Default::default() + }; + for text in ["[]", "[1,]", "[1, 2,]", "[[1,],[2,],]"] { + parse_to_value(text, &options).unwrap(); + } + } + + #[test] + fn missing_comma_with_comment_between_array_elements() { + // when comments are allowed but missing commas are not, + // should still detect the missing comma after the comment is skipped + let result = parse_to_value( + r#"[ + 1 // comment here + 2 +]"#, + &ParseOptions { + allow_comments: true, + allow_missing_commas: false, + ..Default::default() + }, + ); + match result { + Ok(_) => panic!("Expected error, but did not find one."), + Err(err) => assert_eq!(err.to_string(), "Expected comma on line 2 column 4"), + } + } + #[test] fn it_should_parse_unquoted_keys_with_hex_and_trailing_comma() { let text = r#"{ diff --git a/src/parser.rs b/src/parser.rs index 900d2c8..b7c5101 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -192,20 +192,30 @@ impl<'a> JsoncParser<'a> { /// After an array element, scans for the comma/close-bracket and /// returns the next token. pub fn scan_array_comma(&mut self) -> Result>, ParseError> { - let token = self.scan()?; - if matches!(&token, Some(Token::Comma)) { - let comma_range = Range::new(self.scanner.token_start(), self.scanner.token_end()); - let next = self.scan()?; - if matches!(&next, Some(Token::CloseBracket)) && !self.allow_trailing_commas { - return Err( + debug_assert!(self.pending_token.is_none(), "the previous value must be consumed"); + let after_value_end = self.scanner.token_end(); + match self.scan()? { + Some(Token::Comma) => { + let comma_range = Range::new(self.scanner.token_start(), self.scanner.token_end()); + let next = self.scan()?; + if matches!(&next, Some(Token::CloseBracket)) && !self.allow_trailing_commas { + return Err( + self + .scanner + .create_error_for_range(comma_range, ParseErrorKind::TrailingCommasNotAllowed), + ); + } + Ok(next) + } + Some(token) if !self.allow_missing_commas && token.is_value_start() => { + let range = Range::new(after_value_end, after_value_end); + Err( self .scanner - .create_error_for_range(comma_range, ParseErrorKind::TrailingCommasNotAllowed), - ); + .create_error_for_range(range, ParseErrorKind::ExpectedComma), + ) } - Ok(next) - } else { - Ok(token) + token => Ok(token), } } diff --git a/src/serde.rs b/src/serde.rs index 38665a0..e26a73b 100644 --- a/src/serde.rs +++ b/src/serde.rs @@ -722,4 +722,84 @@ mod tests { assert_eq!(result, Config { value: 42 }); } + + #[test] + fn missing_comma_between_array_elements() { + let result = parse_to_serde_value::("[1 2]", &Default::default()).unwrap(); + assert_eq!(result, serde_json::json!([1, 2])); + + // but is strict when strict + assert_has_strict_error("[1 2]", "Expected comma on line 1 column 3"); + assert_has_strict_error("[01]", "Expected comma on line 1 column 3"); + assert_has_strict_error(r#"["a" "b"]"#, "Expected comma on line 1 column 5"); + assert_has_strict_error("[true false]", "Expected comma on line 1 column 6"); + assert_has_strict_error("[null null]", "Expected comma on line 1 column 6"); + assert_has_strict_error("[[1] [2]]", "Expected comma on line 1 column 5"); + assert_has_strict_error(r#"[{"a":1} {"b":2}]"#, "Expected comma on line 1 column 9"); + + for text in ["[]", "[1,2]", "[1 , 2]", "[[1],[2]]", r#"[{"a":1},{"b":2}]"#] { + parse_to_serde_value_strict::(text).unwrap(); + } + } + + #[test] + fn missing_comma_between_array_elements_when_draining() { + // a tuple stops reading before the end of the array, so the + // remaining elements are drained and should still be strict + assert_has_strict_drain_error::<(u32,)>("[1 2]", "Expected comma on line 1 column 3"); + // the drained element here ends with a `}` + assert_has_strict_drain_error::<(u32, SerdeValue)>(r#"[1, {"a":2} 3]"#, "Expected comma on line 1 column 12"); + } + + #[test] + fn missing_comma_with_comment_between_array_elements() { + // when comments are allowed but missing commas are not, + // should still detect the missing comma after the comment is skipped + let result = parse_to_serde_value::( + r#"[ + 1 // comment here + 2 +]"#, + &ParseOptions { + allow_comments: true, + allow_missing_commas: false, + ..Default::default() + }, + ); + match result { + Ok(_) => panic!("Expected error, but did not find one."), + Err(err) => assert_eq!(err.to_string(), "Expected comma on line 2 column 4"), + } + } + + #[track_caller] + fn assert_has_strict_error(text: &str, message: &str) { + match parse_to_serde_value_strict::(text) { + Ok(_) => panic!("Expected error, but did not find one."), + Err(err) => assert_eq!(err.to_string(), message), + } + } + + #[track_caller] + fn assert_has_strict_drain_error(text: &str, message: &str) { + match parse_to_serde_value_strict::(text) { + Ok(_) => panic!("Expected error, but did not find one."), + Err(err) => assert_eq!(err.to_string(), message), + } + } + + fn parse_to_serde_value_strict(text: &str) -> Result { + parse_to_serde_value( + text, + &ParseOptions { + allow_comments: false, + allow_loose_object_property_names: false, + allow_trailing_commas: false, + allow_missing_commas: false, + allow_single_quoted_strings: false, + allow_hexadecimal_numbers: false, + allow_unary_plus_numbers: false, + }, + ) + } } diff --git a/src/tokens.rs b/src/tokens.rs index 7e66f96..8096bb0 100644 --- a/src/tokens.rs +++ b/src/tokens.rs @@ -44,6 +44,14 @@ impl<'a> Token<'a> { Token::CommentBlock(value) => value, } } + + /// Whether this token can begin a JSON value. + pub(crate) fn is_value_start(&self) -> bool { + matches!( + self, + Token::OpenBrace | Token::OpenBracket | Token::String(_) | Token::Boolean(_) | Token::Number(_) | Token::Null + ) + } } /// A token with positional information.