diff --git a/src/lang/modifyAst.ts b/src/lang/modifyAst.ts index e32207c83..5d33b075c 100644 --- a/src/lang/modifyAst.ts +++ b/src/lang/modifyAst.ts @@ -563,6 +563,7 @@ export function createArrayExpression( start: 0, end: 0, digest: null, + nonCodeMeta: { nonCodeNodes: {}, start: [], digest: null }, elements, } } diff --git a/src/wasm-lib/kcl/src/ast/types.rs b/src/wasm-lib/kcl/src/ast/types.rs index d950e594d..0ed1afe46 100644 --- a/src/wasm-lib/kcl/src/ast/types.rs +++ b/src/wasm-lib/kcl/src/ast/types.rs @@ -1149,6 +1149,15 @@ pub enum NonCodeValue { NewLine, } +impl NonCodeValue { + fn should_cause_array_newline(&self) -> bool { + match self { + Self::InlineComment { .. } => false, + Self::Shebang { .. } | Self::BlockComment { .. } | Self::NewLineBlockComment { .. } | Self::NewLine => true, + } + } +} + #[derive(Debug, Default, Clone, Serialize, PartialEq, ts_rs::TS, JsonSchema, Bake)] #[databake(path = kcl_lib::ast::types)] #[ts(export)] @@ -1160,6 +1169,18 @@ pub struct NonCodeMeta { pub digest: Option, } +impl NonCodeMeta { + /// Does this contain anything? + pub fn is_empty(&self) -> bool { + self.non_code_nodes.is_empty() && self.start.is_empty() + } + + /// How many non-code values does this have? + pub fn non_code_nodes_len(&self) -> usize { + self.non_code_nodes.values().map(|x| x.len()).sum() + } +} + // implement Deserialize manually because we to force the keys of non_code_nodes to be usize // and by default the ts type { [statementIndex: number]: NonCodeNode } serializes to a string i.e. "0", "1", etc. impl<'de> Deserialize<'de> for NonCodeMeta { @@ -2224,11 +2245,13 @@ impl From for Expr { #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, ts_rs::TS, JsonSchema, Bake)] #[databake(path = kcl_lib::ast::types)] #[ts(export)] -#[serde(tag = "type")] +#[serde(rename_all = "camelCase", tag = "type")] pub struct ArrayExpression { pub start: usize, pub end: usize, pub elements: Vec, + #[serde(default, skip_serializing_if = "NonCodeMeta::is_empty")] + pub non_code_meta: NonCodeMeta, pub digest: Option, } @@ -2247,6 +2270,7 @@ impl ArrayExpression { start: 0, end: 0, elements, + non_code_meta: Default::default(), digest: None, } } @@ -2280,38 +2304,70 @@ impl ArrayExpression { } fn recast(&self, options: &FormatOptions, indentation_level: usize, is_in_pipe: bool) -> String { - let flat_recast = format!( - "[{}]", - self.elements - .iter() - .map(|el| el.recast(options, 0, false)) - .collect::>() - .join(", ") - ); - let max_array_length = 40; - if flat_recast.len() > max_array_length { - let inner_indentation = if is_in_pipe { - options.get_indentation_offset_pipe(indentation_level + 1) - } else { - options.get_indentation(indentation_level + 1) - }; - format!( - "[\n{}{}\n{}]", - inner_indentation, - self.elements - .iter() - .map(|el| el.recast(options, indentation_level, is_in_pipe)) - .collect::>() - .join(format!(",\n{}", inner_indentation).as_str()), - if is_in_pipe { - options.get_indentation_offset_pipe(indentation_level) + // Reconstruct the order of items in the array. + // An item can be an element (i.e. an expression for a KCL value), + // or a non-code item (e.g. a comment) + let num_items = self.elements.len() + self.non_code_meta.non_code_nodes_len(); + let mut elems = self.elements.iter(); + let mut found_line_comment = false; + let mut format_items: Vec<_> = (0..num_items) + .flat_map(|i| { + if let Some(noncode) = self.non_code_meta.non_code_nodes.get(&i) { + noncode + .iter() + .map(|nc| { + found_line_comment |= nc.value.should_cause_array_newline(); + nc.format("") + }) + .collect::>() } else { - options.get_indentation(indentation_level) - }, - ) - } else { - flat_recast + let el = elems.next().unwrap(); + let s = format!("{}, ", el.recast(options, 0, false)); + vec![s] + } + }) + .collect(); + + // Format these items into a one-line array. + if let Some(item) = format_items.last_mut() { + if let Some(norm) = item.strip_suffix(", ") { + *item = norm.to_owned(); + } } + let format_items = format_items; // Remove mutability + let flat_recast = format!("[{}]", format_items.join("")); + + // We might keep the one-line representation, if it's short enough. + let max_array_length = 40; + let multi_line = flat_recast.len() > max_array_length || found_line_comment; + if !multi_line { + return flat_recast; + } + + // Otherwise, we format a multi-line representation. + let inner_indentation = if is_in_pipe { + options.get_indentation_offset_pipe(indentation_level + 1) + } else { + options.get_indentation(indentation_level + 1) + }; + let formatted_array_lines = format_items + .iter() + .map(|s| { + format!( + "{inner_indentation}{}{}", + if let Some(x) = s.strip_suffix(" ") { x } else { s }, + if s.ends_with('\n') { "" } else { "\n" } + ) + }) + .collect::>() + .join("") + .to_owned(); + let end_indent = if is_in_pipe { + options.get_indentation_offset_pipe(indentation_level) + } else { + options.get_indentation(indentation_level) + }; + format!("[\n{formatted_array_lines}{end_indent}]") } /// Returns a hover value that includes the given character position. @@ -5838,6 +5894,103 @@ const thickness = sqrt(distance * p * FOS * 6 / (sigmaAllow * width))"#; } } + #[test] + fn recast_array_with_comments() { + use winnow::Parser; + for (i, (input, expected, reason)) in [ + ( + "\ +[ + 1, + 2, + 3, + 4, + 5, + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16, + 17, + 18, + 19, + 20, +]", + "\ +[ + 1, + 2, + 3, + 4, + 5, + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16, + 17, + 18, + 19, + 20 +]", + "preserves multi-line arrays", + ), + ( + "\ +[ + 1, + // 2, + 3 +]", + "\ +[ + 1, + // 2, + 3 +]", + "preserves comments", + ), + ( + "\ +[ + 1, + 2, + // 3 +]", + "\ +[ + 1, + 2, + // 3 +]", + "preserves comments at the end of the array", + ), + ] + .into_iter() + .enumerate() + { + let tokens = crate::token::lexer(input).unwrap(); + let expr = crate::parser::parser_impl::array_elem_by_elem.parse(&tokens).unwrap(); + assert_eq!( + expr.recast(&FormatOptions::new(), 0, false), + expected, + "failed test {i}, which is testing that recasting {reason}" + ); + } + } + #[test] fn required_params() { for (i, (test_name, expected, function_expr)) in [ diff --git a/src/wasm-lib/kcl/src/parser/parser_impl.rs b/src/wasm-lib/kcl/src/parser/parser_impl.rs index eb44e81ec..1a4644140 100644 --- a/src/wasm-lib/kcl/src/parser/parser_impl.rs +++ b/src/wasm-lib/kcl/src/parser/parser_impl.rs @@ -1,4 +1,4 @@ -use std::str::FromStr; +use std::{collections::HashMap, str::FromStr}; use winnow::{ combinator::{alt, delimited, opt, peek, preceded, repeat, separated, terminated}, @@ -448,14 +448,91 @@ fn equals(i: TokenSlice) -> PResult { .parse_next(i) } +#[allow(clippy::large_enum_variant)] +pub enum NonCodeOr { + NonCode(NonCodeNode), + Code(T), +} + /// Parse a KCL array of elements. fn array(i: TokenSlice) -> PResult { + alt((array_empty, array_elem_by_elem, array_end_start)).parse_next(i) +} + +/// Match an empty array. +fn array_empty(i: TokenSlice) -> PResult { let start = open_bracket(i)?.start; ignore_whitespace(i); - let elements = alt((integer_range, separated(0.., expression, comma_sep))) - .context(expected( - "array contents, either a numeric range (like 0..10) or a list of elements (like [1, 2, 3])", - )) + let end = close_bracket(i)?.end; + Ok(ArrayExpression { + start, + end, + elements: Default::default(), + non_code_meta: Default::default(), + digest: None, + }) +} + +/// Match something that separates elements of an array. +fn array_separator(i: TokenSlice) -> PResult<()> { + alt(( + // Normally you need a comma. + comma_sep, + // But, if the array is ending, no need for a comma. + peek(preceded(opt(whitespace), close_bracket)).void(), + )) + .parse_next(i) +} + +pub(crate) fn array_elem_by_elem(i: TokenSlice) -> PResult { + let start = open_bracket(i)?.start; + ignore_whitespace(i); + let elements: Vec<_> = repeat( + 0.., + alt(( + terminated(expression.map(NonCodeOr::Code), array_separator), + terminated(non_code_node.map(NonCodeOr::NonCode), whitespace), + )), + ) + .context(expected("array contents, a list of elements (like [1, 2, 3])")) + .parse_next(i)?; + ignore_whitespace(i); + let end = close_bracket(i)?.end; + + // Sort the array's elements (i.e. expression nodes) from the noncode nodes. + let (elements, non_code_nodes): (Vec<_>, HashMap) = elements.into_iter().enumerate().fold( + (Vec::new(), HashMap::new()), + |(mut elements, mut non_code_nodes), (i, e)| { + match e { + NonCodeOr::NonCode(x) => { + non_code_nodes.insert(i, vec![x]); + } + NonCodeOr::Code(x) => { + elements.push(x); + } + } + (elements, non_code_nodes) + }, + ); + let non_code_meta = NonCodeMeta { + non_code_nodes, + start: Vec::new(), + digest: None, + }; + Ok(ArrayExpression { + start, + end, + elements, + non_code_meta, + digest: None, + }) +} + +fn array_end_start(i: TokenSlice) -> PResult { + let start = open_bracket(i)?.start; + ignore_whitespace(i); + let elements = integer_range + .context(expected("array contents, a numeric range (like 0..10)")) .parse_next(i)?; ignore_whitespace(i); let end = close_bracket(i)?.end; @@ -463,6 +540,7 @@ fn array(i: TokenSlice) -> PResult { start, end, elements, + non_code_meta: Default::default(), digest: None, }) } @@ -2779,6 +2857,7 @@ e init: Expr::ArrayExpression(Box::new(ArrayExpression { start: 16, end: 23, + non_code_meta: Default::default(), elements: vec![ Expr::Literal(Box::new(Literal { start: 17, @@ -2956,6 +3035,45 @@ e let _ast = parser.ast().unwrap(); } + #[test] + fn array() { + let program = r#"[1, 2, 3]"#; + let tokens = crate::token::lexer(program).unwrap(); + let mut sl: &[Token] = &tokens; + let _arr = array_elem_by_elem(&mut sl).unwrap(); + } + + #[test] + fn array_linesep_trailing_comma() { + let program = r#"[ + 1, + 2, + 3, + ]"#; + let tokens = crate::token::lexer(program).unwrap(); + let mut sl: &[Token] = &tokens; + let _arr = array_elem_by_elem(&mut sl).unwrap(); + } + + #[allow(unused)] + fn print_tokens(tokens: &[Token]) { + for (i, tok) in tokens.iter().enumerate() { + println!("{i:.2}: ({:?}):) '{}'", tok.token_type, tok.value.replace("\n", "\\n")); + } + } + + #[test] + fn array_linesep_no_trailing_comma() { + let program = r#"[ + 1, + 2, + 3 + ]"#; + let tokens = crate::token::lexer(program).unwrap(); + let mut sl: &[Token] = &tokens; + let _arr = array_elem_by_elem(&mut sl).unwrap(); + } + #[test] fn test_keyword_ok_in_fn_args_return() { let some_program_string = r#"fn thing = (param) => { @@ -3145,7 +3263,11 @@ mod snapshot_tests { Ok(x) => x, Err(e) => panic!("could not parse test: {e:?}"), }; - insta::assert_json_snapshot!(actual); + let mut settings = insta::Settings::clone_current(); + settings.set_sort_maps(true); + settings.bind(|| { + insta::assert_json_snapshot!(actual); + }); } }; } @@ -3264,4 +3386,22 @@ mod snapshot_tests { snapshot_test!(at, "line([0, l], %)"); snapshot_test!(au, include_str!("../../../tests/executor/inputs/cylinder.kcl")); snapshot_test!(av, "fn f = (angle?) => { return default(angle, 360) }"); + snapshot_test!( + aw, + "let numbers = [ + 1, + // A, + // B, + 3, + ]" + ); + snapshot_test!( + ax, + "let numbers = [ + 1, + 2, + // A, + // B, + ]" + ); } diff --git a/src/wasm-lib/kcl/src/parser/snapshots/kcl_lib__parser__parser_impl__snapshot_tests__aw.snap b/src/wasm-lib/kcl/src/parser/snapshots/kcl_lib__parser__parser_impl__snapshot_tests__aw.snap new file mode 100644 index 000000000..ce26f5afe --- /dev/null +++ b/src/wasm-lib/kcl/src/parser/snapshots/kcl_lib__parser__parser_impl__snapshot_tests__aw.snap @@ -0,0 +1,98 @@ +--- +source: kcl/src/parser/parser_impl.rs +expression: actual +--- +{ + "start": 0, + "end": 91, + "body": [ + { + "type": "VariableDeclaration", + "type": "VariableDeclaration", + "start": 0, + "end": 91, + "declarations": [ + { + "type": "VariableDeclarator", + "start": 4, + "end": 91, + "id": { + "type": "Identifier", + "start": 4, + "end": 11, + "name": "numbers", + "digest": null + }, + "init": { + "type": "ArrayExpression", + "type": "ArrayExpression", + "start": 14, + "end": 91, + "elements": [ + { + "type": "Literal", + "type": "Literal", + "start": 28, + "end": 29, + "value": 1, + "raw": "1", + "digest": null + }, + { + "type": "Literal", + "type": "Literal", + "start": 79, + "end": 80, + "value": 3, + "raw": "3", + "digest": null + } + ], + "nonCodeMeta": { + "nonCodeNodes": { + "1": [ + { + "type": "NonCodeNode", + "start": 43, + "end": 48, + "value": { + "type": "blockComment", + "value": "A,", + "style": "line" + }, + "digest": null + } + ], + "2": [ + { + "type": "NonCodeNode", + "start": 61, + "end": 66, + "value": { + "type": "blockComment", + "value": "B,", + "style": "line" + }, + "digest": null + } + ] + }, + "start": [], + "digest": null + }, + "digest": null + }, + "digest": null + } + ], + "kind": "let", + "digest": null + } + ], + "nonCodeMeta": { + "nonCodeNodes": {}, + "start": [], + "digest": null + }, + "digest": null +} diff --git a/src/wasm-lib/kcl/src/parser/snapshots/kcl_lib__parser__parser_impl__snapshot_tests__ax.snap b/src/wasm-lib/kcl/src/parser/snapshots/kcl_lib__parser__parser_impl__snapshot_tests__ax.snap new file mode 100644 index 000000000..d99eb5a5a --- /dev/null +++ b/src/wasm-lib/kcl/src/parser/snapshots/kcl_lib__parser__parser_impl__snapshot_tests__ax.snap @@ -0,0 +1,98 @@ +--- +source: kcl/src/parser/parser_impl.rs +expression: actual +--- +{ + "start": 0, + "end": 91, + "body": [ + { + "type": "VariableDeclaration", + "type": "VariableDeclaration", + "start": 0, + "end": 91, + "declarations": [ + { + "type": "VariableDeclarator", + "start": 4, + "end": 91, + "id": { + "type": "Identifier", + "start": 4, + "end": 11, + "name": "numbers", + "digest": null + }, + "init": { + "type": "ArrayExpression", + "type": "ArrayExpression", + "start": 14, + "end": 91, + "elements": [ + { + "type": "Literal", + "type": "Literal", + "start": 28, + "end": 29, + "value": 1, + "raw": "1", + "digest": null + }, + { + "type": "Literal", + "type": "Literal", + "start": 43, + "end": 44, + "value": 2, + "raw": "2", + "digest": null + } + ], + "nonCodeMeta": { + "nonCodeNodes": { + "2": [ + { + "type": "NonCodeNode", + "start": 58, + "end": 63, + "value": { + "type": "blockComment", + "value": "A,", + "style": "line" + }, + "digest": null + } + ], + "3": [ + { + "type": "NonCodeNode", + "start": 76, + "end": 81, + "value": { + "type": "blockComment", + "value": "B,", + "style": "line" + }, + "digest": null + } + ] + }, + "start": [], + "digest": null + }, + "digest": null + }, + "digest": null + } + ], + "kind": "let", + "digest": null + } + ], + "nonCodeMeta": { + "nonCodeNodes": {}, + "start": [], + "digest": null + }, + "digest": null +} diff --git a/src/wasm-lib/kcl/src/std/patterns.rs b/src/wasm-lib/kcl/src/std/patterns.rs index 7cc07a265..5929e1568 100644 --- a/src/wasm-lib/kcl/src/std/patterns.rs +++ b/src/wasm-lib/kcl/src/std/patterns.rs @@ -254,7 +254,7 @@ async fn make_transform<'a>( } fn array_to_point3d(json: &serde_json::Value, source_ranges: Vec) -> Result { - let serde_json::Value::Array(arr) = dbg!(json) else { + let serde_json::Value::Array(arr) = json else { return Err(KclError::Semantic(KclErrorDetails { message: "Expected an array of 3 numbers (i.e. a 3D point)".to_string(), source_ranges,