Remove KclValue::SketchGroup variant (#3446)

We can store Rust types like `SketchGroup` as their own variant of `KclValue`, or as `KclValue::UserVal`. Sometimes we store in one and try to read from the other, which fails. This causes bugs, like #3338.

Instead, we should use either ::SketchGroup or ::UserVal, and stop using the other. If we stopped using ::UserVal, we'd need a new variant for every Rust type we wanted to build, including user-defined types. So I don't think that's practical.

Instead, we should store every KCL value by de/serializing it into UserVal. This is a first step along that path, removing just the SketchGroup variants. If it goes well, we can remove the other specialized variants too.

My only concern is there might be performance implications from how frequently we convert between serde_json::Value and Rust types via Serde. But I'm not too worried -- there's no parsing JSON strings, just traversing serde_json::Value trees. This isn't great for performance but I think it'll probably be miniscule in comparison to doing all the API calls.

src/clientSideScene/sceneEntities.ts

@@ -50,6 +50,7 @@ import {
   ExtrudeGroup,
   VariableDeclaration,
   VariableDeclarator,
+  sketchGroupFromKclValue,
 } from 'lang/wasm'
 import {
   engineCommandManager,
@@ -588,10 +589,12 @@ export class SceneEntities {
     const variableDeclarationName =
       _node1.node?.declarations?.[0]?.id?.name || ''
 
-    const sg = kclManager.programMemory.get(
+    const sg = sketchGroupFromKclValue(
+      kclManager.programMemory.get(variableDeclarationName),
       variableDeclarationName
-    ) as SketchGroup
-    const lastSeg = sg?.value?.slice(-1)[0] || sg.start
+    )
+    if (err(sg)) return sg
+    const lastSeg = sg.value?.slice(-1)[0] || sg.start
 
     const index = sg.value.length // because we've added a new segment that's not in the memory yet, no need for `-1`
 
@@ -786,9 +789,11 @@ export class SceneEntities {
           programMemoryOverride,
         })
         this.sceneProgramMemory = programMemory
-        const sketchGroup = programMemory.get(
+        const sketchGroup = sketchGroupFromKclValue(
+          programMemory.get(variableDeclarationName),
           variableDeclarationName
-        ) as SketchGroup
+        )
+        if (err(sketchGroup)) return sketchGroup
         const sgPaths = sketchGroup.value
         const orthoFactor = orthoScale(sceneInfra.camControls.camera)
 
@@ -840,9 +845,11 @@ export class SceneEntities {
 
           // Prepare to update the THREEjs scene
           this.sceneProgramMemory = programMemory
-          const sketchGroup = programMemory.get(
+          const sketchGroup = sketchGroupFromKclValue(
+            programMemory.get(variableDeclarationName),
             variableDeclarationName
-          ) as SketchGroup
+          )
+          if (err(sketchGroup)) return sketchGroup
           const sgPaths = sketchGroup.value
           const orthoFactor = orthoScale(sceneInfra.camControls.camera)
 
@@ -1111,8 +1118,12 @@ export class SceneEntities {
 
       const maybeSketchGroup = programMemory.get(variableDeclarationName)
       let sketchGroup = undefined
-      if (maybeSketchGroup?.type === 'SketchGroup') {
-        sketchGroup = maybeSketchGroup
+      const sg = sketchGroupFromKclValue(
+        maybeSketchGroup,
+        variableDeclarationName
+      )
+      if (!err(sg)) {
+        sketchGroup = sg
       } else if ((maybeSketchGroup as ExtrudeGroup).sketchGroup) {
         sketchGroup = (maybeSketchGroup as ExtrudeGroup).sketchGroup
       }
@@ -1656,9 +1667,12 @@ function prepareTruncatedMemoryAndAst(
   )
   if (err(_node)) return _node
   const variableDeclarationName = _node.node?.declarations?.[0]?.id?.name || ''
-  const lastSeg = (
-    programMemory.get(variableDeclarationName) as SketchGroup
-  ).value.slice(-1)[0]
+  const sg = sketchGroupFromKclValue(
+    programMemory.get(variableDeclarationName),
+    variableDeclarationName
+  )
+  if (err(sg)) return sg
+  const lastSeg = sg?.value.slice(-1)[0]
   if (draftSegment) {
     // truncatedAst needs to setup with another segment at the end
     let newSegment
@@ -1782,10 +1796,11 @@ export function sketchGroupFromPathToNode({
   if (result?.type === 'ExtrudeGroup') {
     return result.sketchGroup
   }
-  if (result?.type === 'SketchGroup') {
-    return result
+  const sg = sketchGroupFromKclValue(result, varDec?.id?.name)
+  if (err(sg)) {
+    return null
   }
-  return null
+  return sg
 }
 
 function colorSegment(object: any, color: number) {

src/components/ModelingSidebar/ModelingPanes/MemoryPane.tsx

@@ -1,11 +1,16 @@
 import toast from 'react-hot-toast'
 import ReactJson from 'react-json-view'
 import { useMemo } from 'react'
-import { ProgramMemory, Path, ExtrudeSurface } from 'lang/wasm'
+import {
+  ProgramMemory,
+  Path,
+  ExtrudeSurface,
+  sketchGroupFromKclValue,
+} from 'lang/wasm'
 import { useKclContext } from 'lang/KclProvider'
 import { useResolvedTheme } from 'hooks/useResolvedTheme'
 import { ActionButton } from 'components/ActionButton'
-import { trap } from 'lib/trap'
+import { err, trap } from 'lib/trap'
 import Tooltip from 'components/Tooltip'
 import { useModelingContext } from 'hooks/useModelingContext'
 
@@ -84,8 +89,9 @@ export const processMemory = (programMemory: ProgramMemory) => {
   const processedMemory: any = {}
   for (const [key, val] of programMemory?.visibleEntries()) {
     if (typeof val.value !== 'function') {
-      if (val.type === 'SketchGroup') {
-        processedMemory[key] = val.value.map(({ __geoMeta, ...rest }: Path) => {
+      const sg = sketchGroupFromKclValue(val, null)
+      if (!err(sg)) {
+        processedMemory[key] = sg.value.map(({ __geoMeta, ...rest }: Path) => {
           return rest
         })
       } else if (val.type === 'ExtrudeGroup') {

src/lang/artifact.test.ts

@@ -18,41 +18,45 @@ const mySketch001 = startSketchOn('XY')
     // @ts-ignore
     const sketch001 = programMemory?.get('mySketch001')
     expect(sketch001).toEqual({
-      type: 'SketchGroup',
-      on: expect.any(Object),
-      start: {
-        to: [0, 0],
-        from: [0, 0],
-        tag: null,
-        __geoMeta: {
-          id: expect.any(String),
-          sourceRange: [46, 71],
-        },
-      },
-      value: [
-        {
-          type: 'ToPoint',
-          tag: null,
-          to: [-1.59, -1.54],
+      type: 'UserVal',
+      __meta: [{ sourceRange: [46, 71] }],
+      value: {
+        type: 'SketchGroup',
+        on: expect.any(Object),
+        start: {
+          to: [0, 0],
           from: [0, 0],
-          __geoMeta: {
-            sourceRange: [77, 102],
-            id: expect.any(String),
-          },
-        },
-        {
-          type: 'ToPoint',
-          to: [0.46, -5.82],
-          from: [-1.59, -1.54],
           tag: null,
           __geoMeta: {
-            sourceRange: [108, 132],
             id: expect.any(String),
+            sourceRange: [46, 71],
           },
         },
-      ],
-      id: expect.any(String),
-      __meta: [{ sourceRange: [46, 71] }],
+        value: [
+          {
+            type: 'ToPoint',
+            tag: null,
+            to: [-1.59, -1.54],
+            from: [0, 0],
+            __geoMeta: {
+              sourceRange: [77, 102],
+              id: expect.any(String),
+            },
+          },
+          {
+            type: 'ToPoint',
+            to: [0.46, -5.82],
+            from: [-1.59, -1.54],
+            tag: null,
+            __geoMeta: {
+              sourceRange: [108, 132],
+              id: expect.any(String),
+            },
+          },
+        ],
+        id: expect.any(String),
+        __meta: [{ sourceRange: [46, 71] }],
+      },
     })
   })
   test('extrude artifacts', async () => {

src/lang/executor.test.ts

@@ -1,6 +1,12 @@
 import fs from 'node:fs'
 
-import { parse, ProgramMemory, SketchGroup, initPromise } from './wasm'
+import {
+  parse,
+  ProgramMemory,
+  SketchGroup,
+  initPromise,
+  sketchGroupFromKclValue,
+} from './wasm'
 import { enginelessExecutor } from '../lib/testHelpers'
 import { KCLError } from './errors'
 
@@ -52,7 +58,7 @@ const newVar = myVar + 1`
 `
     const mem = await exe(code)
     // geo is three js buffer geometry and is very bloated to have in tests
-    const minusGeo = mem.get('mySketch')?.value
+    const minusGeo = mem.get('mySketch')?.value?.value
     expect(minusGeo).toEqual([
       {
         type: 'ToPoint',
@@ -146,68 +152,72 @@ const newVar = myVar + 1`
     ].join('\n')
     const mem = await exe(code)
     expect(mem.get('mySk1')).toEqual({
-      type: 'SketchGroup',
-      on: expect.any(Object),
-      start: {
-        to: [0, 0],
-        from: [0, 0],
-        tag: null,
-        __geoMeta: {
-          id: expect.any(String),
-          sourceRange: [39, 63],
-        },
-      },
-      tags: {
-        myPath: {
-          __meta: [
-            {
-              sourceRange: [109, 116],
-            },
-          ],
-          type: 'TagIdentifier',
-          value: 'myPath',
-          info: expect.any(Object),
-        },
-      },
-      value: [
-        {
-          type: 'ToPoint',
-          to: [1, 1],
+      type: 'UserVal',
+      value: {
+        type: 'SketchGroup',
+        on: expect.any(Object),
+        start: {
+          to: [0, 0],
           from: [0, 0],
           tag: null,
           __geoMeta: {
-            sourceRange: [69, 85],
             id: expect.any(String),
+            sourceRange: [39, 63],
           },
         },
-        {
-          type: 'ToPoint',
-          to: [0, 1],
-          from: [1, 1],
-          __geoMeta: {
-            sourceRange: [91, 117],
-            id: expect.any(String),
-          },
-          tag: {
-            end: 116,
-            start: 109,
-            type: 'TagDeclarator',
+        tags: {
+          myPath: {
+            __meta: [
+              {
+                sourceRange: [109, 116],
+              },
+            ],
+            type: 'TagIdentifier',
             value: 'myPath',
-            digest: null,
+            info: expect.any(Object),
           },
         },
-        {
-          type: 'ToPoint',
-          to: [1, 1],
-          from: [0, 1],
-          tag: null,
-          __geoMeta: {
-            sourceRange: [123, 139],
-            id: expect.any(String),
+        value: [
+          {
+            type: 'ToPoint',
+            to: [1, 1],
+            from: [0, 0],
+            tag: null,
+            __geoMeta: {
+              sourceRange: [69, 85],
+              id: expect.any(String),
+            },
           },
-        },
-      ],
-      id: expect.any(String),
+          {
+            type: 'ToPoint',
+            to: [0, 1],
+            from: [1, 1],
+            __geoMeta: {
+              sourceRange: [91, 117],
+              id: expect.any(String),
+            },
+            tag: {
+              end: 116,
+              start: 109,
+              type: 'TagDeclarator',
+              value: 'myPath',
+              digest: null,
+            },
+          },
+          {
+            type: 'ToPoint',
+            to: [1, 1],
+            from: [0, 1],
+            tag: null,
+            __geoMeta: {
+              sourceRange: [123, 139],
+              id: expect.any(String),
+            },
+          },
+        ],
+        id: expect.any(String),
+        __meta: [{ sourceRange: [39, 63] }],
+      },
       __meta: [{ sourceRange: [39, 63] }],
     })
   })
@@ -358,7 +368,7 @@ describe('testing math operators', () => {
       '|> line([-2.21, -legLen(5, min(3, 999))], %)',
     ].join('\n')
     const mem = await exe(code)
-    const sketch = mem.get('part001')
+    const sketch = sketchGroupFromKclValue(mem.get('part001'), 'part001')
     // result of `-legLen(5, min(3, 999))` should be -4
     const yVal = (sketch as SketchGroup).value?.[0]?.to?.[1]
     expect(yVal).toBe(-4)
@@ -376,7 +386,7 @@ describe('testing math operators', () => {
       ``,
     ].join('\n')
     const mem = await exe(code)
-    const sketch = mem.get('part001')
+    const sketch = sketchGroupFromKclValue(mem.get('part001'), 'part001')
     // expect -legLen(segLen('seg01'), myVar) to equal -4 setting the y value back to 0
     expect((sketch as SketchGroup).value?.[1]?.from).toEqual([3, 4])
     expect((sketch as SketchGroup).value?.[1]?.to).toEqual([6, 0])
@@ -385,7 +395,10 @@ describe('testing math operators', () => {
       `legLen(segLen(seg01), myVar)`
     )
     const removedUnaryExpMem = await exe(removedUnaryExp)
-    const removedUnaryExpMemSketch = removedUnaryExpMem.get('part001')
+    const removedUnaryExpMemSketch = sketchGroupFromKclValue(
+      removedUnaryExpMem.get('part001'),
+      'part001'
+    )
 
     // without the minus sign, the y value should be 8
     expect((removedUnaryExpMemSketch as SketchGroup).value?.[1]?.to).toEqual([