BREAKING: Change to disallow indexing KCL records/objects with strings (KittyCAD#6529)

* Change to disallow indexing records/objects with strings * Update output * Remove outdated sim test * Fix tests

Author: jtran

rust/kcl-lib/src/execution/exec_ast.rs

@@ -861,8 +861,8 @@ impl Node<MemberExpression> {
  };
 
  // Check the property and object match -- e.g. ints for arrays, strs for objects.
- match (object, property) {
- (KclValue::Object { value: map, meta: _ }, Property::String(property)) => {
+ match (object, property, self.computed) {
+ (KclValue::Object { value: map, meta: _ }, Property::String(property), false) => {
  if let Some(value) = map.get(&property) {
  Ok(value.to_owned())
  } else {
@@ -872,7 +872,11 @@ impl Node<MemberExpression> {
  }))
  }
  }
- (KclValue::Object { .. }, p) => {
+ (KclValue::Object { .. }, Property::String(property), true) => Err(KclError::Semantic(KclErrorDetails {
+ message: format!("Cannot index object with string; use dot notation instead, e.g. `obj.{property}`"),
+ source_ranges: vec![self.clone().into()],
+ })),
+ (KclValue::Object { .. }, p, _) => {
  let t = p.type_name();
  let article = article_for(t);
  Err(KclError::Semantic(KclErrorDetails {
@@ -885,6 +889,7 @@ impl Node<MemberExpression> {
  (
  KclValue::MixedArray { value: arr, .. } | KclValue::HomArray { value: arr, .. },
  Property::UInt(index),
+ _,
  ) => {
  let value_of_arr = arr.get(index);
  if let Some(value) = value_of_arr {
@@ -896,7 +901,7 @@ impl Node<MemberExpression> {
  }))
  }
  }
- (KclValue::MixedArray { .. } | KclValue::HomArray { .. }, p) => {
+ (KclValue::MixedArray { .. } | KclValue::HomArray { .. }, p, _) => {
  let t = p.type_name();
  let article = article_for(t);
  Err(KclError::Semantic(KclErrorDetails {
@@ -906,10 +911,10 @@ impl Node<MemberExpression> {
  source_ranges: vec![self.clone().into()],
  }))
  }
- (KclValue::Solid { value }, Property::String(prop)) if prop == "sketch" => Ok(KclValue::Sketch {
+ (KclValue::Solid { value }, Property::String(prop), false) if prop == "sketch" => Ok(KclValue::Sketch {
  value: Box::new(value.sketch),
  }),
- (KclValue::Sketch { value: sk }, Property::String(prop)) if prop == "tags" => Ok(KclValue::Object {
+ (KclValue::Sketch { value: sk }, Property::String(prop), false) if prop == "tags" => Ok(KclValue::Object {
  meta: vec![Metadata {
  source_range: SourceRange::from(self.clone()),
  }],
@@ -919,7 +924,7 @@ impl Node<MemberExpression> {
  .map(|(k, tag)| (k.to_owned(), KclValue::TagIdentifier(Box::new(tag.to_owned()))))
  .collect(),
  }),
- (being_indexed, _) => {
+ (being_indexed, _, _) => {
  let t = being_indexed.human_friendly_type();
  let article = article_for(t);
  Err(KclError::Semantic(KclErrorDetails {
@@ -1919,10 +1924,11 @@ impl Property {
  LiteralIdentifier::Identifier(identifier) => {
  let name = &identifier.name;
  if !computed {
- // Treat the property as a literal
+ // This is dot syntax. Treat the property as a literal.
  Ok(Property::String(name.to_string()))
  } else {
- // Actually evaluate memory to compute the property.
+ // This is bracket syntax. Actually evaluate memory to
+ // compute the property.
  let prop = exec_state.stack().get(name, property_src)?;
  jvalue_to_prop(prop, property_sr, name)
  }
@@ -1940,10 +1946,9 @@ impl Property {
  }))
  }
  }
- LiteralValue::String(s) => Ok(Property::String(s)),
  _ => Err(KclError::Semantic(KclErrorDetails {
  source_ranges: vec![sr],
- message: "Only strings or numbers (>= 0) can be properties/indexes".to_owned(),
+ message: "Only numbers (>= 0) can be indexes".to_owned(),
  })),
  }
  }

rust/kcl-lib/src/execution/mod.rs

@@ -1515,44 +1515,6 @@ const fnBox = box(3, 6, 10)"#;
  return myBox
 }
 
-const thisBox = box({start: [0,0], l: 6, w: 10, h: 3})
-"#;
- parse_execute(ast).await.unwrap();
- }
-
- #[tokio::test(flavor = "multi_thread")]
- async fn test_get_member_of_object_with_function_brace() {
- let ast = r#"fn box = (obj) => {
- let myBox = startSketchOn(XY)
- |> startProfile(at = obj["start"])
- |> line(end = [0, obj["l"]])
- |> line(end = [obj["w"], 0])
- |> line(end = [0, -obj["l"]])
- |> close()
- |> extrude(length = obj["h"])
-
- return myBox
-}
-
-const thisBox = box({start: [0,0], l: 6, w: 10, h: 3})
-"#;
- parse_execute(ast).await.unwrap();
- }
-
- #[tokio::test(flavor = "multi_thread")]
- async fn test_get_member_of_object_with_function_mix_period_brace() {
- let ast = r#"fn box = (obj) => {
- let myBox = startSketchOn(XY)
- |> startProfile(at = obj["start"])
- |> line(end = [0, obj["l"]])
- |> line(end = [obj["w"], 0])
- |> line(end = [10 - obj["w"], -obj.l])
- |> close()
- |> extrude(length = obj["h"])
-
- return myBox
-}
-
 const thisBox = box({start: [0,0], l: 6, w: 10, h: 3})
 "#;
  parse_execute(ast).await.unwrap();

rust/kcl-lib/src/parsing/parser.rs

@@ -1310,6 +1310,7 @@ fn member_expression_dot(i: &mut TokenSlice) -> PResult<(LiteralIdentifier, usiz
 /// E.g. `people[0]` or `people[i]` or `people['adam']`
 fn member_expression_subscript(i: &mut TokenSlice) -> PResult<(LiteralIdentifier, usize, bool)> {
  let _ = open_bracket.parse_next(i)?;
+ // TODO: This should be an expression, not just a literal or identifier.
  let property = alt((
  literal.map(LiteralIdentifier::Literal),
  nameable_identifier.map(Box::new).map(LiteralIdentifier::Identifier),

rust/kcl-lib/src/simulation_tests.rs

@@ -498,27 +498,6 @@ mod double_map_fn {
  super::execute(TEST_NAME, false).await
  }
 }
-mod property_of_object {
- const TEST_NAME: &str = "property_of_object";
-
- /// Test parsing KCL.
- #[test]
- fn parse() {
- super::parse(TEST_NAME)
- }
-
- /// Test that parsing and unparsing KCL produces the original KCL input.
- #[tokio::test(flavor = "multi_thread")]
- async fn unparse() {
- super::unparse(TEST_NAME).await
- }
-
- /// Test that KCL is executed correctly.
- #[tokio::test(flavor = "multi_thread")]
- async fn kcl_test_execute() {
- super::execute(TEST_NAME, false).await
- }
-}
 mod index_of_array {
  const TEST_NAME: &str = "index_of_array";
 
@@ -816,6 +795,27 @@ mod invalid_member_object_prop {
  super::execute(TEST_NAME, false).await
  }
 }
+mod invalid_member_object_using_string {
+ const TEST_NAME: &str = "invalid_member_object_using_string";
+
+ /// Test parsing KCL.
+ #[test]
+ fn parse() {
+ super::parse(TEST_NAME)
+ }
+
+ /// Test that parsing and unparsing KCL produces the original KCL input.
+ #[tokio::test(flavor = "multi_thread")]
+ async fn unparse() {
+ super::unparse(TEST_NAME).await
+ }
+
+ /// Test that KCL is executed correctly.
+ #[tokio::test(flavor = "multi_thread")]
+ async fn kcl_test_execute() {
+ super::execute(TEST_NAME, false).await
+ }
+}
 mod non_string_key_of_object {
  const TEST_NAME: &str = "non_string_key_of_object";
 

rust/kcl-lib/tests/computed_var/ast.snap

@@ -249,36 +249,6 @@ description: Result of parsing computed_var.kcl
  "type": "ExpressionStatement",
  "type": "ExpressionStatement"
  },
- {
- "commentStart": 0,
- "declaration": {
- "commentStart": 0,
- "end": 0,
- "id": {
- "commentStart": 0,
- "end": 0,
- "name": "p",
- "start": 0,
- "type": "Identifier"
- },
- "init": {
- "commentStart": 0,
- "end": 0,
- "raw": "\"foo\"",
- "start": 0,
- "type": "Literal",
- "type": "Literal",
- "value": "foo"
- },
- "start": 0,
- "type": "VariableDeclarator"
- },
- "end": 0,
- "kind": "const",
- "start": 0,
- "type": "VariableDeclaration",
- "type": "VariableDeclaration"
- },
  {
  "commentStart": 0,
  "declaration": {
@@ -373,7 +343,7 @@ description: Result of parsing computed_var.kcl
  },
  "init": {
  "commentStart": 0,
- "computed": true,
+ "computed": false,
  "end": 0,
  "object": {
  "commentStart": 0,
@@ -386,7 +356,7 @@ description: Result of parsing computed_var.kcl
  "property": {
  "commentStart": 0,
  "end": 0,
- "name": "p",
+ "name": "foo",
  "start": 0,
  "type": "Identifier",
  "type": "Identifier"
@@ -820,7 +790,7 @@ description: Result of parsing computed_var.kcl
  }
  }
  ],
- "6": [
+ "5": [
  {
  "commentStart": 0,
  "end": 0,
@@ -831,7 +801,7 @@ description: Result of parsing computed_var.kcl
  }
  }
  ],
- "7": [
+ "6": [
  {
  "commentStart": 0,
  "end": 0,
@@ -842,7 +812,7 @@ description: Result of parsing computed_var.kcl
  }
  }
  ],
- "8": [
+ "7": [
  {
  "commentStart": 0,
  "end": 0,

rust/kcl-lib/tests/computed_var/input.kcl

@@ -7,9 +7,8 @@ ten = arr[i]
 
 assert(ten, isEqualTo = 10, error = "oops")
 
-p = "foo"
 obj = { foo = 1, bar = 0 }
-one = obj[p]
+one = obj.foo
 
 assert(one, isEqualTo = 1, error = "oops")
 

rust/kcl-lib/tests/computed_var/program_memory.snap

@@ -117,10 +117,6 @@ description: Variables in memory after executing computed_var.kcl
  }
  }
  },
- "p": {
- "type": "String",
- "value": "foo"
- },
  "ten": {
  "type": "Number",
  "value": 10.0,

rust/kcl-lib/tests/computed_var/unparsed.snap

@@ -11,9 +11,8 @@ ten = arr[i]
 
 assert(ten, isEqualTo = 10, error = "oops")
 
-p = "foo"
 obj = { foo = 1, bar = 0 }
-one = obj[p]
+one = obj.foo
 
 assert(one, isEqualTo = 1, error = "oops")
 

rust/kcl-lib/tests/invalid_index_str/execution_error.snap

@@ -4,8 +4,7 @@ description: Error from executing invalid_index_str.kcl
 ---
 KCL Semantic error
 
- × semantic: Only integers >= 0 can be used as the index of an array, but
- │ you're using a string
+ × semantic: Only numbers (>= 0) can be indexes
  ╭─[2:5]
  1 │ arr = [1, 2, 3]
  2 │ x = arr["s"]

rust/kcl-lib/tests/invalid_member_object_prop/execution_error.snap

@@ -4,8 +4,7 @@ description: Error from executing invalid_member_object_prop.kcl
 ---
 KCL Semantic error
 
- × semantic: Only arrays and objects can be indexed, but you're trying to
- │ index a boolean (true/false value)
+ × semantic: Only numbers (>= 0) can be indexes
  ╭─[2:5]
  1 │ b = true
  2 │ x = b["property"]