Remove KclValue::SketchGroup variant

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. I'll benchmark it and see.

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

@@ -23,7 +23,7 @@ use crate::{
     docs::StdLibFn,
     errors::{KclError, KclErrorDetails},
     executor::{
-        BodyType, DynamicState, ExecutorContext, KclValue, Metadata, PipeInfo, ProgramMemory, SourceRange,
+        BodyType, DynamicState, ExecutorContext, KclValue, Metadata, PipeInfo, ProgramMemory, SketchGroup, SourceRange,
         StatementKind, TagEngineInfo, TagIdentifier, UserVal,
     },
     parser::PIPE_OPERATOR,
@@ -1356,10 +1356,13 @@ impl CallExpression {
                 // TODO: This could probably be done in a better way, but as of now this was my only idea
                 // and it works.
                 match result {
-                    KclValue::SketchGroup(ref sketch_group) => {
-                        for (_, tag) in sketch_group.tags.iter() {
-                            memory.update_tag(&tag.value, tag.clone())?;
-                        }
+                    KclValue::UserVal(ref mut uval) => {
+                        uval.mutate(|sketch_group: &mut SketchGroup| {
+                            for (_, tag) in sketch_group.tags.iter() {
+                                memory.update_tag(&tag.value, tag.clone())?;
+                            }
+                            Ok::<_, KclError>(())
+                        })?;
                     }
                     KclValue::ExtrudeGroup(ref mut extrude_group) => {
                         for value in &extrude_group.value {

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

@@ -203,13 +203,22 @@ impl Environment {
         }
 
         for (_, val) in self.bindings.iter_mut() {
-            if let KclValue::SketchGroup(ref mut sketch_group) = val {
-                if sketch_group.original_id == sg.original_id {
-                    for tag in sg.tags.iter() {
-                        sketch_group.tags.insert(tag.0.clone(), tag.1.clone());
-                    }
+            let KclValue::UserVal(v) = val else { continue };
+            let meta = v.meta.clone();
+            let maybe_sg: Result<SketchGroup, _> = serde_json::from_value(v.value.clone());
+            let Ok(mut sketch_group) = maybe_sg else {
+                continue;
+            };
+
+            if sketch_group.original_id == sg.original_id {
+                for tag in sg.tags.iter() {
+                    sketch_group.tags.insert(tag.0.clone(), tag.1.clone());
                 }
             }
+            *val = KclValue::UserVal(UserVal {
+                meta,
+                value: serde_json::to_value(sketch_group).expect("can always turn SketchGroup into JSON"),
+            });
         }
     }
 }
@@ -268,10 +277,7 @@ pub enum KclValue {
     TagDeclarator(Box<TagDeclarator>),
     Plane(Box<Plane>),
     Face(Box<Face>),
-    SketchGroup(Box<SketchGroup>),
-    SketchGroups {
-        value: Vec<Box<SketchGroup>>,
-    },
+
     ExtrudeGroup(Box<ExtrudeGroup>),
     ExtrudeGroups {
         value: Vec<Box<ExtrudeGroup>>,
@@ -289,27 +295,8 @@ pub enum KclValue {
 }
 
 impl KclValue {
-    pub(crate) fn get_sketch_group_set(&self) -> Result<SketchGroupSet> {
-        match self {
-            KclValue::SketchGroup(s) => Ok(SketchGroupSet::SketchGroup(s.clone())),
-            KclValue::SketchGroups { value } => Ok(SketchGroupSet::SketchGroups(value.clone())),
-            KclValue::UserVal(value) => {
-                let value = value.value.clone();
-                match value {
-                    JValue::Null | JValue::Bool(_) | JValue::Number(_) | JValue::String(_) => Err(anyhow::anyhow!(
-                        "Failed to deserialize sketch group set from JSON {}",
-                        human_friendly_type(&value)
-                    )),
-                    JValue::Array(_) => serde_json::from_value::<Vec<Box<SketchGroup>>>(value)
-                        .map(SketchGroupSet::from)
-                        .map_err(|e| anyhow::anyhow!("Failed to deserialize array of sketch groups from JSON: {}", e)),
-                    JValue::Object(_) => serde_json::from_value::<Box<SketchGroup>>(value)
-                        .map(SketchGroupSet::from)
-                        .map_err(|e| anyhow::anyhow!("Failed to deserialize sketch group from JSON: {}", e)),
-                }
-            }
-            _ => anyhow::bail!("Not a sketch group or sketch groups: {:?}", self),
-        }
+    pub(crate) fn new_user_val<T: Serialize>(meta: Vec<Metadata>, val: T) -> Self {
+        Self::UserVal(UserVal::set(meta, val))
     }
 
     pub(crate) fn get_extrude_group_set(&self) -> Result<ExtrudeGroupSet> {
@@ -342,8 +329,6 @@ impl KclValue {
             KclValue::UserVal(u) => human_friendly_type(&u.value),
             KclValue::TagDeclarator(_) => "TagDeclarator",
             KclValue::TagIdentifier(_) => "TagIdentifier",
-            KclValue::SketchGroup(_) => "SketchGroup",
-            KclValue::SketchGroups { .. } => "SketchGroups",
             KclValue::ExtrudeGroup(_) => "ExtrudeGroup",
             KclValue::ExtrudeGroups { .. } => "ExtrudeGroups",
             KclValue::ImportedGeometry(_) => "ImportedGeometry",
@@ -356,20 +341,14 @@ impl KclValue {
 
 impl From<SketchGroupSet> for KclValue {
     fn from(sg: SketchGroupSet) -> Self {
-        match sg {
-            SketchGroupSet::SketchGroup(sg) => KclValue::SketchGroup(sg),
-            SketchGroupSet::SketchGroups(sgs) => KclValue::SketchGroups { value: sgs },
-        }
+        KclValue::UserVal(UserVal::set(sg.meta(), sg))
     }
 }
 
 impl From<Vec<Box<SketchGroup>>> for KclValue {
     fn from(sg: Vec<Box<SketchGroup>>) -> Self {
-        if sg.len() == 1 {
-            KclValue::SketchGroup(sg[0].clone())
-        } else {
-            KclValue::SketchGroups { value: sg }
-        }
+        let meta = sg.iter().flat_map(|sg| sg.meta.clone()).collect();
+        KclValue::UserVal(UserVal::set(meta, sg))
     }
 }
 
@@ -428,6 +407,24 @@ pub enum SketchGroupSet {
     SketchGroups(Vec<Box<SketchGroup>>),
 }
 
+impl SketchGroupSet {
+    pub fn meta(&self) -> Vec<Metadata> {
+        match self {
+            SketchGroupSet::SketchGroup(sg) => sg.meta.clone(),
+            SketchGroupSet::SketchGroups(sg) => sg.iter().flat_map(|sg| sg.meta.clone()).collect(),
+        }
+    }
+}
+
+impl From<SketchGroupSet> for Vec<SketchGroup> {
+    fn from(value: SketchGroupSet) -> Self {
+        match value {
+            SketchGroupSet::SketchGroup(sg) => vec![*sg],
+            SketchGroupSet::SketchGroups(sgs) => sgs.into_iter().map(|sg| *sg).collect(),
+        }
+    }
+}
+
 impl From<SketchGroup> for SketchGroupSet {
     fn from(sg: SketchGroup) -> Self {
         SketchGroupSet::SketchGroup(Box::new(sg))
@@ -641,6 +638,43 @@ pub struct UserVal {
     pub meta: Vec<Metadata>,
 }
 
+impl UserVal {
+    /// If the UserVal matches the type `T`, return it.
+    pub fn get<T: serde::de::DeserializeOwned>(&self) -> Option<(T, Vec<Metadata>)> {
+        let meta = self.meta.clone();
+        // TODO: This clone might cause performance problems, it'll happen a lot.
+        let res: Result<T, _> = serde_json::from_value(self.value.clone());
+        if let Ok(t) = res {
+            Some((t, meta))
+        } else {
+            None
+        }
+    }
+
+    /// If the UserVal matches the type `T`, then mutate it via the given closure.
+    /// If the closure returns Err, the mutation won't be applied.
+    pub fn mutate<T, F, E>(&mut self, mutate: F) -> Result<(), E>
+    where
+        T: serde::de::DeserializeOwned + Serialize,
+        F: FnOnce(&mut T) -> Result<(), E>,
+    {
+        let Some((mut val, meta)) = self.get::<T>() else {
+            return Ok(());
+        };
+        mutate(&mut val)?;
+        *self = Self::set(meta, val);
+        Ok(())
+    }
+
+    /// Put the given value into this UserVal.
+    pub fn set<T: serde::Serialize>(meta: Vec<Metadata>, val: T) -> Self {
+        Self {
+            meta,
+            value: serde_json::to_value(val).expect("all KCL values should be compatible with JSON"),
+        }
+    }
+}
+
 #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, ts_rs::TS, JsonSchema)]
 #[ts(export)]
 #[serde(tag = "type", rename_all = "camelCase")]
@@ -720,11 +754,6 @@ impl From<KclValue> for Vec<SourceRange> {
             KclValue::UserVal(u) => u.meta.iter().map(|m| m.source_range).collect(),
             KclValue::TagDeclarator(t) => t.into(),
             KclValue::TagIdentifier(t) => t.meta.iter().map(|m| m.source_range).collect(),
-            KclValue::SketchGroup(s) => s.meta.iter().map(|m| m.source_range).collect(),
-            KclValue::SketchGroups { value } => value
-                .iter()
-                .flat_map(|sg| sg.meta.iter().map(|m| m.source_range))
-                .collect(),
             KclValue::ExtrudeGroup(e) => e.meta.iter().map(|m| m.source_range).collect(),
             KclValue::ExtrudeGroups { value } => value
                 .iter()

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

@@ -251,11 +251,11 @@ impl Args {
         FromArgs::from_args(self, 0)
     }
 
-    pub(crate) fn get_sketch_groups(&self) -> Result<(SketchGroupSet, Box<SketchGroup>), KclError> {
+    pub(crate) fn get_sketch_groups(&self) -> Result<(SketchGroupSet, SketchGroup), KclError> {
         FromArgs::from_args(self, 0)
     }
 
-    pub(crate) fn get_sketch_group(&self) -> Result<Box<SketchGroup>, KclError> {
+    pub(crate) fn get_sketch_group(&self) -> Result<SketchGroup, KclError> {
         FromArgs::from_args(self, 0)
     }
 
@@ -270,9 +270,7 @@ impl Args {
         FromArgs::from_args(self, 0)
     }
 
-    pub(crate) fn get_sketch_group_and_optional_tag(
-        &self,
-    ) -> Result<(Box<SketchGroup>, Option<TagDeclarator>), KclError> {
+    pub(crate) fn get_sketch_group_and_optional_tag(&self) -> Result<(SketchGroup, Option<TagDeclarator>), KclError> {
         FromArgs::from_args(self, 0)
     }
 
@@ -283,7 +281,7 @@ impl Args {
         FromArgs::from_args(self, 0)
     }
 
-    pub(crate) fn get_data_and_sketch_group<'a, T>(&'a self) -> Result<(T, Box<SketchGroup>), KclError>
+    pub(crate) fn get_data_and_sketch_group<'a, T>(&'a self) -> Result<(T, SketchGroup), KclError>
     where
         T: serde::de::DeserializeOwned + FromArgs<'a>,
     {
@@ -299,7 +297,7 @@ impl Args {
 
     pub(crate) fn get_data_and_sketch_group_and_tag<'a, T>(
         &'a self,
-    ) -> Result<(T, Box<SketchGroup>, Option<TagDeclarator>), KclError>
+    ) -> Result<(T, SketchGroup, Option<TagDeclarator>), KclError>
     where
         T: serde::de::DeserializeOwned + FromKclValue<'a> + Sized,
     {
@@ -338,7 +336,7 @@ impl Args {
         FromArgs::from_args(self, 0)
     }
 
-    pub(crate) fn get_tag_to_number_sketch_group(&self) -> Result<(TagIdentifier, f64, Box<SketchGroup>), KclError> {
+    pub(crate) fn get_tag_to_number_sketch_group(&self) -> Result<(TagIdentifier, f64, SketchGroup), KclError> {
         FromArgs::from_args(self, 0)
     }
 
@@ -550,15 +548,6 @@ impl<'a> FromKclValue<'a> for TagIdentifier {
     }
 }
 
-impl<'a> FromKclValue<'a> for &'a SketchGroup {
-    fn from_mem_item(arg: &'a KclValue) -> Option<Self> {
-        let KclValue::SketchGroup(s) = arg else {
-            return None;
-        };
-        Some(s.as_ref())
-    }
-}
-
 macro_rules! impl_from_arg_via_json {
     ($typ:path) => {
         impl<'a> FromKclValue<'a> for $typ {
@@ -608,6 +597,8 @@ impl_from_arg_via_json!(super::revolve::RevolveData);
 impl_from_arg_via_json!(super::sketch::SketchData);
 impl_from_arg_via_json!(crate::std::import::ImportFormat);
 impl_from_arg_via_json!(crate::std::polar::PolarCoordsData);
+impl_from_arg_via_json!(SketchGroup);
+impl_from_arg_via_json!(SketchGroupSet);
 impl_from_arg_via_json!(FaceTag);
 impl_from_arg_via_json!(String);
 impl_from_arg_via_json!(u32);
@@ -618,24 +609,6 @@ impl_from_arg_via_json!(bool);
 impl_from_arg_for_array!(2);
 impl_from_arg_for_array!(3);
 
-impl<'a> FromKclValue<'a> for &'a Box<SketchGroup> {
-    fn from_mem_item(arg: &'a KclValue) -> Option<Self> {
-        let KclValue::SketchGroup(s) = arg else {
-            return None;
-        };
-        Some(s)
-    }
-}
-
-impl<'a> FromKclValue<'a> for Box<SketchGroup> {
-    fn from_mem_item(arg: &'a KclValue) -> Option<Self> {
-        let KclValue::SketchGroup(s) = arg else {
-            return None;
-        };
-        Some(s.to_owned())
-    }
-}
-
 impl<'a> FromKclValue<'a> for Box<ExtrudeGroup> {
     fn from_mem_item(arg: &'a KclValue) -> Option<Self> {
         let KclValue::ExtrudeGroup(s) = arg else {
@@ -656,15 +629,16 @@ impl<'a> FromKclValue<'a> for ExtrudeGroupSet {
         arg.get_extrude_group_set().ok()
     }
 }
-impl<'a> FromKclValue<'a> for SketchGroupSet {
-    fn from_mem_item(arg: &'a KclValue) -> Option<Self> {
-        arg.get_sketch_group_set().ok()
-    }
-}
 impl<'a> FromKclValue<'a> for SketchSurfaceOrGroup {
     fn from_mem_item(arg: &'a KclValue) -> Option<Self> {
         match arg {
-            KclValue::SketchGroup(sg) => Some(Self::SketchGroup(sg.clone())),
+            KclValue::UserVal(uv) => {
+                if let Some((sg, _meta)) = uv.get() {
+                    Some(Self::SketchGroup(sg))
+                } else {
+                    None
+                }
+            }
             KclValue::Plane(sg) => Some(Self::SketchSurface(SketchSurface::Plane(sg.clone()))),
             KclValue::Face(sg) => Some(Self::SketchSurface(SketchSurface::Face(sg.clone()))),
             _ => None,

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

@@ -76,7 +76,7 @@ async fn inner_extrude(length: f64, sketch_group_set: SketchGroupSet, args: Args
     let id = uuid::Uuid::new_v4();
 
     // Extrude the element(s).
-    let sketch_groups: Vec<Box<SketchGroup>> = sketch_group_set.into();
+    let sketch_groups: Vec<SketchGroup> = sketch_group_set.into();
     let mut extrude_groups = Vec::new();
     for sketch_group in &sketch_groups {
         // Before we extrude, we need to enable the sketch mode.
@@ -118,7 +118,7 @@ async fn inner_extrude(length: f64, sketch_group_set: SketchGroupSet, args: Args
 }
 
 pub(crate) async fn do_post_extrude(
-    sketch_group: Box<SketchGroup>,
+    sketch_group: SketchGroup,
     length: f64,
     id: Uuid,
     args: Args,
@@ -155,7 +155,7 @@ pub(crate) async fn do_post_extrude(
         }));
     };
 
-    let mut sketch_group = *sketch_group.clone();
+    let mut sketch_group = sketch_group.clone();
 
     // If we were sketching on a face, we need the original face id.
     if let SketchSurface::Face(ref face) = sketch_group.on {

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

@@ -102,7 +102,7 @@ impl RevolveAxisAndOrigin {
 
 /// Revolve a sketch around an axis.
 pub async fn revolve(args: Args) -> Result<KclValue, KclError> {
-    let (data, sketch_group): (RevolveData, Box<SketchGroup>) = args.get_data_and_sketch_group()?;
+    let (data, sketch_group): (RevolveData, SketchGroup) = args.get_data_and_sketch_group()?;
 
     let extrude_group = inner_revolve(data, sketch_group, args).await?;
     Ok(KclValue::ExtrudeGroup(extrude_group))
@@ -249,7 +249,7 @@ pub async fn revolve(args: Args) -> Result<KclValue, KclError> {
 }]
 async fn inner_revolve(
     data: RevolveData,
-    sketch_group: Box<SketchGroup>,
+    sketch_group: SketchGroup,
     args: Args,
 ) -> Result<Box<ExtrudeGroup>, KclError> {
     if let Some(angle) = data.angle {