From e16ecc28a3a33267c0c47b081ff344505f535538 Mon Sep 17 00:00:00 2001 From: Adam Chalmers Date: Thu, 22 Aug 2024 13:54:59 -0500 Subject: [PATCH] KCL parser: Allow comments in multi-line object expression (#3607) Like my previous PR to array expressions (https://github.com/KittyCAD/modeling-app/pull/3539), but for object expressions. Closes https://github.com/KittyCAD/modeling-app/issues/1528 --- src/lang/modifyAst.ts | 9 +- src/wasm-lib/kcl/src/ast/types.rs | 141 ++++++++++++++---- src/wasm-lib/kcl/src/parser/parser_impl.rs | 81 ++++++++-- ...rser__parser_impl__snapshot_tests__ay.snap | 111 ++++++++++++++ ...rser__parser_impl__snapshot_tests__az.snap | 111 ++++++++++++++ 5 files changed, 411 insertions(+), 42 deletions(-) create mode 100644 src/wasm-lib/kcl/src/parser/snapshots/kcl_lib__parser__parser_impl__snapshot_tests__ay.snap create mode 100644 src/wasm-lib/kcl/src/parser/snapshots/kcl_lib__parser__parser_impl__snapshot_tests__az.snap diff --git a/src/lang/modifyAst.ts b/src/lang/modifyAst.ts index 198493f629..8f985df58c 100644 --- a/src/lang/modifyAst.ts +++ b/src/lang/modifyAst.ts @@ -563,7 +563,7 @@ export function createArrayExpression( start: 0, end: 0, digest: null, - nonCodeMeta: { nonCodeNodes: {}, start: [], digest: null }, + nonCodeMeta: nonCodeMetaEmpty(), elements, } } @@ -577,7 +577,7 @@ export function createPipeExpression( end: 0, digest: null, body, - nonCodeMeta: { nonCodeNodes: {}, start: [], digest: null }, + nonCodeMeta: nonCodeMetaEmpty(), } } @@ -613,6 +613,7 @@ export function createObjectExpression(properties: { start: 0, end: 0, digest: null, + nonCodeMeta: nonCodeMetaEmpty(), properties: Object.entries(properties).map(([key, value]) => ({ type: 'ObjectProperty', start: 0, @@ -1065,3 +1066,7 @@ export async function deleteFromSelection( return new Error('Selection not recognised, could not delete') } + +const nonCodeMetaEmpty = () => { + return { nonCodeNodes: {}, start: [], digest: null } +} diff --git a/src/wasm-lib/kcl/src/ast/types.rs b/src/wasm-lib/kcl/src/ast/types.rs index 35f4833ea2..37eeeb87f4 100644 --- a/src/wasm-lib/kcl/src/ast/types.rs +++ b/src/wasm-lib/kcl/src/ast/types.rs @@ -2466,11 +2466,13 @@ impl ArrayExpression { #[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 ObjectExpression { pub start: usize, pub end: usize, pub properties: Vec, + #[serde(default, skip_serializing_if = "NonCodeMeta::is_empty")] + pub non_code_meta: NonCodeMeta, pub digest: Option, } @@ -2481,6 +2483,7 @@ impl ObjectExpression { start: 0, end: 0, properties, + non_code_meta: Default::default(), digest: None, } } @@ -2514,6 +2517,14 @@ impl ObjectExpression { } fn recast(&self, options: &FormatOptions, indentation_level: usize, is_in_pipe: bool) -> String { + if self + .non_code_meta + .non_code_nodes + .values() + .any(|nc| nc.iter().any(|nc| nc.value.should_cause_array_newline())) + { + return self.recast_multi_line(options, indentation_level, is_in_pipe); + } let flat_recast = format!( "{{ {} }}", self.properties @@ -2529,35 +2540,49 @@ impl ObjectExpression { .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.properties - .iter() - .map(|prop| { - format!( - "{}: {}", - prop.key.name, - prop.value.recast(options, indentation_level + 1, is_in_pipe) - ) - }) - .collect::>() - .join(format!(",\n{}", inner_indentation).as_str()), - if is_in_pipe { - options.get_indentation_offset_pipe(indentation_level) + let needs_multiple_lines = flat_recast.len() > max_array_length; + if !needs_multiple_lines { + return flat_recast; + } + self.recast_multi_line(options, indentation_level, is_in_pipe) + } + + /// Recast, but always outputs the object with newlines between each property. + fn recast_multi_line(&self, options: &FormatOptions, indentation_level: usize, is_in_pipe: bool) -> String { + let inner_indentation = if is_in_pipe { + options.get_indentation_offset_pipe(indentation_level + 1) + } else { + options.get_indentation(indentation_level + 1) + }; + let num_items = self.properties.len() + self.non_code_meta.non_code_nodes_len(); + let mut props = self.properties.iter(); + let 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| nc.format("")).collect::>() } else { - options.get_indentation(indentation_level) - }, - ) + let prop = props.next().unwrap(); + // Use a comma unless it's the last item + let comma = if i == num_items - 1 { "" } else { ",\n" }; + let s = format!( + "{}: {}{comma}", + prop.key.name, + prop.value.recast(options, indentation_level + 1, is_in_pipe).trim() + ); + vec![s] + } + }) + .collect(); + dbg!(&format_items); + let end_indent = if is_in_pipe { + options.get_indentation_offset_pipe(indentation_level) } else { - flat_recast - } + options.get_indentation(indentation_level) + }; + format!( + "{{\n{inner_indentation}{}\n{end_indent}}}", + format_items.join(&inner_indentation), + ) } /// Returns a hover value that includes the given character position. @@ -5897,6 +5922,66 @@ const thickness = sqrt(distance * p * FOS * 6 / (sigmaAllow * width))"#; } } + #[test] + fn recast_objects_no_comments() { + let input = r#" +const sketch002 = startSketchOn({ + plane: { + origin: { x: 1, y: 2, z: 3 }, + x_axis: { x: 4, y: 5, z: 6 }, + y_axis: { x: 7, y: 8, z: 9 }, + z_axis: { x: 10, y: 11, z: 12 } + } + }) +"#; + let expected = r#"const sketch002 = startSketchOn({ + plane: { + origin: { x: 1, y: 2, z: 3 }, + x_axis: { x: 4, y: 5, z: 6 }, + y_axis: { x: 7, y: 8, z: 9 }, + z_axis: { x: 10, y: 11, z: 12 } + } +}) +"#; + let tokens = crate::token::lexer(input).unwrap(); + let p = crate::parser::Parser::new(tokens); + let ast = p.ast().unwrap(); + let actual = ast.recast(&FormatOptions::new(), 0); + assert_eq!(actual, expected); + } + + #[test] + fn recast_objects_with_comments() { + use winnow::Parser; + for (i, (input, expected, reason)) in [( + "\ +{ + a: 1, + // b: 2, + c: 3 +}", + "\ +{ + a: 1, + // b: 2, + c: 3 +}", + "preserves comments", + )] + .into_iter() + .enumerate() + { + let tokens = crate::token::lexer(input).unwrap(); + crate::parser::parser_impl::print_tokens(&tokens); + let expr = crate::parser::parser_impl::object.parse(&tokens).unwrap(); + assert_eq!( + expr.recast(&FormatOptions::new(), 0, false), + expected, + "failed test {i}, which is testing that recasting {reason}" + ); + } + } + #[test] fn recast_array_with_comments() { use winnow::Parser; diff --git a/src/wasm-lib/kcl/src/parser/parser_impl.rs b/src/wasm-lib/kcl/src/parser/parser_impl.rs index 1a46441401..93d6043a11 100644 --- a/src/wasm-lib/kcl/src/parser/parser_impl.rs +++ b/src/wasm-lib/kcl/src/parser/parser_impl.rs @@ -586,22 +586,60 @@ fn object_property(i: TokenSlice) -> PResult { }) } +/// Match something that separates properties of an object. +fn property_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_brace)).void(), + )) + .parse_next(i) +} + /// Parse a KCL object value. -fn object(i: TokenSlice) -> PResult { +pub(crate) fn object(i: TokenSlice) -> PResult { let start = open_brace(i)?.start; ignore_whitespace(i); - let properties = separated(0.., object_property, comma_sep) - .context(expected( - "a comma-separated list of key-value pairs, e.g. 'height: 4, width: 3'", - )) - .parse_next(i)?; + let properties: Vec<_> = repeat( + 0.., + alt(( + terminated(non_code_node.map(NonCodeOr::NonCode), whitespace), + terminated(object_property, property_separator).map(NonCodeOr::Code), + )), + ) + .context(expected( + "a comma-separated list of key-value pairs, e.g. 'height: 4, width: 3'", + )) + .parse_next(i)?; + + // Sort the object's properties from the noncode nodes. + let (properties, non_code_nodes): (Vec<_>, HashMap) = properties.into_iter().enumerate().fold( + (Vec::new(), HashMap::new()), + |(mut properties, mut non_code_nodes), (i, e)| { + match e { + NonCodeOr::NonCode(x) => { + non_code_nodes.insert(i, vec![x]); + } + NonCodeOr::Code(x) => { + properties.push(x); + } + } + (properties, non_code_nodes) + }, + ); ignore_trailing_comma(i); ignore_whitespace(i); let end = close_brace(i)?.end; + let non_code_meta = NonCodeMeta { + non_code_nodes, + ..Default::default() + }; Ok(ObjectExpression { start, end, properties, + non_code_meta, digest: None, }) } @@ -3056,12 +3094,6 @@ e } #[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#"[ @@ -3259,6 +3291,7 @@ mod snapshot_tests { #[test] fn $func_name() { let tokens = crate::token::lexer($test_kcl_program).unwrap(); + print_tokens(&tokens); let actual = match program.parse(&tokens) { Ok(x) => x, Err(e) => panic!("could not parse test: {e:?}"), @@ -3404,4 +3437,28 @@ mod snapshot_tests { // B, ]" ); + snapshot_test!( + ay, + "let props = { + a: 1, + // b: 2, + c: 3, + }" + ); + snapshot_test!( + az, + "let props = { + a: 1, + // b: 2, + c: 3 + }" + ); +} + +#[allow(unused)] +#[cfg(test)] +pub(crate) fn print_tokens(tokens: &[Token]) { + for (i, tok) in tokens.iter().enumerate() { + println!("{i:.2}: ({:?}):) '{}'", tok.token_type, tok.value.replace("\n", "\\n")); + } } diff --git a/src/wasm-lib/kcl/src/parser/snapshots/kcl_lib__parser__parser_impl__snapshot_tests__ay.snap b/src/wasm-lib/kcl/src/parser/snapshots/kcl_lib__parser__parser_impl__snapshot_tests__ay.snap new file mode 100644 index 0000000000..6ceee08761 --- /dev/null +++ b/src/wasm-lib/kcl/src/parser/snapshots/kcl_lib__parser__parser_impl__snapshot_tests__ay.snap @@ -0,0 +1,111 @@ +--- +source: kcl/src/parser/parser_impl.rs +expression: actual +--- +{ + "start": 0, + "end": 80, + "body": [ + { + "type": "VariableDeclaration", + "type": "VariableDeclaration", + "start": 0, + "end": 80, + "declarations": [ + { + "type": "VariableDeclarator", + "start": 4, + "end": 80, + "id": { + "type": "Identifier", + "start": 4, + "end": 9, + "name": "props", + "digest": null + }, + "init": { + "type": "ObjectExpression", + "type": "ObjectExpression", + "start": 12, + "end": 80, + "properties": [ + { + "type": "ObjectProperty", + "start": 26, + "end": 30, + "key": { + "type": "Identifier", + "start": 26, + "end": 27, + "name": "a", + "digest": null + }, + "value": { + "type": "Literal", + "type": "Literal", + "start": 29, + "end": 30, + "value": 1, + "raw": "1", + "digest": null + }, + "digest": null + }, + { + "type": "ObjectProperty", + "start": 65, + "end": 69, + "key": { + "type": "Identifier", + "start": 65, + "end": 66, + "name": "c", + "digest": null + }, + "value": { + "type": "Literal", + "type": "Literal", + "start": 68, + "end": 69, + "value": 3, + "raw": "3", + "digest": null + }, + "digest": null + } + ], + "nonCodeMeta": { + "nonCodeNodes": { + "1": [ + { + "type": "NonCodeNode", + "start": 44, + "end": 52, + "value": { + "type": "blockComment", + "value": "b: 2,", + "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__az.snap b/src/wasm-lib/kcl/src/parser/snapshots/kcl_lib__parser__parser_impl__snapshot_tests__az.snap new file mode 100644 index 0000000000..0241231ec1 --- /dev/null +++ b/src/wasm-lib/kcl/src/parser/snapshots/kcl_lib__parser__parser_impl__snapshot_tests__az.snap @@ -0,0 +1,111 @@ +--- +source: kcl/src/parser/parser_impl.rs +expression: actual +--- +{ + "start": 0, + "end": 79, + "body": [ + { + "type": "VariableDeclaration", + "type": "VariableDeclaration", + "start": 0, + "end": 79, + "declarations": [ + { + "type": "VariableDeclarator", + "start": 4, + "end": 79, + "id": { + "type": "Identifier", + "start": 4, + "end": 9, + "name": "props", + "digest": null + }, + "init": { + "type": "ObjectExpression", + "type": "ObjectExpression", + "start": 12, + "end": 79, + "properties": [ + { + "type": "ObjectProperty", + "start": 26, + "end": 30, + "key": { + "type": "Identifier", + "start": 26, + "end": 27, + "name": "a", + "digest": null + }, + "value": { + "type": "Literal", + "type": "Literal", + "start": 29, + "end": 30, + "value": 1, + "raw": "1", + "digest": null + }, + "digest": null + }, + { + "type": "ObjectProperty", + "start": 65, + "end": 69, + "key": { + "type": "Identifier", + "start": 65, + "end": 66, + "name": "c", + "digest": null + }, + "value": { + "type": "Literal", + "type": "Literal", + "start": 68, + "end": 69, + "value": 3, + "raw": "3", + "digest": null + }, + "digest": null + } + ], + "nonCodeMeta": { + "nonCodeNodes": { + "1": [ + { + "type": "NonCodeNode", + "start": 44, + "end": 52, + "value": { + "type": "blockComment", + "value": "b: 2,", + "style": "line" + }, + "digest": null + } + ] + }, + "start": [], + "digest": null + }, + "digest": null + }, + "digest": null + } + ], + "kind": "let", + "digest": null + } + ], + "nonCodeMeta": { + "nonCodeNodes": {}, + "start": [], + "digest": null + }, + "digest": null +}