Merge branch 'nadro/gh-5157/center-rectangle-kcl-fix' into achalmers/kw-fn-sketches

e2e/playwright/sketch-tests.spec.ts

@@ -99,6 +99,7 @@ test.describe('Sketch tests', () => {
   test('Can delete most of a sketch and the line tool will still work', async ({
     page,
     homePage,
+    scene,
   }) => {
     const u = await getUtils(page)
     await page.addInitScript(async () => {
@@ -112,12 +113,13 @@ test.describe('Sketch tests', () => {
     })
 
     await homePage.goToModelingScene()
+    await scene.waitForExecutionDone()
 
     await expect(async () => {
       await page.getByText('tangentialArcTo([24.95, -5.38], %)').click()
       await expect(
         page.getByRole('button', { name: 'Edit Sketch' })
-      ).toBeEnabled({ timeout: 1000 })
+      ).toBeEnabled({ timeout: 2000 })
       await page.getByRole('button', { name: 'Edit Sketch' }).click()
     }).toPass({ timeout: 40_000, intervals: [1_000] })
 

src/lang/modifyAst.test.ts

@@ -5,6 +5,8 @@ import {
   Identifier,
   SourceRange,
   topLevelRange,
+  LiteralValue,
+  Literal,
 } from './wasm'
 import {
   createLiteral,
@@ -37,10 +39,26 @@ beforeAll(async () => {
 })
 
 describe('Testing createLiteral', () => {
-  it('should create a literal', () => {
+  it('should create a literal number without units', () => {
     const result = createLiteral(5)
     expect(result.type).toBe('Literal')
     expect((result as any).value.value).toBe(5)
+    expect((result as any).value.suffix).toBe('None')
+    expect((result as Literal).raw).toBe('5')
+  })
+  it('should create a literal number with units', () => {
+    const lit: LiteralValue = { value: 5, suffix: 'Mm' }
+    const result = createLiteral(lit)
+    expect(result.type).toBe('Literal')
+    expect((result as any).value.value).toBe(5)
+    expect((result as any).value.suffix).toBe('Mm')
+    expect((result as Literal).raw).toBe('5mm')
+  })
+  it('should create a literal boolean', () => {
+    const result = createLiteral(false)
+    expect(result.type).toBe('Literal')
+    expect((result as Literal).value).toBe(false)
+    expect((result as Literal).raw).toBe('false')
   })
 })
 describe('Testing createIdentifier', () => {

src/lang/modifyAst.ts

@@ -22,6 +22,7 @@ import {
   SourceRange,
   sketchFromKclValue,
   isPathToNodeNumber,
+  formatNumber,
 } from './wasm'
 import {
   isNodeSafeToReplacePath,
@@ -780,11 +781,26 @@ export function splitPathAtPipeExpression(pathToNode: PathToNode): {
   return splitPathAtPipeExpression(pathToNode.slice(0, -1))
 }
 
+/**
+ * Note: This depends on WASM, but it's not async.  Callers are responsible for
+ * awaiting init of the WASM module.
+ */
 export function createLiteral(value: LiteralValue | number): Node<Literal> {
-  const raw = `${value}`
   if (typeof value === 'number') {
     value = { value, suffix: 'None' }
   }
+  let raw: string
+  if (typeof value === 'string') {
+    // TODO: Should we handle escape sequences?
+    raw = `${value}`
+  } else if (typeof value === 'boolean') {
+    raw = `${value}`
+  } else if (typeof value.value === 'number' && value.suffix === 'None') {
+    // Fast path for numbers when there are no units.
+    raw = `${value.value}`
+  } else {
+    raw = formatNumber(value.value, value.suffix)
+  }
   return {
     type: 'Literal',
     start: 0,

src/lang/util.ts

@@ -8,6 +8,8 @@ import {
   ArtifactGraph,
   CallExpressionKw,
   Expr,
+  LiteralValue,
+  NumericSuffix,
 } from './wasm'
 import { filterArtifacts } from 'lang/std/artifactGraph'
 import { isOverlap } from 'lib/utils'
@@ -111,3 +113,15 @@ export function findKwArgAnyIndex(
 export function isAbsolute(call: CallExpressionKw): boolean {
   return findKwArgAny(['endAbsolute'], call) !== undefined
 }
+
+export function isLiteralValueNumber(
+  e: LiteralValue
+): e is { value: number; suffix: NumericSuffix } {
+  return (
+    typeof e === 'object' &&
+    'value' in e &&
+    typeof e.value === 'number' &&
+    'suffix' in e &&
+    typeof e.suffix === 'string'
+  )
+}

src/lang/wasm.test.ts

@@ -1,5 +1,5 @@
 import { err } from 'lib/trap'
-import { initPromise, parse, ParseResult } from './wasm'
+import { formatNumber, initPromise, parse, ParseResult } from './wasm'
 import { enginelessExecutor } from 'lib/testHelpers'
 import { Node } from 'wasm-lib/kcl/bindings/Node'
 import { Program } from '../wasm-lib/kcl/bindings/Program'
@@ -20,3 +20,12 @@ it('can execute parsed AST', async () => {
   expect(err(execState)).toEqual(false)
   expect(execState.memory.get('x')?.value).toEqual(1)
 })
+
+it('formats numbers with units', () => {
+  expect(formatNumber(1, 'None')).toEqual('1')
+  expect(formatNumber(1, 'Count')).toEqual('1_')
+  expect(formatNumber(1, 'Mm')).toEqual('1mm')
+  expect(formatNumber(1, 'Inch')).toEqual('1in')
+  expect(formatNumber(0.5, 'Mm')).toEqual('0.5mm')
+  expect(formatNumber(-0.5, 'Mm')).toEqual('-0.5mm')
+})

src/lang/wasm.ts

@@ -2,6 +2,7 @@ import {
   init,
   parse_wasm,
   recast_wasm,
+  format_number,
   execute,
   kcl_lint,
   modify_ast_for_sketch_wasm,
@@ -54,6 +55,7 @@ import { ArtifactCommand } from 'wasm-lib/kcl/bindings/Artifact'
 import { ArtifactGraph as RustArtifactGraph } from 'wasm-lib/kcl/bindings/Artifact'
 import { Artifact } from './std/artifactGraph'
 import { getNodePathFromSourceRange } from 'lang/queryAstNodePathUtils'
+import { NumericSuffix } from 'wasm-lib/kcl/bindings/NumericSuffix'
 
 export type { Artifact } from 'wasm-lib/kcl/bindings/Artifact'
 export type { ArtifactCommand } from 'wasm-lib/kcl/bindings/Artifact'
@@ -92,6 +94,7 @@ export type { Literal } from '../wasm-lib/kcl/bindings/Literal'
 export type { LiteralValue } from '../wasm-lib/kcl/bindings/LiteralValue'
 export type { ArrayExpression } from '../wasm-lib/kcl/bindings/ArrayExpression'
 export type { SourceRange } from 'wasm-lib/kcl/bindings/SourceRange'
+export type { NumericSuffix } from 'wasm-lib/kcl/bindings/NumericSuffix'
 
 export type SyntaxType =
   | 'Program'
@@ -642,6 +645,13 @@ export const recast = (ast: Program): string | Error => {
   return recast_wasm(JSON.stringify(ast))
 }
 
+/**
+ * Format a number with suffix as KCL.
+ */
+export function formatNumber(value: number, suffix: NumericSuffix): string {
+  return format_number(value, JSON.stringify(suffix))
+}
+
 export const makeDefaultPlanes = async (
   engineCommandManager: EngineCommandManager
 ): Promise<DefaultPlanes> => {

src/lib/rectangleTool.test.ts

@@ -0,0 +1,92 @@
+import { expect } from 'vitest'
+import {
+  recast,
+  assertParse,
+  topLevelRange,
+  VariableDeclaration,
+  initPromise,
+} from 'lang/wasm'
+import { updateCenterRectangleSketch } from './rectangleTool'
+import { getNodePathFromSourceRange } from 'lang/queryAstNodePathUtils'
+import { getNodeFromPath } from 'lang/queryAst'
+import { findUniqueName } from 'lang/modifyAst'
+import { err, trap } from './trap'
+
+beforeAll(async () => {
+  await initPromise
+})
+
+describe('library rectangleTool helper functions', () => {
+  describe('updateCenterRectangleSketch', () => {
+    // regression test for https://github.com/KittyCAD/modeling-app/issues/5157
+    test('should update AST and source code', async () => {
+      // Base source code that will be edited in place
+      const sourceCode = `sketch001 = startSketchOn('XZ')
+|> startProfileAt([120.37, 162.76], %)
+|> angledLine([0, 0], %, $rectangleSegmentA001)
+|> angledLine([segAng(rectangleSegmentA001) + 90, 0], %, $rectangleSegmentB001)
+|> angledLine([
+segAng(rectangleSegmentA001),
+-segLen(rectangleSegmentA001)
+], %, $rectangleSegmentC001)
+|> lineTo([profileStartX(%), profileStartY(%)], %)
+|> close(%)
+`
+      // Create ast
+      const _ast = assertParse(sourceCode)
+      let ast = structuredClone(_ast)
+
+      // Find some nodes and paths to reference
+      const sketchSnippet = `startProfileAt([120.37, 162.76], %)`
+      const sketchRange = topLevelRange(
+        sourceCode.indexOf(sketchSnippet),
+        sourceCode.indexOf(sketchSnippet) + sketchSnippet.length
+      )
+      const sketchPathToNode = getNodePathFromSourceRange(ast, sketchRange)
+      const _node = getNodeFromPath<VariableDeclaration>(
+        ast,
+        sketchPathToNode || [],
+        'VariableDeclaration'
+      )
+      if (trap(_node)) return
+      const sketchInit = _node.node?.declaration.init
+
+      // Hard code inputs that a user would have taken with their mouse
+      const x = 40
+      const y = 60
+      const rectangleOrigin = [120, 180]
+      const tags: [string, string, string] = [
+        'rectangleSegmentA001',
+        'rectangleSegmentB001',
+        'rectangleSegmentC001',
+      ]
+
+      // Update the ast
+      if (sketchInit.type === 'PipeExpression') {
+        updateCenterRectangleSketch(
+          sketchInit,
+          x,
+          y,
+          tags[0],
+          rectangleOrigin[0],
+          rectangleOrigin[1]
+        )
+      }
+
+      // ast is edited in place from the updateCenterRectangleSketch
+      const expectedSourceCode = `sketch001 = startSketchOn('XZ')
+  |> startProfileAt([80, 120], %)
+  |> angledLine([0, 80], %, $rectangleSegmentA001)
+  |> angledLine([segAng(rectangleSegmentA001) + 90, 120], %, $rectangleSegmentB001)
+  |> angledLine([
+       segAng(rectangleSegmentA001),
+       -segLen(rectangleSegmentA001)
+     ], %, $rectangleSegmentC001)
+  |> lineTo([profileStartX(%), profileStartY(%)], %)
+  |> close(%)
+`
+      const recasted = recast(ast)
+      expect(recasted).toEqual(expectedSourceCode)
+    })
+  })
+})

src/lib/rectangleTool.ts

@@ -10,13 +10,19 @@ import {
   createTagDeclarator,
   createUnaryExpression,
 } from 'lang/modifyAst'
-import { ArrayExpression, CallExpression, PipeExpression } from 'lang/wasm'
+import {
+  ArrayExpression,
+  CallExpression,
+  PipeExpression,
+  recast,
+} from 'lang/wasm'
 import { roundOff } from 'lib/utils'
 import {
   isCallExpression,
   isArrayExpression,
   isLiteral,
   isBinaryExpression,
+  isLiteralValueNumber,
 } from 'lang/util'
 
 /**
@@ -144,6 +150,7 @@ export function updateCenterRectangleSketch(
     if (isArrayExpression(arrayExpression)) {
       const literal = arrayExpression.elements[0]
       if (isLiteral(literal)) {
+        if (isLiteralValueNumber(literal.value)) {
           callExpression.arguments[0] = createArrayExpression([
             createLiteral(literal.value),
             createLiteral(Math.abs(twoX)),
@@ -151,6 +158,7 @@ export function updateCenterRectangleSketch(
         }
       }
     }
+  }
 
   callExpression = pipeExpression.body[3]
   if (isCallExpression(callExpression)) {

src/lib/wasm_lib_wrapper.ts

@@ -10,6 +10,7 @@
 import {
   parse_wasm as ParseWasm,
   recast_wasm as RecastWasm,
+  format_number as FormatNumber,
   execute as Execute,
   kcl_lint as KclLint,
   modify_ast_for_sketch_wasm as ModifyAstForSketch,
@@ -51,6 +52,9 @@ export const parse_wasm: typeof ParseWasm = (...args) => {
 export const recast_wasm: typeof RecastWasm = (...args) => {
   return getModule().recast_wasm(...args)
 }
+export const format_number: typeof FormatNumber = (...args) => {
+  return getModule().format_number(...args)
+}
 export const execute: typeof Execute = (...args) => {
   return getModule().execute(...args)
 }

src/wasm-lib/kcl/src/lib.rs

@@ -120,6 +120,11 @@ pub mod std_utils {
     pub use crate::std::utils::{get_tangential_arc_to_info, is_points_ccw_wasm, TangentialArcInfoInput};
 }
 
+pub mod pretty {
+    pub use crate::parsing::token::NumericSuffix;
+    pub use crate::unparser::format_number;
+}
+
 use serde::{Deserialize, Serialize};
 
 #[allow(unused_imports)]

src/wasm-lib/kcl/src/unparser.rs

@@ -8,6 +8,7 @@ use crate::parsing::{
         LiteralValue, MemberExpression, MemberObject, Node, NonCodeNode, NonCodeValue, ObjectExpression, Parameter,
         PipeExpression, Program, TagDeclarator, UnaryExpression, VariableDeclaration, VariableKind,
     },
+    token::NumericSuffix,
     PIPE_OPERATOR,
 };
 
@@ -370,6 +371,11 @@ impl VariableDeclaration {
     }
 }
 
+// Used by TS.
+pub fn format_number(value: f64, suffix: NumericSuffix) -> String {
+    format!("{value}{suffix}")
+}
+
 impl Literal {
     fn recast(&self) -> String {
         match self.value {

src/wasm-lib/src/wasm.rs

@@ -5,7 +5,8 @@ use std::sync::Arc;
 use futures::stream::TryStreamExt;
 use gloo_utils::format::JsValueSerdeExt;
 use kcl_lib::{
-    exec::IdGenerator, CacheInformation, CoreDump, EngineManager, ExecState, ModuleId, OldAstState, Point2d, Program,
+    exec::IdGenerator, pretty::NumericSuffix, CacheInformation, CoreDump, EngineManager, ExecState, ModuleId,
+    OldAstState, Point2d, Program,
 };
 use tokio::sync::RwLock;
 use tower_lsp::{LspService, Server};
@@ -251,6 +252,14 @@ pub fn recast_wasm(json_str: &str) -> Result<JsValue, JsError> {
     Ok(JsValue::from_serde(&program.recast())?)
 }
 
+#[wasm_bindgen]
+pub fn format_number(value: f64, suffix_json: &str) -> Result<String, JsError> {
+    console_error_panic_hook::set_once();
+
+    let suffix: NumericSuffix = serde_json::from_str(suffix_json).map_err(JsError::from)?;
+    Ok(kcl_lib::pretty::format_number(value, suffix))
+}
+
 #[wasm_bindgen]
 pub struct ServerConfig {
     into_server: js_sys::AsyncIterator,