From fed922cfb8aba2bc7db973e3605a8933e0fa0b30 Mon Sep 17 00:00:00 2001 From: Jonathan Tran Date: Wed, 5 Feb 2025 20:23:29 -0500 Subject: [PATCH] Fix to invalidate execution cache when top-level annotations change (#5238) * Fix to invalidate cache when top-level annotations change * Change to BTreeMap to allow deterministic iteration * Fix execution cache to look at annotations --- src/wasm-lib/kcl/Cargo.toml | 2 +- src/wasm-lib/kcl/src/execution/cache.rs | 81 ++++++++++++++++++- src/wasm-lib/kcl/src/parsing/ast/digest.rs | 69 +++++++++++++++- src/wasm-lib/kcl/src/parsing/ast/types/mod.rs | 59 +++++++++----- src/wasm-lib/kcl/src/parsing/parser.rs | 10 +-- 5 files changed, 193 insertions(+), 28 deletions(-) diff --git a/src/wasm-lib/kcl/Cargo.toml b/src/wasm-lib/kcl/Cargo.toml index 4482cd16b..5d2478438 100644 --- a/src/wasm-lib/kcl/Cargo.toml +++ b/src/wasm-lib/kcl/Cargo.toml @@ -32,6 +32,7 @@ gltf-json = "1.4.1" http = { workspace = true } image = { version = "0.25.5", default-features = false, features = ["png"] } indexmap = { version = "2.7.0", features = ["serde"] } +itertools = "0.13.0" kittycad = { workspace = true } kittycad-modeling-cmds = { workspace = true } lazy_static = "1.5.0" @@ -115,7 +116,6 @@ expectorate = "1.1.0" handlebars = "6.3.0" image = { version = "0.25.5", default-features = false, features = ["png"] } insta = { version = "1.41.1", features = ["json", "filters", "redactions"] } -itertools = "0.13.0" miette = { version = "7.2.0", features = ["fancy"] } pretty_assertions = "1.4.1" tokio = { version = "1.41.1", features = ["rt-multi-thread", "macros", "time"] } diff --git a/src/wasm-lib/kcl/src/execution/cache.rs b/src/wasm-lib/kcl/src/execution/cache.rs index 7fab073f4..27ea99657 100644 --- a/src/wasm-lib/kcl/src/execution/cache.rs +++ b/src/wasm-lib/kcl/src/execution/cache.rs @@ -2,11 +2,12 @@ use std::sync::Arc; +use itertools::{EitherOrBoth, Itertools}; use tokio::sync::RwLock; use crate::{ execution::{ExecState, ExecutorSettings}, - parsing::ast::types::{Node, Program}, + parsing::ast::types::{Node, NonCodeValue, Program}, walk::Node as WalkNode, }; @@ -113,6 +114,44 @@ pub(super) async fn get_changed_program(old: CacheInformation<'_>, new: CacheInf return CacheResult::NoAction(try_reapply_settings); } + // Check if the annotations are different. + if !old_ast.annotations().zip_longest(new_ast.annotations()).all(|pair| { + match pair { + EitherOrBoth::Both(old, new) => { + // Compare annotations, ignoring source ranges. Digests must + // have been computed before this. + match (&old.value, &new.value) { + ( + NonCodeValue::Annotation { name, properties }, + NonCodeValue::Annotation { + name: new_name, + properties: new_properties, + }, + ) => { + name.digest == new_name.digest + && properties + .as_ref() + .map(|props| props.iter().map(|p| p.digest).collect::>()) + == new_properties + .as_ref() + .map(|props| props.iter().map(|p| p.digest).collect::>()) + } + _ => false, + } + } + _ => false, + } + }) { + // If any of the annotations are different at the beginning of the + // program, it's likely the settings, and we have to bust the cache and + // re-execute the whole thing. + return CacheResult::ReExecute { + clear_scene: true, + reapply_settings: true, + program: new.ast.clone(), + }; + } + // Check if the changes were only to Non-code areas, like comments or whitespace. let (clear_scene, program) = generate_changed_program(old_ast, new_ast); CacheResult::ReExecute { @@ -126,6 +165,8 @@ pub(super) async fn get_changed_program(old: CacheInformation<'_>, new: CacheInf /// way in which this gets invoked should always be through /// [get_changed_program]. This is purely to contain the logic on /// how we construct a new [CacheResult]. +/// +/// Digests *must* be computed before calling this. fn generate_changed_program(old_ast: Node, mut new_ast: Node) -> (bool, Node) { if !old_ast.body.iter().zip(new_ast.body.iter()).all(|(old, new)| { let old_node: WalkNode = old.into(); @@ -466,4 +507,42 @@ shell({ faces = ['end'], thickness = 0.25 }, firstSketch)"#; assert_eq!(result, CacheResult::NoAction(true)); } + + // Changing the units settings using an annotation with the exact same file + // should bust the cache. + #[tokio::test(flavor = "multi_thread")] + async fn test_get_changed_program_same_code_but_different_unit_setting_using_annotation() { + let old_code = r#"@settings(defaultLengthUnit = in) +startSketchOn('XY') +"#; + let new_code = r#"@settings(defaultLengthUnit = mm) +startSketchOn('XY') +"#; + + let (program, ctx, _) = parse_execute(old_code).await.unwrap(); + + let mut new_program = crate::Program::parse_no_errs(new_code).unwrap(); + new_program.compute_digest(); + + let result = get_changed_program( + CacheInformation { + ast: &program.ast, + settings: &ctx.settings, + }, + CacheInformation { + ast: &new_program.ast, + settings: &ctx.settings, + }, + ) + .await; + + assert_eq!( + result, + CacheResult::ReExecute { + clear_scene: true, + reapply_settings: true, + program: new_program.ast + } + ); + } } diff --git a/src/wasm-lib/kcl/src/parsing/ast/digest.rs b/src/wasm-lib/kcl/src/parsing/ast/digest.rs index 4131924b6..79768fbd7 100644 --- a/src/wasm-lib/kcl/src/parsing/ast/digest.rs +++ b/src/wasm-lib/kcl/src/parsing/ast/digest.rs @@ -1,6 +1,9 @@ use sha2::{Digest as DigestTrait, Sha256}; -use super::types::{DefaultParamVal, ItemVisibility, LabelledExpression, LiteralValue, VariableKind}; +use super::types::{ + DefaultParamVal, ItemVisibility, LabelledExpression, LiteralValue, NonCodeMeta, NonCodeNode, NonCodeValue, + VariableKind, +}; use crate::parsing::ast::types::{ ArrayExpression, ArrayRangeExpression, BinaryExpression, BinaryPart, BodyItem, CallExpression, CallExpressionKw, ElseIf, Expr, ExpressionStatement, FnArgType, FunctionExpression, Identifier, IfExpression, ImportItem, @@ -79,12 +82,60 @@ impl Program { for body_item in slf.body.iter_mut() { hasher.update(body_item.compute_digest()); } + // This contains settings annotations. + hasher.update(slf.non_code_meta.compute_digest()); if let Some(shebang) = &slf.shebang { hasher.update(&shebang.inner.content); } }); } +impl NonCodeMeta { + compute_digest!(|slf, hasher| { + for non_code_node in slf.start_nodes.iter_mut() { + hasher.update(non_code_node.compute_digest()); + } + for (_, non_code_nodes) in slf.non_code_nodes.iter_mut() { + for non_code_node in non_code_nodes.iter_mut() { + hasher.update(non_code_node.compute_digest()); + } + } + }); +} + +impl NonCodeNode { + compute_digest!(|slf, hasher| { + hasher.update(slf.value.compute_digest()); + }); +} + +impl NonCodeValue { + pub fn compute_digest(&mut self) -> Digest { + let mut hasher = Sha256::new(); + match self { + NonCodeValue::InlineComment { .. } => {} + NonCodeValue::BlockComment { .. } => {} + NonCodeValue::NewLineBlockComment { .. } => {} + NonCodeValue::NewLine => {} + NonCodeValue::Annotation { + ref mut name, + properties, + } => { + hasher.update(name.compute_digest()); + if let Some(properties) = properties { + hasher.update(properties.len().to_ne_bytes()); + for property in properties.iter_mut() { + hasher.update(property.compute_digest()); + } + } else { + hasher.update("no_properties"); + } + } + } + hasher.finalize().into() + } +} + impl BodyItem { pub fn compute_digest(&mut self) -> Digest { match self { @@ -460,4 +511,20 @@ mod test { assert_eq!(prog1_digest, prog3_digest); } + + #[tokio::test(flavor = "multi_thread")] + async fn test_annotations_digest() { + // Settings annotations should be included in the digest. + let prog1_string = r#"@settings(defaultLengthUnit = in) +startSketchOn('XY') +"#; + let prog1_digest = crate::parsing::top_level_parse(prog1_string).unwrap().compute_digest(); + + let prog2_string = r#"@settings(defaultLengthUnit = mm) +startSketchOn('XY') +"#; + let prog2_digest = crate::parsing::top_level_parse(prog2_string).unwrap().compute_digest(); + + assert!(prog1_digest != prog2_digest); + } } diff --git a/src/wasm-lib/kcl/src/parsing/ast/types/mod.rs b/src/wasm-lib/kcl/src/parsing/ast/types/mod.rs index 35e8d9b9d..de5e5523f 100644 --- a/src/wasm-lib/kcl/src/parsing/ast/types/mod.rs +++ b/src/wasm-lib/kcl/src/parsing/ast/types/mod.rs @@ -2,7 +2,7 @@ use std::{ cell::RefCell, - collections::HashMap, + collections::{BTreeMap, HashMap}, fmt, ops::{Deref, DerefMut, RangeInclusive}, rc::Rc, @@ -254,15 +254,26 @@ impl Node { Ok(findings) } - /// Get the annotations for the meta settings from the kcl file. - pub fn get_meta_settings(&self) -> Result, KclError> { - let annotations = self - .non_code_meta + pub fn annotations(&self) -> impl Iterator> { + self.non_code_meta .start_nodes .iter() - .filter_map(|n| n.annotation().map(|result| (result, n.as_source_range()))); - for (annotation, source_range) in annotations { + .filter(|n| n.value_is_annotation()) + } + + pub fn annotations_mut(&mut self) -> impl Iterator> { + self.non_code_meta + .start_nodes + .iter_mut() + .filter(|n| n.value_is_annotation()) + } + + /// Get the annotations for the meta settings from the kcl file. + pub fn get_meta_settings(&self) -> Result, KclError> { + for annotation_node in self.annotations() { + let annotation = &annotation_node.value; if annotation.annotation_name() == Some(annotations::SETTINGS) { + let source_range = annotation_node.as_source_range(); let mut meta_settings = crate::execution::MetaSettings::default(); meta_settings.update_from_annotation(annotation, source_range)?; return Ok(Some(meta_settings)); @@ -275,17 +286,15 @@ impl Node { pub fn change_meta_settings(&mut self, settings: crate::execution::MetaSettings) -> Result { let mut new_program = self.clone(); let mut found = false; - for node in &mut new_program.non_code_meta.start_nodes { - if let Some(annotation) = node.annotation() { - if annotation.annotation_name() == Some(annotations::SETTINGS) { - let annotation = NonCodeValue::new_from_meta_settings(&settings); - *node = Node::no_src(NonCodeNode { - value: annotation, - digest: None, - }); - found = true; - break; - } + for node in new_program.annotations_mut() { + if node.value.annotation_name() == Some(annotations::SETTINGS) { + let annotation = NonCodeValue::new_from_meta_settings(&settings); + *node = Node::no_src(NonCodeNode { + value: annotation, + digest: None, + }); + found = true; + break; } } @@ -1090,6 +1099,16 @@ impl NonCodeNode { _ => None, } } + + pub fn value_is_annotation(&self) -> bool { + match self.value { + NonCodeValue::InlineComment { .. } + | NonCodeValue::BlockComment { .. } + | NonCodeValue::NewLineBlockComment { .. } + | NonCodeValue::NewLine => false, + NonCodeValue::Annotation { .. } => true, + } + } } #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, ts_rs::TS, JsonSchema)] @@ -1175,7 +1194,7 @@ impl NonCodeValue { #[ts(export)] #[serde(rename_all = "camelCase")] pub struct NonCodeMeta { - pub non_code_nodes: HashMap>, + pub non_code_nodes: BTreeMap>, pub start_nodes: NodeList, #[serde(default, skip_serializing_if = "Option::is_none")] @@ -1214,7 +1233,7 @@ impl<'de> Deserialize<'de> for NonCodeMeta { .non_code_nodes .into_iter() .map(|(key, value)| Ok((key.parse().map_err(serde::de::Error::custom)?, value))) - .collect::, _>>()?; + .collect::, _>>()?; Ok(NonCodeMeta { non_code_nodes, start_nodes: helper.start_nodes, diff --git a/src/wasm-lib/kcl/src/parsing/parser.rs b/src/wasm-lib/kcl/src/parsing/parser.rs index 805f02d40..30dd7048d 100644 --- a/src/wasm-lib/kcl/src/parsing/parser.rs +++ b/src/wasm-lib/kcl/src/parsing/parser.rs @@ -1,7 +1,7 @@ // TODO optimise size of CompilationError #![allow(clippy::result_large_err)] -use std::{cell::RefCell, collections::HashMap, str::FromStr}; +use std::{cell::RefCell, collections::BTreeMap, str::FromStr}; use winnow::{ combinator::{alt, delimited, opt, peek, preceded, repeat, separated, separated_pair, terminated}, @@ -755,8 +755,8 @@ pub(crate) fn array_elem_by_elem(i: &mut TokenSlice) -> PResult, HashMap) = elements.into_iter().enumerate().fold( - (Vec::new(), HashMap::new()), + let (elements, non_code_nodes): (Vec<_>, BTreeMap) = elements.into_iter().enumerate().fold( + (Vec::new(), BTreeMap::new()), |(mut elements, mut non_code_nodes), (i, e)| { match e { NonCodeOr::NonCode(x) => { @@ -898,8 +898,8 @@ pub(crate) fn object(i: &mut TokenSlice) -> PResult> { .parse_next(i)?; // Sort the object's properties from the noncode nodes. - let (properties, non_code_nodes): (Vec<_>, HashMap) = properties.into_iter().enumerate().fold( - (Vec::new(), HashMap::new()), + let (properties, non_code_nodes): (Vec<_>, BTreeMap) = properties.into_iter().enumerate().fold( + (Vec::new(), BTreeMap::new()), |(mut properties, mut non_code_nodes), (i, e)| { match e { NonCodeOr::NonCode(x) => {