mirror of
https://github.com/nushell/nushell.git
synced 2026-10-29 09:25:02 +08:00
fix(parser): reject unexpected semicolons in list literals and patterns (#19063)
## Description List literals such as `[1; 2]` previously evaluated to `[1]`, silently discarding content after the semicolon. The same issue affected `match` list patterns and could cause incorrect matches. In this PR, I added an error check that reports parse error for unexpected semicolon separators in both list literals and list patterns, with guidance to use commas or whitespace instead. It also aligns the NUON table grammar with the documented requirement for at least one data row and marks the header-only table parsing bug as fixed. Note: this version also rejects `[;1]`, `[1;]`, and `[;]`. I can't find rules in the NUON spec that allow leading or trailing semicolons. The parser now rejects all semicolon separators in list literals but I think this should be discussed further in #11631 before merging, since it may break existing code that relies on the current behavior. ## User-facing changes (Release notes) List literals and `match` list patterns now reject unexpected semicolon separators instead of silently accepting malformed input or discarding subsequent content. Header-only table syntax such as `[[a b];]` now also produces a parse error. ## Additional notes Fixes #11631. Related: the semicolon discussion in #14188 and the NUON specification added in #18925. AI assistance (GPT-6 Astra) was used to investigate related issues and add regression tests. The final review and writing of this PR were done by myself.
This commit is contained in:
1 parent
3e080262cc
commit
9c9f93c727
5 files changed
+177
-6
No files matched your search
@@ -130,6 +130,18 @@ pub fn parse_list_expression(
|
||||
working_set.error(err)
|
||||
}
|
||||
|
||||
if let Some(token) = output
|
||||
.iter()
|
||||
.find(|token| token.contents == TokenContents::Semicolon)
|
||||
{
|
||||
working_set.error(ParseError::LabeledErrorWithHelp {
|
||||
error: "Unexpected semicolon in list".into(),
|
||||
label: "not a valid list separator".into(),
|
||||
help: "Use commas or whitespace to separate list items.".into(),
|
||||
span: token.span,
|
||||
});
|
||||
}
|
||||
|
||||
let (mut output, err) = lite_parse(&output, working_set);
|
||||
if let Some(err) = err {
|
||||
working_set.error(err)
|
||||
@@ -261,18 +273,26 @@ pub(crate) fn parse_table_expression(
|
||||
let [first, second, rest @ ..] = &tokens[..] else {
|
||||
return parse_list_expression(working_set, span, list_element_shape);
|
||||
};
|
||||
|
||||
if !working_set.get_span_contents(first.span).starts_with(b"[")
|
||||
|| second.contents != TokenContents::Semicolon
|
||||
|| rest.is_empty()
|
||||
{
|
||||
return parse_list_expression(working_set, span, list_element_shape);
|
||||
};
|
||||
}
|
||||
|
||||
let head = parse_table_row(working_set, first.span);
|
||||
|
||||
let errors = working_set.parse_errors.len();
|
||||
|
||||
let (head, rows) = match head {
|
||||
Ok((head, _)) => {
|
||||
if rest.is_empty() {
|
||||
working_set.error(ParseError::Expected(
|
||||
"table row",
|
||||
Span::new(second.span.end, second.span.end),
|
||||
));
|
||||
}
|
||||
|
||||
let rows = rest
|
||||
.iter()
|
||||
.filter_map(|it| {
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
#![allow(clippy::byte_char_slices)]
|
||||
|
||||
use crate::{
|
||||
lex, lite_parse,
|
||||
TokenContents, lex, lite_parse,
|
||||
parse_helpers::is_variable,
|
||||
parser::{ensure_not_reserved_variable_name, parse_value},
|
||||
};
|
||||
@@ -125,6 +125,18 @@ pub fn parse_list_pattern(working_set: &mut StateWorkingSet, span: Span) -> Matc
|
||||
working_set.error(err);
|
||||
}
|
||||
|
||||
if let Some(token) = output
|
||||
.iter()
|
||||
.find(|token| token.contents == TokenContents::Semicolon)
|
||||
{
|
||||
working_set.error(ParseError::LabeledErrorWithHelp {
|
||||
error: "Unexpected semicolon in list pattern".into(),
|
||||
label: "not a valid list separator".into(),
|
||||
help: "Use commas or whitespace to separate list items.".into(),
|
||||
span: token.span,
|
||||
});
|
||||
}
|
||||
|
||||
let (output, err) = lite_parse(&output, working_set);
|
||||
if let Some(err) = err {
|
||||
working_set.error(err);
|
||||
|
||||
@@ -3751,6 +3751,145 @@ fn conditional_branch_types(#[case] code: &str, #[case] expected_tys: &[Type]) {
|
||||
assert_eq!(out_ty, expected_ty);
|
||||
}
|
||||
|
||||
#[rstest]
|
||||
#[case::malformed_table("[a b c; [1 2 3]]")]
|
||||
#[case::between_items("[1; 2]")]
|
||||
#[case::before_newline("[1;\n2]")]
|
||||
#[case::before_variable("[1; $undefined]")]
|
||||
#[case::leading("[;1]")]
|
||||
#[case::trailing("[1;]")]
|
||||
#[case::empty("[;]")]
|
||||
#[case::repeated("[1;; 2]")]
|
||||
#[case::nested_list("[[1; 2]]")]
|
||||
#[case::external_argument("^foo [1; 2]")]
|
||||
#[case::table_header("[[a; b]; [1]]")]
|
||||
#[case::table_row("[[a b]; [1 2; 3 4]]")]
|
||||
fn reject_invalid_list_semicolons(#[case] source: &str) {
|
||||
let engine_state = EngineState::new();
|
||||
let mut working_set = StateWorkingSet::new(&engine_state);
|
||||
let _ = parse(&mut working_set, None, source.as_bytes(), false);
|
||||
|
||||
let error = working_set
|
||||
.parse_errors
|
||||
.first()
|
||||
.expect("a semicolon in a list must produce a parse error");
|
||||
assert!(
|
||||
matches!(error, ParseError::LabeledErrorWithHelp { error, .. }
|
||||
if error == "Unexpected semicolon in list"),
|
||||
"unexpected diagnostic: {error:?}"
|
||||
);
|
||||
assert_eq!(working_set.get_span_contents(error.span()), b";");
|
||||
}
|
||||
|
||||
#[rstest]
|
||||
#[case::plain("[[a b];]")]
|
||||
#[case::whitespace("[[a b]; \n]")]
|
||||
#[case::comment("[[a b]; # no rows\n]")]
|
||||
#[case::empty_header("[[];]")]
|
||||
fn table_without_rows_reports_missing_row(#[case] source: &str) {
|
||||
let engine_state = EngineState::new();
|
||||
let mut working_set = StateWorkingSet::new(&engine_state);
|
||||
let _ = parse(&mut working_set, None, source.as_bytes(), false);
|
||||
|
||||
assert!(
|
||||
matches!(
|
||||
working_set.parse_errors.as_slice(),
|
||||
[ParseError::Expected("table row", _)]
|
||||
),
|
||||
"unexpected diagnostics: {:?}",
|
||||
working_set.parse_errors
|
||||
);
|
||||
|
||||
let span = working_set.parse_errors[0].span();
|
||||
assert_eq!(span.start, span.end);
|
||||
assert!(span.start > 0);
|
||||
assert_eq!(
|
||||
working_set.get_span_contents(Span::new(span.start - 1, span.start)),
|
||||
b";"
|
||||
);
|
||||
}
|
||||
|
||||
#[rstest]
|
||||
#[case::double_quoted(r#"["a;b" 2]"#)]
|
||||
#[case::single_quoted("['a;b' 2]")]
|
||||
#[case::backtick_quoted("[`a;b` 2]")]
|
||||
#[case::raw_string("[r#'a;b'# 2]")]
|
||||
#[case::interpolation(r#"[$"(1; 2)" 3]"#)]
|
||||
#[case::comment("[1 # ; ignored\n 2]")]
|
||||
#[case::subexpression("[(1; 2) 3]")]
|
||||
#[case::closure("[{1; 2} 3]")]
|
||||
#[case::nested_table("[[[a]; [1]] 2]")]
|
||||
#[case::table_separator("[[a]; [1]]")]
|
||||
#[case::table_quoted_header(r#"[["a;b"]; [1]]"#)]
|
||||
#[case::table_subexpression_cell("[[a]; [(1; 2)]]")]
|
||||
#[case::table_nested_table_cell("[[a]; [[[b]; [1]]]]")]
|
||||
fn accept_valid_list_semicolons(#[case] source: &str) {
|
||||
let engine_state = EngineState::new();
|
||||
let mut working_set = StateWorkingSet::new(&engine_state);
|
||||
let _ = parse(&mut working_set, None, source.as_bytes(), false);
|
||||
|
||||
assert!(
|
||||
working_set.parse_errors.is_empty(),
|
||||
"{:?}",
|
||||
working_set.parse_errors
|
||||
);
|
||||
}
|
||||
|
||||
#[rstest]
|
||||
#[case::between_items("[1; 2]")]
|
||||
#[case::before_newline("[1;\n2]")]
|
||||
#[case::leading("[;1]")]
|
||||
#[case::trailing("[1;]")]
|
||||
#[case::empty("[;]")]
|
||||
#[case::repeated("[1;; 2]")]
|
||||
#[case::nested_list("[[1; 2]]")]
|
||||
#[case::nested_record("[{a: [1; 2]}]")]
|
||||
#[case::ignore_rest("[1 ..; 2]")]
|
||||
#[case::capture_rest("[1 ..$rest; 2]")]
|
||||
fn reject_invalid_list_pattern_semicolons(#[case] pattern: &str) {
|
||||
let engine_state = EngineState::new();
|
||||
let mut working_set = StateWorkingSet::new(&engine_state);
|
||||
working_set.add_decl(Box::new(MatchMocked));
|
||||
let source = format!("match [] {{ {pattern} => true, _ => false }}");
|
||||
let _ = parse(&mut working_set, None, source.as_bytes(), false);
|
||||
|
||||
let error = working_set
|
||||
.parse_errors
|
||||
.first()
|
||||
.expect("a semicolon in a list pattern must produce a parse error");
|
||||
assert!(
|
||||
matches!(error, ParseError::LabeledErrorWithHelp { error, .. }
|
||||
if error == "Unexpected semicolon in list pattern"),
|
||||
"unexpected diagnostic: {error:?}"
|
||||
);
|
||||
assert_eq!(working_set.get_span_contents(error.span()), b";");
|
||||
}
|
||||
|
||||
#[rstest]
|
||||
#[case::double_quoted(r#"["a;b" 2]"#)]
|
||||
#[case::single_quoted("['a;b' 2]")]
|
||||
#[case::backtick_quoted("[`a;b` 2]")]
|
||||
#[case::raw_string("[r#'a;b'# 2]")]
|
||||
#[case::comment("[1 # ; ignored\n 2]")]
|
||||
#[case::constant_subexpression("[(1; 2)]")]
|
||||
#[case::nested_list(r#"[["a;b"]]"#)]
|
||||
#[case::nested_record(r#"[{a: "b;c"}]"#)]
|
||||
#[case::ignore_rest("[1 ..]")]
|
||||
#[case::capture_rest("[1 ..$rest]")]
|
||||
fn accept_valid_list_pattern_semicolons(#[case] pattern: &str) {
|
||||
let engine_state = EngineState::new();
|
||||
let mut working_set = StateWorkingSet::new(&engine_state);
|
||||
working_set.add_decl(Box::new(MatchMocked));
|
||||
let source = format!("match [] {{ {pattern} => true, _ => false }}");
|
||||
let _ = parse(&mut working_set, None, source.as_bytes(), false);
|
||||
|
||||
assert!(
|
||||
working_set.parse_errors.is_empty(),
|
||||
"{:?}",
|
||||
working_set.parse_errors
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn record_semicolon_gives_separator_help() {
|
||||
let engine_state = EngineState::new();
|
||||
|
||||
@@ -9,7 +9,7 @@ the list is empty, this file goes away and so does the todo at the top of the sp
|
||||
|
||||
- [ ] bug 1 - a leading utf-8 bom is rejected. strip one instead.
|
||||
- [ ] bug 2 - the empty document decodes to `null`. error instead.
|
||||
- [ ] bug 3 - `[[a b];]` is a list containing the header, not an empty table. error instead.
|
||||
- [x] bug 3 - `[[a b];]` is a list containing the header, not an empty table. error instead.
|
||||
- [ ] bug 4 - an empty container under indentation emits a blank line. do not emit it.
|
||||
- [ ] bug 5 - `--raw --no-commas` emits no separator and loses data. make the two exclusive.
|
||||
- [ ] bug 6 - table width is measured in bytes but padded in runes. use one measure for both.
|
||||
@@ -51,7 +51,7 @@ byte for byte still has to reproduce these until they are fixed.
|
||||
```
|
||||
- not an empty table - a list containing the header list. an empty table is not expressible
|
||||
in the table form; `[]` must be used.
|
||||
- **reproduce**, for compatibility. documenting it as an intended feature would be dishonest.
|
||||
- **fixed**: the parser now recognizes this as an incomplete table and reports the missing row.
|
||||
|
||||
- bug 4 - an empty container under indentation emits a blank line.
|
||||
```nushell
|
||||
|
||||
@@ -86,7 +86,7 @@
|
||||
list := "[" ws* (value (sep value)* sep?)? ws* "]"
|
||||
record := "{" ws* (pair (sep pair)* sep?)? ws* "}"
|
||||
pair := key ws* ":" ws* value
|
||||
table := "[" ws* list ws* ";" ws* (list (sep list)* sep?)? ws* "]"
|
||||
table := "[" ws* list ws* ";" ws* list (sep list)* sep? ws* "]"
|
||||
binary := "0x[" (hex | sep)* "]" | "0b[" (bit | sep)* "]"
|
||||
cell_path := "$" ("." member)*
|
||||
member := (bare | '"' esc* '"' | "'" any* "'" | int) "?"?
|
||||
|
||||
Reference in new issue
Block a user