Error on non-count indexing (#7539)

Signed-off-by: Nick Cameron <nrc@ncameron.org>

public/kcl-samples/car-wheel-assembly/main.kcl

@@ -15,14 +15,14 @@ import "car-tire.kcl" as carTire
 import * from "parameters.kcl"
 
 // Place the car rotor
-rotor = carRotor
+carRotor
   |> translate(x = 0, y = 0.5, z = 0)
 
 // Place the car wheel
 carWheel
 
 // Place the lug nuts
-lgnut = lugNut
+lugNut
   |> patternCircular3d(
        arcDegrees = 360,
        axis = [0, 1, 0],
@@ -32,19 +32,8 @@ lgnut = lugNut
      )
 
 // Place the brake caliper
-cal = brakeCaliper
+brakeCaliper
   |> translate(x = 0, y = 0.5, z = 0)
 
 // Place the car tire
 carTire
-
-
-fn animate(step: number(_)) {
-  angle = 0.6deg
-  rotate(rotor, pitch = angle)
-  rotate(lgnut, pitch = angle)
-  rotate(cal, pitch = angle)
-  rotate(carWheel, pitch = angle)
-  rotate(carTire, pitch = angle)
-  return 0
-}

public/kcl-samples/clock/main.kcl

@@ -369,7 +369,7 @@ profile007 = startProfile(
   |> line(%, endAbsolute = [profileStartX(%), profileStartY(%)])
   |> close(%)
 profile008 = circle(sketch005, center = [0, 0], diameter = nubDiameter)
-hourHand = subtract2d(profile007, tool = profile008)
+subtract2d(profile007, tool = profile008)
   |> extrude(%, length = 5)
   |> appearance(%, color = "#404040")
 
@@ -413,7 +413,7 @@ profile009 = startProfile(
   |> line(%, endAbsolute = [profileStartX(%), profileStartY(%)])
   |> close(%)
 profile010 = circle(sketch006, center = [0, 0], diameter = 30)
-minuteHand = subtract2d(profile009, tool = profile010)
+subtract2d(profile009, tool = profile010)
   |> extrude(%, length = 5)
   |> appearance(%, color = "#404040")
 
@@ -439,8 +439,3 @@ profile004 = startProfile(sketch003, at = [-slotWidth / 2, 200])
   |> extrude(%, length = -20)
 
 // todo: create cavity for the screw to slide into (need csg update)
-
-fn animate(step: number(_)) {
-  rotate(hourHand, yaw = -0.1deg)
-  return rotate(minuteHand, yaw = -0.6deg)
-}

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

@@ -17,11 +17,14 @@ use crate::{
     },
     fmt,
     modules::{ModuleId, ModulePath, ModuleRepr},
-    parsing::ast::types::{
+    parsing::{
+        ast::types::{
             Annotation, ArrayExpression, ArrayRangeExpression, AscribedExpression, BinaryExpression, BinaryOperator,
             BinaryPart, BodyItem, Expr, IfExpression, ImportPath, ImportSelector, ItemVisibility, LiteralIdentifier,
-        LiteralValue, MemberExpression, Name, Node, NodeRef, ObjectExpression, PipeExpression, Program, TagDeclarator,
+            LiteralValue, MemberExpression, Name, Node, NodeRef, ObjectExpression, PipeExpression, Program,
-        Type, UnaryExpression, UnaryOperator,
+            TagDeclarator, Type, UnaryExpression, UnaryOperator,
+        },
+        token::NumericSuffix,
     },
     source_range::SourceRange,
     std::args::TyF64,
@@ -1666,12 +1669,18 @@ impl Property {
             LiteralIdentifier::Literal(literal) => {
                 let value = literal.value.clone();
                 match value {
-                    LiteralValue::Number { value, .. } => {
+                    n @ LiteralValue::Number { value, suffix } => {
+                        if !matches!(suffix, NumericSuffix::None | NumericSuffix::Count) {
+                            return Err(KclError::new_semantic(KclErrorDetails::new(
+                                format!("{n} is not a valid index, indices must be non-dimensional numbers"),
+                                property_sr,
+                            )));
+                        }
                         if let Some(x) = crate::try_f64_to_usize(value) {
                             Ok(Property::UInt(x))
                         } else {
                             Err(KclError::new_semantic(KclErrorDetails::new(
-                                format!("{value} is not a valid index, indices must be whole numbers >= 0"),
+                                format!("{n} is not a valid index, indices must be whole numbers >= 0"),
                                 property_sr,
                             )))
                         }
@@ -1690,10 +1699,13 @@ fn jvalue_to_prop(value: &KclValue, property_sr: Vec<SourceRange>, name: &str) -
     let make_err =
         |message: String| Err::<Property, _>(KclError::new_semantic(KclErrorDetails::new(message, property_sr)));
     match value {
-        KclValue::Number{value: num, .. } => {
+        n @ KclValue::Number{value: num, ty, .. } => {
+            if !matches!(ty, NumericType::Known(crate::exec::UnitType::Count) | NumericType::Default { .. } | NumericType::Any ) {
+                return make_err(format!("arrays can only be indexed by non-dimensioned numbers, found {}", n.human_friendly_type()));
+            }
             let num = *num;
             if num < 0.0 {
-                return make_err(format!("'{num}' is negative, so you can't index an array with it"))
+                return make_err(format!("'{num}' is negative, so you can't index an array with it"));
             }
             let nearest_int = crate::try_f64_to_usize(num);
             if let Some(nearest_int) = nearest_int {
@@ -2141,4 +2153,23 @@ c = ((PI * 2) / 3): number(deg)
         let result = parse_execute(ast).await.unwrap();
         assert_eq!(result.exec_state.errors().len(), 2);
     }
+
+    #[tokio::test(flavor = "multi_thread")]
+    async fn non_count_indexing() {
+        let ast = r#"x = [0, 0]
+y = x[1mm]
+"#;
+        parse_execute(ast).await.unwrap_err();
+
+        let ast = r#"x = [0, 0]
+y = 1deg
+z = x[y]
+"#;
+        parse_execute(ast).await.unwrap_err();
+
+        let ast = r#"x = [0, 0]
+y = x[0mm + 1]
+"#;
+        parse_execute(ast).await.unwrap_err();
+    }
 }

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

@@ -805,43 +805,6 @@ impl ExecutorContext {
         Ok(outcome)
     }
 
-    pub async fn run_additional(&self, program: crate::Program) -> Result<ExecOutcome, KclErrorWithOutputs> {
-        assert!(!self.is_mock());
-
-        let (program, exec_state, result) = match cache::read_old_ast().await {
-            Some(cached_state) => {
-                let mut exec_state = cached_state.reconstitute_exec_state();
-                exec_state.mut_stack().restore_env(cached_state.main.result_env);
-
-                let result = self.run_concurrent(&program, &mut exec_state, None, true).await;
-
-                (program, exec_state, result)
-            }
-            None => {
-                let mut exec_state = ExecState::new(self);
-
-                let result = self.run_concurrent(&program, &mut exec_state, None, false).await;
-
-                (program, exec_state, result)
-            }
-        };
-
-        // Throw the error.
-        let result = result?;
-
-        // Save this as the last successful execution to the cache.
-        cache::write_old_ast(GlobalState::new(
-            exec_state.clone(),
-            self.settings.clone(),
-            program.ast,
-            result.0,
-        ))
-        .await;
-
-        let outcome = exec_state.into_exec_outcome(result.0, self).await;
-        Ok(outcome)
-    }
-
     /// Perform the execution of a program.
     ///
     /// To access non-fatal errors and warnings, extract them from the `ExecState`.

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

@@ -111,48 +111,6 @@ impl Context {
         ctx.run_with_caching(program).await
     }
 
-    /// Execute an additional program using the cache.
-    #[wasm_bindgen(js_name = executeAdditional)]
-    pub async fn execute_additional(
-        &self,
-        program_ast_json: &str,
-        path: Option<String>,
-        settings: &str,
-    ) -> Result<JsValue, JsValue> {
-        console_error_panic_hook::set_once();
-
-        self.execute_additional_typed(program_ast_json, path, settings)
-            .await
-            .and_then(|outcome| {
-                JsValue::from_serde(&outcome).map_err(|e| {
-                    // The serde-wasm-bindgen does not work here because of weird HashMap issues.
-                    // DO NOT USE serde_wasm_bindgen::to_value it will break the frontend.
-                    KclErrorWithOutputs::no_outputs(KclError::internal(format!(
-                        "Could not serialize successful KCL result. {TRUE_BUG} Details: {e}"
-                    )))
-                })
-            })
-            .map_err(|e: KclErrorWithOutputs| JsValue::from_serde(&e).unwrap())
-    }
-
-    async fn execute_additional_typed(
-        &self,
-        program_ast_json: &str,
-        path: Option<String>,
-        settings: &str,
-    ) -> Result<ExecOutcome, KclErrorWithOutputs> {
-        let program: Program = serde_json::from_str(program_ast_json).map_err(|e| {
-            let err = KclError::internal(format!("Could not deserialize KCL AST. {TRUE_BUG} Details: {e}"));
-            KclErrorWithOutputs::no_outputs(err)
-        })?;
-        let ctx = self.create_executor_ctx(settings, path, false).map_err(|e| {
-            KclErrorWithOutputs::no_outputs(KclError::internal(format!(
-                "Could not create KCL executor context. {TRUE_BUG} Details: {e}"
-            )))
-        })?;
-        ctx.run_additional(program).await
-    }
-
     /// Reset the scene and bust the cache.
     /// ONLY use this if you absolutely need to reset the scene and bust the cache.
     #[wasm_bindgen(js_name = bustCacheAndResetScene)]

src/components/ModelingSidebar/ModelingPane.tsx

@@ -7,8 +7,6 @@ import type { CustomIconName } from '@src/components/CustomIcon'
 import Tooltip from '@src/components/Tooltip'
 
 import styles from './ModelingPane.module.css'
-import { reportRejection } from '@src/lib/trap'
-import { kclManager } from '@src/lib/singletons'
 
 export interface ModelingPaneProps {
   id: string
@@ -21,28 +19,6 @@ export interface ModelingPaneProps {
   onClose: () => void
 }
 
-const ANIMATE_INTERVAL = 16 // milliseconds
-let animateTimeout: NodeJS.Timeout | null = null
-
-function doAnimate() {
-  ;(async () => {
-    await kclManager.executeAnimate()
-    if (animateTimeout !== null) {
-      animateTimeout = setTimeout(doAnimate, ANIMATE_INTERVAL)
-    }
-  })().catch(reportRejection)
-}
-
-function onPlay() {
-  console.log('Play button clicked')
-  if (animateTimeout) {
-    clearTimeout(animateTimeout)
-    animateTimeout = null
-  } else {
-    animateTimeout = setTimeout(doAnimate, ANIMATE_INTERVAL)
-  }
-}
-
 export const ModelingPaneHeader = ({
   id,
   icon,
@@ -64,20 +40,6 @@ export const ModelingPaneHeader = ({
         )}
         <span data-testid={id + '-header'}>{title}</span>
       </div>
-      {id === 'code' && (
-        <ActionButton
-          Element="button"
-          iconStart={{
-            icon: 'play',
-            iconClassName: '!text-current',
-            bgClassName: 'bg-transparent dark:bg-transparent',
-          }}
-          className="!p-0 !bg-transparent hover:text-primary border-transparent dark:!border-transparent hover:!border-primary dark:hover:!border-chalkboard-70 !outline-none"
-          onClick={() => onPlay()}
-        >
-          <Tooltip position="bottom-right">Play</Tooltip>
-        </ActionButton>
-      )}
       {Menu instanceof Function ? <Menu /> : Menu}
       <ActionButton
         Element="button"

src/lang/KclSingleton.ts

@@ -17,12 +17,7 @@ import {
   compilationErrorsToDiagnostics,
   kclErrorsToDiagnostics,
 } from '@src/lang/errors'
-import {
+import { executeAst, executeAstMock, lintAst } from '@src/lang/langHelpers'
-  executeAdditional,
-  executeAst,
-  executeAstMock,
-  lintAst,
-} from '@src/lang/langHelpers'
 import { getNodeFromPath, getSettingsAnnotation } from '@src/lang/queryAst'
 import { CommandLogType } from '@src/lang/std/commandLog'
 import type { EngineCommandManager } from '@src/lang/std/engineConnection'
@@ -111,7 +106,6 @@ export class KclManager extends EventTarget {
     preComments: [],
     commentStart: 0,
   }
-  private _animateState = { step: 0 }
   private _execState: ExecState = emptyExecState()
   private _variables: VariableMap = {}
   lastSuccessfulVariables: VariableMap = {}
@@ -456,7 +450,6 @@ export class KclManager extends EventTarget {
     const ast = args.ast || this.ast
     markOnce('code/startExecuteAst')
 
-    this._animateState.step = 0
     const currentExecutionId = args.executionId || Date.now()
     this._cancelTokens.set(currentExecutionId, false)
 
@@ -548,39 +541,6 @@ export class KclManager extends EventTarget {
     markOnce('code/endExecuteAst')
   }
 
-  async executeAnimate(): Promise<void> {
-    if (this.isExecuting) {
-      return
-    }
-
-    const code = `animate(step = ${this._animateState.step})`
-    const result = parse(code)
-    if (err(result)) {
-      console.error(result)
-      return
-    }
-    const program = result.program
-    if (!program) {
-      console.error('No program returned from parse')
-      return
-    }
-
-    const { errors } = await executeAdditional({
-      ast: program,
-      path: this.singletons.codeManager.currentFilePath || undefined,
-      rustContext: this.singletons.rustContext,
-    })
-    if (errors.length > 0) {
-      console.error('Errors executing animate:', errors)
-      return
-    }
-
-    this._animateState.step += 1
-    if (this._animateState.step === Number.MAX_SAFE_INTEGER) {
-      this._animateState.step = 0
-    }
-  }
-
   // DO NOT CALL THIS from codemirror ever.
   async executeAstMock(ast: Program): Promise<null | Error> {
     await this.ensureWasmInit()

src/lang/langHelpers.ts

@@ -84,31 +84,6 @@ export async function executeAst({
   }
 }
 
-export async function executeAdditional({
-  ast,
-  rustContext,
-  path,
-}: {
-  ast: Node<Program>
-  rustContext: RustContext
-  path?: string
-}): Promise<ExecutionResult> {
-  try {
-    const settings = await jsAppSettings()
-    const execState = await rustContext.executeAdditional(ast, settings, path)
-
-    await rustContext.waitForAllEngineCommands()
-    return {
-      logs: [],
-      errors: [],
-      execState,
-      isInterrupted: false,
-    }
-  } catch (e: any) {
-    return handleExecuteError(e)
-  }
-}
-
 export async function executeAstMock({
   ast,
   rustContext,

src/lib/rustContext.ts

@@ -96,34 +96,6 @@ export default class RustContext {
     }
   }
 
-  /** Execute an additional program using the cache. */
-  async executeAdditional(
-    node: Node<Program>,
-    settings: DeepPartial<Configuration>,
-    path?: string
-  ): Promise<ExecState> {
-    const instance = await this._checkInstance()
-
-    try {
-      const result = await instance.executeAdditional(
-        JSON.stringify(node),
-        path,
-        JSON.stringify(settings)
-      )
-      // Set the default planes, safe to call after execute.
-      const outcome = execStateFromRust(result)
-
-      this._defaultPlanes = outcome.defaultPlanes
-
-      // Return the result.
-      return outcome
-    } catch (e: any) {
-      const err = errFromErrWithOutputs(e)
-      this._defaultPlanes = err.defaultPlanes
-      return Promise.reject(err)
-    }
-  }
-
   /** Execute a program with in mock mode. */
   async executeMock(
     node: Node<Program>,