Give a warning when using per-project default units (#5956)

* Give a warning when using per-project default units

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

* Factor non-settings out of MetaSettings

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

* Fix formatting

* Fix code pane e2e test

* Fix callstack blowup in edit flow

* Avoid dumb timeout issue with command registration in test

* Use a safer way to wait for modeling command registration in test

---------

Signed-off-by: Nick Cameron <nrc@ncameron.org>
Co-authored-by: Nick Cameron <nrc@ncameron.org>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Pierre Jacquier <pierre@zoo.dev>
Co-authored-by: Frank Noirot <frankjohnson1993@gmail.com>

e2e/playwright/code-pane-and-errors.spec.ts

@@ -21,14 +21,15 @@ test.describe('Code pane and errors', { tag: ['@skipWin'] }, () => {
     await page.addInitScript(() => {
       localStorage.setItem(
         'persistCode',
-        `// Extruded Triangle
-  sketch001 = startSketchOn(XZ)
-    |> startProfileAt([0, 0], %)
-    |> line(end = [10, 0])
-    |> line(end = [-5, 10])
-    |> line(endAbsolute = [profileStartX(%), profileStartY(%)])
-    |> close()
-  extrude001 = extrude(sketch001, length = 5)`
+        `@settings(defaultLengthUnit = in)
+// Extruded Triangle
+sketch001 = startSketchOn(XZ)
+  |> startProfileAt([0, 0], %)
+  |> line(end = [10, 0])
+  |> line(end = [-5, 10])
+  |> line(endAbsolute = [profileStartX(%), profileStartY(%)])
+  |> close()
+extrude001 = extrude(sketch001, length = 5)`
       )
     })
 

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

@@ -927,9 +927,8 @@ fn get_autocomplete_string_from_schema(schema: &schemars::schema::Schema) -> Res
 mod tests {
     use pretty_assertions::assert_eq;
 
-    use crate::docs::kcl_doc::{self, DocData};
-
     use super::StdLibFn;
+    use crate::docs::kcl_doc::{self, DocData};
 
     #[test]
     fn test_serialize_function() {

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

@@ -3,6 +3,7 @@ use std::collections::HashMap;
 use async_recursion::async_recursion;
 use indexmap::IndexMap;
 
+use super::kcl_value::TypeDef;
 use crate::{
     engine::ExecutionKind,
     errors::{KclError, KclErrorDetails},
@@ -31,8 +32,6 @@ use crate::{
     CompilationError,
 };
 
-use super::kcl_value::TypeDef;
-
 enum StatementKind<'a> {
     Declaration { name: &'a str },
     Expression,
@@ -59,7 +58,9 @@ impl ExecutorContext {
         for annotation in annotations {
             if annotation.name() == Some(annotations::SETTINGS) {
                 if matches!(body_type, BodyType::Root) {
-                    exec_state.mod_local.settings.update_from_annotation(annotation)?;
+                    if exec_state.mod_local.settings.update_from_annotation(annotation)? {
+                        exec_state.mod_local.explicit_length_units = true;
+                    }
                     let new_units = exec_state.length_unit();
                     if !self.engine.execution_kind().await.is_isolated() {
                         self.engine
@@ -299,7 +300,7 @@ impl ExecutorContext {
                     let impl_kind = annotations::get_impl(&ty.outer_attrs, metadata.source_range)?.unwrap_or_default();
                     match impl_kind {
                         annotations::Impl::Rust => {
-                            let std_path = match &exec_state.mod_local.settings.std_path {
+                            let std_path = match &exec_state.mod_local.std_path {
                                 Some(p) => p,
                                 None => {
                                     return Err(KclError::Semantic(KclErrorDetails {
@@ -585,7 +586,7 @@ impl ExecutorContext {
     ) -> Result<KclValue, KclError> {
         let item = match init {
             Expr::None(none) => KclValue::from(none),
-            Expr::Literal(literal) => KclValue::from_literal((**literal).clone(), &exec_state.mod_local.settings),
+            Expr::Literal(literal) => KclValue::from_literal((**literal).clone(), exec_state),
             Expr::TagDeclarator(tag) => tag.execute(exec_state).await?,
             Expr::Name(name) => {
                 let value = name.get_result(exec_state, self).await?.clone();
@@ -616,7 +617,7 @@ impl ExecutorContext {
                     .unwrap_or(false);
 
                 if rust_impl {
-                    if let Some(std_path) = &exec_state.mod_local.settings.std_path {
+                    if let Some(std_path) = &exec_state.mod_local.std_path {
                         let (func, props) = crate::std::std_fn(std_path, statement_kind.expect_name());
                         KclValue::Function {
                             value: FunctionSource::Std {
@@ -719,10 +720,7 @@ impl BinaryPart {
     #[async_recursion]
     pub async fn get_result(&self, exec_state: &mut ExecState, ctx: &ExecutorContext) -> Result<KclValue, KclError> {
         match self {
-            BinaryPart::Literal(literal) => Ok(KclValue::from_literal(
-                (**literal).clone(),
-                &exec_state.mod_local.settings,
-            )),
+            BinaryPart::Literal(literal) => Ok(KclValue::from_literal((**literal).clone(), exec_state)),
             BinaryPart::Name(name) => name.get_result(exec_state, ctx).await.cloned(),
             BinaryPart::BinaryExpression(binary_expression) => binary_expression.get_result(exec_state, ctx).await,
             BinaryPart::CallExpression(call_expression) => call_expression.execute(exec_state, ctx).await,
@@ -1854,15 +1852,12 @@ fn assign_args_to_params(
         return Err(err_wrong_number_args);
     }
 
-    let mem = &mut exec_state.mod_local.stack;
-    let settings = &exec_state.mod_local.settings;
-
     // Add the arguments to the memory.  A new call frame should have already
     // been created.
     for (index, param) in function_expression.params.iter().enumerate() {
         if let Some(arg) = args.get(index) {
             // Argument was provided.
-            mem.add(
+            exec_state.mut_stack().add(
                 param.identifier.name.clone(),
                 arg.value.clone(),
                 (&param.identifier).into(),
@@ -1872,11 +1867,10 @@ fn assign_args_to_params(
             if let Some(ref default_val) = param.default_value {
                 // If the corresponding parameter is optional,
                 // then it's fine, the user doesn't need to supply it.
-                mem.add(
-                    param.identifier.name.clone(),
-                    KclValue::from_default_param(default_val.clone(), settings),
-                    (&param.identifier).into(),
-                )?;
+                let value = KclValue::from_default_param(default_val.clone(), exec_state);
+                exec_state
+                    .mut_stack()
+                    .add(param.identifier.name.clone(), value, (&param.identifier).into())?;
             } else {
                 // But if the corresponding parameter was required,
                 // then the user has called with too few arguments.
@@ -1916,8 +1910,6 @@ fn assign_args_to_params_kw(
     // Add the arguments to the memory.  A new call frame should have already
     // been created.
     let source_ranges = vec![function_expression.into()];
-    let mem = &mut exec_state.mod_local.stack;
-    let settings = &exec_state.mod_local.settings;
 
     for param in function_expression.params.iter() {
         if param.labeled {
@@ -1925,7 +1917,7 @@ fn assign_args_to_params_kw(
             let arg_val = match arg {
                 Some(arg) => arg.value.clone(),
                 None => match param.default_value {
-                    Some(ref default_val) => KclValue::from_default_param(default_val.clone(), settings),
+                    Some(ref default_val) => KclValue::from_default_param(default_val.clone(), exec_state),
                     None => {
                         return Err(KclError::Semantic(KclErrorDetails {
                             source_ranges,
@@ -1937,7 +1929,9 @@ fn assign_args_to_params_kw(
                     }
                 },
             };
-            mem.add(param.identifier.name.clone(), arg_val, (&param.identifier).into())?;
+            exec_state
+                .mut_stack()
+                .add(param.identifier.name.clone(), arg_val, (&param.identifier).into())?;
         } else {
             let Some(unlabeled) = args.unlabeled.take() else {
                 let param_name = &param.identifier.name;
@@ -1954,7 +1948,7 @@ fn assign_args_to_params_kw(
                     })
                 });
             };
-            mem.add(
+            exec_state.mut_stack().add(
                 param.identifier.name.clone(),
                 unlabeled.value.clone(),
                 (&param.identifier).into(),

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

@@ -4,10 +4,11 @@ use anyhow::Result;
 use schemars::JsonSchema;
 use serde::Serialize;
 
-use super::{memory::EnvironmentRef, MetaSettings};
+use super::{types::UnitLen, EnvironmentRef, ExecState, MetaSettings};
 use crate::{
     errors::KclErrorDetails,
     execution::{
+        annotations::{SETTINGS, SETTINGS_UNIT_LENGTH},
         types::{NumericType, PrimitiveType, RuntimeType},
         Face, Helix, ImportedGeometry, Metadata, Plane, Sketch, Solid, TagIdentifier,
     },
@@ -15,7 +16,7 @@ use crate::{
         DefaultParamVal, FunctionExpression, KclNone, Literal, LiteralValue, Node, TagDeclarator, TagNode,
     },
     std::StdFnProps,
-    KclError, ModuleId, SourceRange,
+    CompilationError, KclError, ModuleId, SourceRange,
 };
 
 pub type KclObjectFields = HashMap<String, KclValue>;
@@ -308,22 +309,38 @@ impl KclValue {
         }
     }
 
-    pub(crate) fn from_literal(literal: Node<Literal>, settings: &MetaSettings) -> Self {
+    pub(crate) fn from_literal(literal: Node<Literal>, exec_state: &mut ExecState) -> Self {
         let meta = vec![literal.metadata()];
         match literal.inner.value {
-            LiteralValue::Number { value, suffix } => KclValue::Number {
-                value,
-                meta,
-                ty: NumericType::from_parsed(suffix, settings),
-            },
+            LiteralValue::Number { value, suffix } => {
+                let ty = NumericType::from_parsed(suffix, &exec_state.mod_local.settings);
+                if let NumericType::Default { len, .. } = &ty {
+                    if !exec_state.mod_local.explicit_length_units && *len != UnitLen::Mm {
+                        exec_state.warn(
+                            CompilationError::err(
+                                literal.as_source_range(),
+                                "Project-wide units are deprecated. Prefer to use per-file default units.",
+                            )
+                            .with_suggestion(
+                                "Fix by adding per-file settings",
+                                format!("@{SETTINGS}({SETTINGS_UNIT_LENGTH} = {len})\n"),
+                                // Insert at the start of the file.
+                                Some(SourceRange::new(0, 0, literal.module_id)),
+                                crate::errors::Tag::Deprecated,
+                            ),
+                        );
+                    }
+                }
+                KclValue::Number { value, meta, ty }
+            }
             LiteralValue::String(value) => KclValue::String { value, meta },
             LiteralValue::Bool(value) => KclValue::Bool { value, meta },
         }
     }
 
-    pub(crate) fn from_default_param(param: DefaultParamVal, settings: &MetaSettings) -> Self {
+    pub(crate) fn from_default_param(param: DefaultParamVal, exec_state: &mut ExecState) -> Self {
         match param {
-            DefaultParamVal::Literal(lit) => Self::from_literal(lit, settings),
+            DefaultParamVal::Literal(lit) => Self::from_literal(lit, exec_state),
             DefaultParamVal::KclNone(none) => KclValue::KclNone {
                 value: none,
                 meta: Default::default(),

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

@@ -992,7 +992,11 @@ mod tests {
     use pretty_assertions::assert_eq;
 
     use super::*;
-    use crate::{errors::KclErrorDetails, execution::memory::Stack, ModuleId};
+    use crate::{
+        errors::{KclErrorDetails, Severity},
+        execution::memory::Stack,
+        ModuleId,
+    };
 
     /// Convenience function to get a JSON value from memory and unwrap.
     #[track_caller]
@@ -1595,6 +1599,34 @@ const inInches = 2.0 * inch()"#;
         );
     }
 
+    #[tokio::test(flavor = "multi_thread")]
+    async fn test_unit_suggest() {
+        let src = "foo = 42";
+        let program = crate::Program::parse_no_errs(src).unwrap();
+        let ctx = ExecutorContext {
+            engine: Arc::new(Box::new(
+                crate::engine::conn_mock::EngineConnection::new().await.unwrap(),
+            )),
+            fs: Arc::new(crate::fs::FileManager::new()),
+            stdlib: Arc::new(crate::std::StdLib::new()),
+            settings: ExecutorSettings {
+                units: UnitLength::Ft,
+                ..Default::default()
+            },
+            context_type: ContextType::Mock,
+        };
+        let mut exec_state = ExecState::new(&ctx);
+        ctx.run(&program, &mut exec_state).await.unwrap();
+        let errs = exec_state.errors();
+        assert_eq!(errs.len(), 1, "{errs:?}");
+        let warn = &errs[0];
+        assert_eq!(warn.severity, Severity::Warning);
+        assert_eq!(
+            warn.apply_suggestion(src).unwrap(),
+            "@settings(defaultLengthUnit = ft)\nfoo = 42"
+        )
+    }
+
     #[tokio::test(flavor = "multi_thread")]
     async fn test_zero_param_fn() {
         let ast = r#"const sigmaAllow = 35000 // psi

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

@@ -72,6 +72,8 @@ pub(super) struct ModuleState {
     pub module_exports: Vec<String>,
     /// Settings specified from annotations.
     pub settings: MetaSettings,
+    pub(super) explicit_length_units: bool,
+    pub(super) std_path: Option<String>,
 }
 
 impl ExecState {
@@ -296,10 +298,11 @@ impl ModuleState {
             stack: memory.new_stack(),
             pipe_value: Default::default(),
             module_exports: Default::default(),
+            explicit_length_units: false,
+            std_path,
             settings: MetaSettings {
                 default_length_units: exec_settings.units.into(),
                 default_angle_units: Default::default(),
-                std_path,
             },
         }
     }
@@ -311,22 +314,23 @@ impl ModuleState {
 pub struct MetaSettings {
     pub default_length_units: types::UnitLen,
     pub default_angle_units: types::UnitAngle,
-    pub std_path: Option<String>,
 }
 
 impl MetaSettings {
     pub(crate) fn update_from_annotation(
         &mut self,
         annotation: &crate::parsing::ast::types::Node<Annotation>,
-    ) -> Result<(), KclError> {
+    ) -> Result<bool, KclError> {
         let properties = annotations::expect_properties(annotations::SETTINGS, annotation)?;
 
+        let mut updated_len = false;
         for p in properties {
             match &*p.inner.key.name {
                 annotations::SETTINGS_UNIT_LENGTH => {
                     let value = annotations::expect_ident(&p.inner.value)?;
                     let value = types::UnitLen::from_str(value, annotation.as_source_range())?;
                     self.default_length_units = value;
+                    updated_len = true;
                 }
                 annotations::SETTINGS_UNIT_ANGLE => {
                     let value = annotations::expect_ident(&p.inner.value)?;
@@ -346,6 +350,6 @@ impl MetaSettings {
             }
         }
 
-        Ok(())
+        Ok(updated_len)
     }
 }

rust/kcl-lib/src/log.rs

@@ -6,8 +6,13 @@ use dhat::{HeapStats, Profiler};
 use web_time::Instant;
 
 const LOG_ENV_VAR: &str = "ZOO_LOG";
+const FORCE_LOGGING: bool = false;
+
 lazy_static::lazy_static! {
     static ref ENABLED: bool = {
+        if FORCE_LOGGING {
+            return true;
+        }
         let env_var = env::var(LOG_ENV_VAR);
         let Ok(env_var) = env_var else {
             return false;

rust/kcl-lib/src/parsing/ast/types/mod.rs

@@ -1270,7 +1270,7 @@ pub enum NonCodeValue {
     /// An example of this is the following:
     /// ```no_run
     /// /* This is a
-    ///     block comment */
+    /// block comment */
     /// 1 + 1
     /// ```
     /// Now this is important. The block comment is attached to the next line.

rust/kcl-lib/src/std/sweep.rs

@@ -151,7 +151,6 @@ pub async fn sweep(exec_state: &mut ExecState, args: Args) -> Result<KclValue, K
 ///
 /// sweep(circleSketch, path = sweepPath, sectional = true)
 /// ```
-///
 
 #[stdlib {
     name = "sweep",

rust/kcl-lib/std/math.kcl

@@ -1,4 +1,5 @@
 @no_std
+@settings(defaultLengthUnit = mm)
 
 /// The value of `pi`, Archimedes’ constant (π).
 ///

rust/kcl-lib/std/prelude.kcl

@@ -1,4 +1,5 @@
 @no_std
+@settings(defaultLengthUnit = mm)
 
 // Note that everything in the prelude is treated as exported.
 

src/components/UnitsMenu.tsx

@@ -58,7 +58,6 @@ export function UnitsMenu() {
                         const newCode = changeKclSettings(codeManager.code, {
                           defaultLengthUnits: unitLengthToUnitLen(unit),
                           defaultAngleUnits: { type: 'Degrees' },
-                          stdPath: null,
                         })
                         if (err(newCode)) {
                           toast.error(