From 776787217b3629a08cce4afefa58742db264db30 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Sun, 26 Jul 2026 09:52:47 -0400 Subject: [PATCH 1/2] fix: `allow_missing_commas: false` was ignored for array elements Array parsing never consulted `allow_missing_commas`, so strict parsing accepted invalid JSON like `[1 2]` and `[01]`. Now array elements error with `ExpectedComma` the same way object properties do. Closes #84 --- src/parse_to_ast.rs | 63 ++++++++++++++++++++++++++++++++++++++----- src/parse_to_value.rs | 40 +++++++++++++++++++++++++++ src/parser.rs | 31 +++++++++++++-------- src/serde.rs | 46 +++++++++++++++++++++++++++++++ src/tokens.rs | 15 +++++++++++ 5 files changed, 178 insertions(+), 17 deletions(-) diff --git a/src/parse_to_ast.rs b/src/parse_to_ast.rs index bbe6783..57aba33 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 token.is_value_start() && !context.allow_missing_commas => { + let range = Range { + start: after_value_end, + end: after_value_end, + }; + return Err(context.create_error_for_range(range, ParseErrorKind::ExpectedComma)); + } + _ => {} } } @@ -742,6 +753,46 @@ 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"); + } + + #[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..17fae16 100644 --- a/src/parse_to_value.rs +++ b/src/parse_to_value.rs @@ -500,6 +500,46 @@ 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"); + } + + #[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..d1bdb0b 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -192,20 +192,29 @@ 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( + 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 token.is_value_start() && !self.allow_missing_commas => { + 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..004c855 100644 --- a/src/serde.rs +++ b/src/serde.rs @@ -722,4 +722,50 @@ 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("[[1] [2]]", "Expected comma on line 1 column 5"); + } + + #[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 + let result = parse_to_serde_value_strict::<(u32,)>("[1 2]"); + match result { + Ok(_) => panic!("Expected error, but did not find one."), + Err(err) => assert_eq!(err.to_string(), "Expected comma on line 1 column 3"), + } + } + + #[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), + } + } + + 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..e4c9528 100644 --- a/src/tokens.rs +++ b/src/tokens.rs @@ -44,6 +44,21 @@ impl<'a> Token<'a> { Token::CommentBlock(value) => value, } } + + /// If this token could begin a value, which is used to detect a + /// missing comma between array elements. + pub(crate) fn is_value_start(&self) -> bool { + matches!( + self, + Token::OpenBrace + | Token::OpenBracket + | Token::String(_) + | Token::Word(_) + | Token::Boolean(_) + | Token::Number(_) + | Token::Null + ) + } } /// A token with positional information. From 6f25dd1ddd29aba90af65750afb218951f100768 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Sun, 26 Jul 2026 09:59:32 -0400 Subject: [PATCH 2/2] code review - keep reporting `Unexpected word` for words in arrays, which are never valid elements regardless of `allow_missing_commas` - debug assert the previous value was consumed in `scan_array_comma` - more tests: cst, unterminated arrays, comments on the serde path and arrays that are still valid when missing commas are not allowed --- src/cst/mod.rs | 16 ++++++++++++++ src/parse_to_ast.rs | 49 ++++++++++++++++++++++++++++++------------- src/parse_to_value.rs | 46 +++++++++++++++++++++++++++++----------- src/parser.rs | 3 ++- src/serde.rs | 38 +++++++++++++++++++++++++++++++-- src/tokens.rs | 11 ++-------- 6 files changed, 125 insertions(+), 38 deletions(-) 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 57aba33..3a8d8af 100644 --- a/src/parse_to_ast.rs +++ b/src/parse_to_ast.rs @@ -424,7 +424,7 @@ fn parse_array<'a>(context: &mut Context<'a>) -> Result, ParseError> { return Err(context.create_error_for_range(comma_range, ParseErrorKind::TrailingCommasNotAllowed)); } } - Some(token) if token.is_value_start() && !context.allow_missing_commas => { + Some(token) if !context.allow_missing_commas && token.is_value_start() => { let range = Range { start: after_value_end, end: after_value_end, @@ -593,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(); @@ -769,6 +769,27 @@ mod tests { 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] diff --git a/src/parse_to_value.rs b/src/parse_to_value.rs index 17fae16..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( @@ -517,6 +518,27 @@ mod tests { 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] diff --git a/src/parser.rs b/src/parser.rs index d1bdb0b..b7c5101 100644 --- a/src/parser.rs +++ b/src/parser.rs @@ -192,6 +192,7 @@ 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> { + 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) => { @@ -206,7 +207,7 @@ impl<'a> JsoncParser<'a> { } Ok(next) } - Some(token) if token.is_value_start() && !self.allow_missing_commas => { + Some(token) if !self.allow_missing_commas && token.is_value_start() => { let range = Range::new(after_value_end, after_value_end); Err( self diff --git a/src/serde.rs b/src/serde.rs index 004c855..e26a73b 100644 --- a/src/serde.rs +++ b/src/serde.rs @@ -732,17 +732,43 @@ mod tests { 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 - let result = parse_to_serde_value_strict::<(u32,)>("[1 2]"); + 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 1 column 3"), + Err(err) => assert_eq!(err.to_string(), "Expected comma on line 2 column 4"), } } @@ -754,6 +780,14 @@ mod tests { } } + #[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, diff --git a/src/tokens.rs b/src/tokens.rs index e4c9528..8096bb0 100644 --- a/src/tokens.rs +++ b/src/tokens.rs @@ -45,18 +45,11 @@ impl<'a> Token<'a> { } } - /// If this token could begin a value, which is used to detect a - /// missing comma between array elements. + /// 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::Word(_) - | Token::Boolean(_) - | Token::Number(_) - | Token::Null + Token::OpenBrace | Token::OpenBracket | Token::String(_) | Token::Boolean(_) | Token::Number(_) | Token::Null ) } }