Program memory should not store JSON

[adamchalmers]

Background

KCL program memory can store various different values, defined by the MemoryItem enum.

One case is "UserVal" which ultimately just stores serde_json::Value (see definition of UserVal). User values are serialized into JSON before storing them in program memory. Reading from memory deserializes the value into its expected type, and returns a KCL type error if the value couldn't be deserialized into the right type.

Problem

This works OK but leads to bugs when developing the modeling-app wasm-lib. Why? Because so many types implement serde::Deserialize and you can try to deserialize a memory item into any of them. Even if those types never get written to memory ever! E.g. we never store a SocketAddrV4 in KCL program memory, but the rust compiler won't warn you if you try to request that from KCL memory. But it should! It should be a rust-compile-time error to request something we'd never expect to find in KCL memory.

Solution

We should remove the UserVal variant from MemoryItem and add new variants for KCL primitive types (numbers, strings, the KCL none type, etc).

This way, you ensure that reading values from memory requires that the value can at least be stored in memory.

[jtran]

I learned that in KCL, 0 / 0 is null! I learned this trying to make a test case that uses NaN or infinity. Couldn’t figure out those, but I got something else :smiling_imp: This happens because we represent all our numbers with a JSON type, and JSON doesn’t support NaN or infinity.

Slack thread

Trying to use null as a number:

The above is just an observation. I don't think anyone intended the above. But I think that long-term, if we use floating point numbers, we should support all of floating point, including NaN and infinity.

[adamchalmers]

We basically want a model for what sorts of values exist in KCL. Here's my proposal for what KCL types exist.

• Booleans
• Integers
• Fractional numbers
• Strings
• Tag declaration
• Tag ID
• Closure (i.e. function + captured environment)
• Imported geometry
• KclNone (this is the value of an optional parameter if no argument is given)
• KclArray (i.e. Vec)
• KclObject (i.e. HashMap<String, KclValue>)

Each of these would be a variant of enum KclValue.

If we do this, then KclValue's variants for SketchGroup, SketchGroupSet etc would be removed. Instead, SketchGroup would be stored as a KclObject. Functions which return SketchGroup would serialize it into a KclObject. Functions which take a sketch group would have to check the KCL value being passed as an argument is the KclObject variant, then deserialize that KclObject into a SketchGroup.

[jtran]

As a KCL user, I like the idea of making SketchGroup and other things be Objects like any other user-defined object. My only concern would be whether we can represent all the things we need internally. But if we can do that, I think this is a good idea.

[jtran]

I'm looking at the definition of SketchGroup et al now. The first thing that jumps out at me is that one of its fields is its Paths. A Path contains all the coordinates of the path, whether each segment is a line, tangential arc, angled line, etc. So this would need to be represented as KclObjects too, in addition to Plane, Face, and ExtrudeGroup.

If I were doing this, it seems big enough that I'd probably do it as a separate change, i.e. one PR to replace JSON with booleans, integers, floats, and strings. Then after it's merged, a second PR to collapse the rest into KclObjects.

Even just the first PR should fix #3338 and other issues.

[adamchalmers]

I'll probably define

trait AsKclObject {
  fn into(&self) -> KclObject
  fn from(KclObject) -> Result<Self, KclError>
}

and impl this for types like SketchGroup and ExtrudeGroup and various types we send to the API. At some point in the future we can make a derive macro if we want.

Then I'll also have

trait AsKclValue {
  fn into(&self) -> KclValue
  fn from(KclValue) -> Result<Self, KclError>
}

this will be implemented for ints, floats, strings etc by mapping them to/from the KclValue enum. We'll have a blanket impl for any T that impls AsKclObject.

[jtran]

To clarify, this is what I propose as a first step to reduce the downstream impact. I believe this would fix #3338 because the SketchGroup would be stored as a value in the Object's HashMap, as a KclValue::SketchGroup (or possibly KclValue::SketchGroups plural). This avoids serialization to JSON. The serialization to JSON is problematic because nothing in executor knows how to deserialize again.

If you're doing the change, it's up to you whether you want to try to do it all at once, like in your comment above.

pub enum KclValue {
    // I don't want to get hung up on this.  I think we use null in a couple
    // places.  This is a replacement for that.  It would be nice to have
    // something for when a function has no meaningful return value, instead of
    // Option::None.
    Unit,
    Boolean(bool),
    String(String),
    Integer(i64),
    Float(f64),
    Array(Vec<KclValue>),
    Object(HashMap<String, KclValue>),

    // Everything below is unchanged.
    TagIdentifier(Box<TagIdentifier>),
    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>>,
    },
    ImportedGeometry(ImportedGeometry),
    #[ts(skip)]
    Function {
        #[serde(skip)]
        func: Option<MemoryFunction>,
        expression: Box<FunctionExpression>,
        memory: Box<ProgramMemory>,
        #[serde(rename = "__meta")]
        meta: Vec<Metadata>,
    },
}