From adfc68a5c86738645e0be2ca963760efd9f6d543 Mon Sep 17 00:00:00 2001 From: peefy Date: Sat, 25 May 2024 14:40:31 +0800 Subject: [PATCH] refactor: override spec with more attribute config operation including : and += Signed-off-by: peefy --- kclvm/Cargo.lock | 1 + kclvm/ast/src/ast.rs | 2 +- kclvm/query/Cargo.toml | 1 + kclvm/query/src/lib.rs | 12 +- kclvm/query/src/override.rs | 210 +++++++++++++++++++++++++-------- kclvm/query/src/tests.rs | 56 +++++---- kclvm/query/src/util.rs | 2 +- kclvm/runner/src/runner.rs | 7 +- kclvm/spec/gpyrpc/gpyrpc.proto | 10 +- 9 files changed, 205 insertions(+), 96 deletions(-) diff --git a/kclvm/Cargo.lock b/kclvm/Cargo.lock index fd63fb31e..a2255a94a 100644 --- a/kclvm/Cargo.lock +++ b/kclvm/Cargo.lock @@ -1758,6 +1758,7 @@ dependencies = [ "anyhow", "compiler_base_macros", "compiler_base_session", + "fancy-regex", "indexmap 1.9.3", "kclvm-ast", "kclvm-ast-pretty", diff --git a/kclvm/ast/src/ast.rs b/kclvm/ast/src/ast.rs index 9a5d7fbda..69a48fe82 100644 --- a/kclvm/ast/src/ast.rs +++ b/kclvm/ast/src/ast.rs @@ -341,10 +341,10 @@ pub struct CmdArgSpec { /// KCL command line override spec, e.g. `kcl main.k -O pkgpath:path.to.field=field_value` #[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] pub struct OverrideSpec { - pub pkgpath: String, pub field_path: String, pub field_value: String, pub action: OverrideAction, + pub operation: ConfigEntryOperation, } #[derive(Serialize, Deserialize, Debug, Clone, PartialEq)] diff --git a/kclvm/query/Cargo.toml b/kclvm/query/Cargo.toml index fa28e6158..cbb0218db 100644 --- a/kclvm/query/Cargo.toml +++ b/kclvm/query/Cargo.toml @@ -18,6 +18,7 @@ kclvm-sema = {path = "../sema"} kclvm-error = {path = "../error"} serde = { version = "1.0", features = ["derive"] } serde_json = "1.0" +fancy-regex = "0.7.1" maplit = "1.0.2" [dev-dependencies] diff --git a/kclvm/query/src/lib.rs b/kclvm/query/src/lib.rs index beb0323f5..f336ba1ef 100644 --- a/kclvm/query/src/lib.rs +++ b/kclvm/query/src/lib.rs @@ -14,7 +14,6 @@ mod tests; mod util; use anyhow::{anyhow, Result}; -use kclvm_ast::ast; use kclvm_ast_pretty::print_ast_module; use kclvm_error::diagnostic::Errors; use kclvm_parser::parse_file; @@ -23,8 +22,6 @@ use kclvm_sema::pre_process::fix_config_expr_nest_attr; pub use query::{get_schema_type, GetSchemaOption}; pub use r#override::{apply_override_on_module, apply_overrides}; -use self::r#override::parse_override_spec; - /// Override and rewrite a file with override specifications. Please note that this is an external user API, /// and it can directly modify the KCL file in place. /// @@ -84,11 +81,6 @@ pub fn override_file( specs: &[String], import_paths: &[String], ) -> Result { - // Parse override spec strings. - let overrides = specs - .iter() - .map(|s| parse_override_spec(s)) - .collect::, _>>()?; // Parse file to AST module. let mut parse_result = match parse_file(file, None) { Ok(module) => module, @@ -96,8 +88,8 @@ pub fn override_file( }; let mut result = false; // Override AST module. - for o in &overrides { - if apply_override_on_module(&mut parse_result.module, o, import_paths)? { + for s in specs { + if apply_override_on_module(&mut parse_result.module, s, import_paths)? { result = true; } } diff --git a/kclvm/query/src/override.rs b/kclvm/query/src/override.rs index 6440f4f62..669cf213c 100644 --- a/kclvm/query/src/override.rs +++ b/kclvm/query/src/override.rs @@ -15,7 +15,7 @@ use kclvm_sema::pre_process::{fix_config_expr_nest_attr, transform_multi_assign} use crate::{node::AstNodeMover, path::parse_attribute_path}; -use super::util::{invalid_spec_error, split_field_path}; +use super::util::invalid_spec_error; /// Import statement column offset always start with 1. /// todo: The (1-based) column offset needs to be constrained by specifications. @@ -40,17 +40,12 @@ const IMPORT_STMT_COLUMN_OFFSET: u64 = 1; /// ``` pub fn apply_overrides( prog: &mut ast::Program, - overrides: &[ast::OverrideSpec], + overrides: &[String], import_paths: &[String], print_ast: bool, ) -> Result<()> { for o in overrides { - let pkgpath = if o.pkgpath.is_empty() { - MAIN_PKG - } else { - &o.pkgpath - }; - if let Some(modules) = prog.pkgs.get_mut(pkgpath) { + if let Some(modules) = prog.pkgs.get_mut(MAIN_PKG) { for m in modules.iter_mut() { if apply_override_on_module(m, o, import_paths)? && print_ast { let code_str = print_ast_module(m); @@ -103,11 +98,12 @@ fn build_expr_from_string(value: &str) -> Option> { /// ``` pub fn apply_override_on_module( m: &mut ast::Module, - o: &ast::OverrideSpec, + o: &str, import_paths: &[String], ) -> Result { // Apply import paths on AST module. apply_import_paths_on_module(m, import_paths)?; + let o = parse_override_spec(o)?; let ss = parse_attribute_path(&o.field_path)?; let default = String::default(); let target_id = ss.get(0).unwrap_or(&default); @@ -137,7 +133,8 @@ pub fn apply_override_on_module( override_value: build_expr_from_string(value), override_target_count: 0, has_override: false, - action: o.action.clone(), + action: o.action, + operation: o.operation, }; transformer.walk_module(m); Ok(transformer.has_override) @@ -152,36 +149,97 @@ pub fn apply_override_on_module( /// action: ast::OverrideAction::CreateOrUpdate, /// } pub fn parse_override_spec(spec: &str) -> Result { - if spec.contains('=') { + if let Some((path, value, operation)) = split_override_spec_op(spec) { // Create or update the override value. - let split_values = spec.splitn(2, '=').map(|s| s.trim()).collect::>(); - let path = split_values - .first() - .ok_or_else(|| invalid_spec_error(spec))?; - let field_value = split_values - .get(1) - .ok_or_else(|| invalid_spec_error(spec))?; - let (pkgpath, field_path) = split_field_path(path)?; - Ok(ast::OverrideSpec { - pkgpath, - field_path, - field_value: field_value.to_string(), - action: ast::OverrideAction::CreateOrUpdate, - }) + let field_path = path.trim().to_string(); + let field_value = value.trim().to_string(); + if field_path.is_empty() || field_value.is_empty() { + Err(invalid_spec_error(spec)) + } else { + Ok(ast::OverrideSpec { + field_path, + field_value, + action: ast::OverrideAction::CreateOrUpdate, + operation, + }) + } } else if let Some(stripped_spec) = spec.strip_suffix('-') { // Delete the override value. - let (pkgpath, field_path) = split_field_path(stripped_spec)?; - Ok(ast::OverrideSpec { - pkgpath, - field_path, - field_value: "".to_string(), - action: ast::OverrideAction::Delete, - }) + let field_path = stripped_spec.trim().to_string(); + if field_path.is_empty() { + Err(invalid_spec_error(spec)) + } else { + Ok(ast::OverrideSpec { + field_path: stripped_spec.trim().to_string(), + field_value: "".to_string(), + action: ast::OverrideAction::Delete, + operation: ast::ConfigEntryOperation::Override, + }) + } } else { Err(invalid_spec_error(spec)) } } +/// split_override_spec_op split the override_spec and do not split the override_op in list +/// expr, dict expr and string e.g., "a.b=1" -> (a.b, 1, =), "a["a=1"]=1" -> (a["a=1"], =, 1) +pub fn split_override_spec_op(spec: &str) -> Option<(String, String, ast::ConfigEntryOperation)> { + let mut i = 0; + let mut stack = String::new(); + while i < spec.chars().count() { + let (c_idx, c) = spec.char_indices().nth(i).unwrap(); + if c == '=' && stack.is_empty() { + return Some(( + spec[..c_idx].to_string(), + spec[c_idx + 1..].to_string(), + ast::ConfigEntryOperation::Override, + )); + } else if c == ':' && stack.is_empty() { + return Some(( + spec[..c_idx].to_string(), + spec[c_idx + 1..].to_string(), + ast::ConfigEntryOperation::Union, + )); + } else if c == '+' && stack.is_empty() { + if let Some((c_next_idx, c_next)) = spec.char_indices().nth(i + 1) { + if c_next == '=' { + return Some(( + spec[..c_idx].to_string(), + spec[c_next_idx + 1..].to_string(), + ast::ConfigEntryOperation::Insert, + )); + } + } + } + // List/Dict type + else if c == '[' || c == '{' { + stack.push(c); + } + // List/Dict type + else if c == ']' || c == '}' { + stack.pop(); + } + // String literal type + else if c == '\"' { + let t: String = spec.chars().skip(i).collect(); + let re = fancy_regex::Regex::new(r#""(?!"").*?(? Result<()> { if import_paths.is_empty() { @@ -248,6 +306,7 @@ struct OverrideTransformer { pub override_target_count: usize, pub has_override: bool, pub action: ast::OverrideAction, + pub operation: ast::ConfigEntryOperation, } impl<'ctx> MutSelfMutWalker<'ctx> for OverrideTransformer { @@ -365,24 +424,81 @@ impl<'ctx> MutSelfMutWalker<'ctx> for OverrideTransformer { }, )))), value: self.clone_override_value(), - operation: ast::ConfigEntryOperation::Override, + operation: self.operation.clone(), insert_index: -1, }))], }))) }; + match &self.operation { + ast::ConfigEntryOperation::Override => { + let assign = ast::AssignStmt { + targets: vec![Box::new(ast::Node::dummy_node(ast::Identifier { + names: vec![ast::Node::dummy_node(self.target_id.clone())], + ctx: ast::ExprContext::Store, + pkgpath: "".to_string(), + }))], + ty: None, + value, + }; + module + .body + .push(Box::new(ast::Node::dummy_node(ast::Stmt::Assign(assign)))); + } + ast::ConfigEntryOperation::Union => { + let schema_expr: Result, _> = + value.as_ref().clone().try_into(); + match schema_expr { + Ok(schema_expr) => { + let stmt = ast::UnificationStmt { + target: Box::new(ast::Node::dummy_node(ast::Identifier { + names: vec![ast::Node::dummy_node( + self.target_id.clone(), + )], + ctx: ast::ExprContext::Store, + pkgpath: "".to_string(), + })), + value: Box::new(schema_expr), + }; + module.body.push(Box::new(ast::Node::dummy_node( + ast::Stmt::Unification(stmt), + ))); + } + Err(_) => { + let stmt = ast::AssignStmt { + targets: vec![Box::new(ast::Node::dummy_node( + ast::Identifier { + names: vec![ast::Node::dummy_node( + self.target_id.clone(), + )], + ctx: ast::ExprContext::Store, + pkgpath: "".to_string(), + }, + ))], + ty: None, + value, + }; + module.body.push(Box::new(ast::Node::dummy_node( + ast::Stmt::Assign(stmt), + ))); + } + } + } + ast::ConfigEntryOperation::Insert => { + let stmt = ast::AugAssignStmt { + target: Box::new(ast::Node::dummy_node(ast::Identifier { + names: vec![ast::Node::dummy_node(self.target_id.clone())], + ctx: ast::ExprContext::Store, + pkgpath: "".to_string(), + })), + op: ast::AugOp::Add, + value, + }; + module + .body + .push(Box::new(ast::Node::dummy_node(ast::Stmt::AugAssign(stmt)))); + } + } - let assign = ast::AssignStmt { - targets: vec![Box::new(ast::Node::dummy_node(ast::Identifier { - names: vec![ast::Node::dummy_node(self.target_id.clone())], - ctx: ast::ExprContext::Store, - pkgpath: "".to_string(), - }))], - ty: None, - value, - }; - module - .body - .push(Box::new(ast::Node::dummy_node(ast::Stmt::Assign(assign)))); self.has_override = true; } ast::OverrideAction::Delete => { @@ -451,7 +567,7 @@ impl<'ctx> MutSelfMutWalker<'ctx> for OverrideTransformer { self.override_key.clone(), )))), value: self.clone_override_value(), - operation: ast::ConfigEntryOperation::Override, + operation: self.operation.clone(), insert_index: -1, }))); self.has_override = true; @@ -585,7 +701,7 @@ impl OverrideTransformer { .push(Box::new(ast::Node::dummy_node(ast::ConfigEntry { key: Some(Box::new(ast::Node::dummy_node(ast::Expr::Identifier(key)))), value: self.clone_override_value(), - operation: ast::ConfigEntryOperation::Override, + operation: self.operation.clone(), insert_index: -1, }))); changed = true; diff --git a/kclvm/query/src/tests.rs b/kclvm/query/src/tests.rs index a62d2b909..0aaf84c29 100644 --- a/kclvm/query/src/tests.rs +++ b/kclvm/query/src/tests.rs @@ -5,8 +5,9 @@ use std::{ }; use super::{r#override::apply_override_on_module, *}; -use crate::{path::parse_attribute_path, selector::list_variables}; -use kclvm_ast::ast; +use crate::{ + path::parse_attribute_path, r#override::parse_override_spec, selector::list_variables, +}; use kclvm_error::{DiagnosticId, ErrorKind, Level}; use kclvm_parser::parse_file_force_errors; use pretty_assertions::assert_eq; @@ -26,20 +27,20 @@ fn get_test_dir(sub: String) -> PathBuf { fn test_override_file_simple() { let specs = vec![ "config.image=image/image".to_string(), - ":config.image=\"image/image:v1\"".to_string(), - ":config.data={id=1,value=\"override_value\"}".to_string(), - ":dict_config={\"image\": \"image/image:v2\" \"data\":{\"id\":2 \"value2\": \"override_value2\"}}".to_string(), - ":envs=[{key=\"key1\" value=\"value1\"} {key=\"key2\" value=\"value2\"}]".to_string(), - ":isfilter=False".to_string(), - ":count=2".to_string(), - ":msg=\"Hi World\"".to_string(), - ":delete-".to_string(), - ":dict_delete.image-".to_string(), - ":dict_delete_whole-".to_string(), - ":insert_config.key=1".to_string(), - ":uni_config.labels.key1=1".to_string(), - ":config_unification=Config {\"image\": \"image/image:v4\"}".to_string(), - ":config_unification_delete-".to_string() + "config.image=\"image/image:v1\"".to_string(), + "config.data={id=1,value=\"override_value\"}".to_string(), + "dict_config={\"image\": \"image/image:v2\" \"data\":{\"id\":2 \"value2\": \"override_value2\"}}".to_string(), + "envs=[{key=\"key1\" value=\"value1\"} {key=\"key2\" value=\"value2\"}]".to_string(), + "isfilter=False".to_string(), + "count=2".to_string(), + "msg=\"Hi World\"".to_string(), + "delete-".to_string(), + "dict_delete.image-".to_string(), + "dict_delete_whole-".to_string(), + "insert_config.key=1".to_string(), + "uni_config.labels.key1=1".to_string(), + "config_unification=Config {\"image\": \"image/image:v4\"}".to_string(), + "config_unification_delete-".to_string() ]; let simple_path = get_test_dir("simple.k".to_string()); @@ -110,12 +111,14 @@ fn test_override_file_config() { "appConfigurationUnification.labels.key.key=\"override_value\"".to_string(), "appConfigurationUnification.overQuota=False".to_string(), "appConfigurationUnification.resource.cpu-".to_string(), + "config.x:1".to_string(), + "config.y=1".to_string(), + "config.z+=[1,2,3]".to_string(), + "var1:1".to_string(), + "var2=1".to_string(), + "var3+=[1,2,3]".to_string(), + "var4:AppConfiguration {image:'image'}".to_string(), ]; - let overrides = specs - .iter() - .map(|s| parse_override_spec(s)) - .filter_map(Result::ok) - .collect::>(); let import_paths = vec![]; let mut cargo_file_path = PathBuf::from(CARGO_FILE_PATH); @@ -123,8 +126,8 @@ fn test_override_file_config() { let abs_path = cargo_file_path.to_str().unwrap(); let mut module = parse_file_force_errors(abs_path, None).unwrap(); - for o in &overrides { - apply_override_on_module(&mut module, o, &import_paths).unwrap(); + for s in &specs { + apply_override_on_module(&mut module, s, &import_paths).unwrap(); } let expected_code = print_ast_module(&module); assert_eq!( @@ -178,6 +181,11 @@ appConfigurationUnification: AppConfiguration { mainContainer: Main {name: "override_name"} overQuota: False } +config = {x: 1, y = 1, z += [1, 2, 3]} +var1 = 1 +var2 = 1 +var3 += [1, 2, 3] +var4: AppConfiguration {image: 'image'} "# ); } @@ -692,5 +700,5 @@ fn test_override_file_with_invalid_spec() { let result = override_file(&file, &specs, &import_paths); assert!(result.is_err()); let err = result.err().unwrap(); - assert_eq!(err.to_string(), "Invalid spec format '....', expected := or :-"); + assert_eq!(err.to_string(), "Invalid spec format '....', expected =filed_value>, :filed_value>, +=filed_value> or -"); } diff --git a/kclvm/query/src/util.rs b/kclvm/query/src/util.rs index f4d811fd9..9c3ffffb4 100644 --- a/kclvm/query/src/util.rs +++ b/kclvm/query/src/util.rs @@ -25,7 +25,7 @@ pub(crate) fn split_field_path(path: &str) -> Result<(String, String)> { /// Get the invalid spec error message. #[inline] pub(crate) fn invalid_spec_error(spec: &str) -> anyhow::Error { - anyhow!("Invalid spec format '{}', expected := or :-", spec) + anyhow!("Invalid spec format '{}', expected =filed_value>, :filed_value>, +=filed_value> or -", spec) } /// Get the invalid symbol selector spec error message. diff --git a/kclvm/runner/src/runner.rs b/kclvm/runner/src/runner.rs index 16c7979c7..6c2614750 100644 --- a/kclvm/runner/src/runner.rs +++ b/kclvm/runner/src/runner.rs @@ -9,7 +9,6 @@ use kclvm_config::{ settings::{SettingsFile, SettingsPathBuf}, }; use kclvm_error::{Diagnostic, Handler}; -use kclvm_query::r#override::parse_override_spec; #[cfg(not(target_arch = "wasm32"))] use kclvm_runtime::kclvm_plugin_init; #[cfg(feature = "llvm")] @@ -42,7 +41,7 @@ pub struct ExecProgramArgs { /// -D key=value pub args: Vec, /// -O override_spec - pub overrides: Vec, + pub overrides: Vec, /// -S path_selector pub path_selector: Vec, pub disable_yaml_result: bool, @@ -188,8 +187,8 @@ impl TryFrom for ExecProgramArgs { args.fast_eval = cli_configs.fast_eval.unwrap_or_default(); args.include_schema_type_path = cli_configs.include_schema_type_path.unwrap_or_default(); - for override_str in &cli_configs.overrides.unwrap_or_default() { - args.overrides.push(parse_override_spec(override_str)?); + for override_str in cli_configs.overrides.unwrap_or_default() { + args.overrides.push(override_str); } args.path_selector = cli_configs.path_selector.unwrap_or_default(); args.set_external_pkg_from_package_maps( diff --git a/kclvm/spec/gpyrpc/gpyrpc.proto b/kclvm/spec/gpyrpc/gpyrpc.proto index c26671724..39f589d19 100644 --- a/kclvm/spec/gpyrpc/gpyrpc.proto +++ b/kclvm/spec/gpyrpc/gpyrpc.proto @@ -18,14 +18,6 @@ message CmdArgSpec { string value = 2; } -// kcl main.k -O pkgpath:path.to.field=field_value -message CmdOverrideSpec { - string pkgpath = 1; - string field_path = 2; - string field_value = 3; - string action = 4; -} - // ---------------------------------------------------------------------------- // Error types // ---------------------------------------------------------------------------- @@ -190,7 +182,7 @@ message ExecProgram_Args { repeated string k_code_list = 3; repeated CmdArgSpec args = 4; - repeated CmdOverrideSpec overrides = 5; + repeated string overrides = 5; bool disable_yaml_result = 6;