From bb98f5e0d5e83a1bff66af74f1cf137628ab43d3 Mon Sep 17 00:00:00 2001 From: never Date: Tue, 8 Nov 2022 11:00:11 +0800 Subject: [PATCH 1/2] fix : panic caused by duplicated keys in schema --- kclvm/runtime/src/crypto/crypto.rs | 2 +- kclvm/runtime/src/value/api.rs | 10 ++-- kclvm/runtime/src/value/val_bin.rs | 2 +- kclvm/runtime/src/value/val_bin_aug.rs | 2 +- kclvm/runtime/src/value/val_dict.rs | 2 +- kclvm/runtime/src/value/val_union.rs | 47 ++++++++++++------- .../duplicated_key/duplicated_key1/main.k | 12 +++++ .../duplicated_key1/stdout.golden | 2 + .../duplicated_key/duplicated_key2/main.k | 12 +++++ .../duplicated_key2/stdout.golden | 3 ++ .../duplicated_key/duplicated_key3/main.k | 12 +++++ .../duplicated_key3/stdout.golden | 5 ++ 12 files changed, 85 insertions(+), 26 deletions(-) create mode 100644 test/grammar/schema/duplicated_key/duplicated_key1/main.k create mode 100644 test/grammar/schema/duplicated_key/duplicated_key1/stdout.golden create mode 100644 test/grammar/schema/duplicated_key/duplicated_key2/main.k create mode 100644 test/grammar/schema/duplicated_key/duplicated_key2/stdout.golden create mode 100644 test/grammar/schema/duplicated_key/duplicated_key3/main.k create mode 100644 test/grammar/schema/duplicated_key/duplicated_key3/stdout.golden diff --git a/kclvm/runtime/src/crypto/crypto.rs b/kclvm/runtime/src/crypto/crypto.rs index 9c6341555..4334f6e15 100644 --- a/kclvm/runtime/src/crypto/crypto.rs +++ b/kclvm/runtime/src/crypto/crypto.rs @@ -25,7 +25,7 @@ pub extern "C" fn kclvm_crypto_md5( let args = ptr_as_ref(args); if let Some(s) = args.arg_i_str(0, None) { - let hex = format!("{:x}", md5::compute(&s)); + let hex = format!("{:x}", md5::compute(s)); return ValueRef::str(hex.as_ref()).into_raw(); } panic!("md5() missing 1 required positional argument: 'value'"); diff --git a/kclvm/runtime/src/value/api.rs b/kclvm/runtime/src/value/api.rs index a167bf537..3784f7be3 100644 --- a/kclvm/runtime/src/value/api.rs +++ b/kclvm/runtime/src/value/api.rs @@ -1745,20 +1745,20 @@ pub extern "C" fn kclvm_value_union( let mut entry = b.dict_get_entry(k).unwrap().deep_copy(); entry.dict_update_key_value(k, v); result = a - .union(&entry, true, false, false, false) + .union_entry(&entry, true, false, false, false) .clone() .into_raw(); } else { let entry = b.dict_get_entry(k).unwrap(); result = a - .union(&entry, true, false, false, false) + .union_entry(&entry, true, false, false, false) .clone() .into_raw(); } } result } else { - a.union(b, true, false, false, false).into_raw() + a.union_entry(b, true, false, false, false).into_raw() } } @@ -2075,7 +2075,7 @@ pub extern "C" fn kclvm_schema_value_check( if should_add_attr && is_not_in_schema { let value = index_sign_value .deep_copy() - .union(value, true, false, false, true); + .union_entry(value, true, false, false, true); let op = config .ops .get(key) @@ -2277,7 +2277,7 @@ pub extern "C" fn kclvm_schema_value_new( let config = ptr_as_ref(config); let result = schema_value_or_func .deep_copy() - .union(config, true, false, true, true); + .union_entry(config, true, false, true, true); result.into_raw() } } diff --git a/kclvm/runtime/src/value/val_bin.rs b/kclvm/runtime/src/value/val_bin.rs index cfddb0f05..08ced368b 100644 --- a/kclvm/runtime/src/value/val_bin.rs +++ b/kclvm/runtime/src/value/val_bin.rs @@ -296,7 +296,7 @@ impl ValueRef { (Value::int_value(a), Value::int_value(b)) => return Self::int(*a | *b), _ => {} } - self.deep_copy().union(x, true, false, true, true) + self.deep_copy().union_entry(x, true, false, true, true) } pub fn bin_subscr(&self, x: &Self) -> Self { diff --git a/kclvm/runtime/src/value/val_bin_aug.rs b/kclvm/runtime/src/value/val_bin_aug.rs index 1cc708583..3a70a60f1 100644 --- a/kclvm/runtime/src/value/val_bin_aug.rs +++ b/kclvm/runtime/src/value/val_bin_aug.rs @@ -390,7 +390,7 @@ impl ValueRef { }; if !valid { if self.is_list_or_config() || x.is_list_or_config() { - self.union(x, true, false, true, true); + self.union_entry(x, true, false, true, true); } else { panic_unsupported_bin_op!("|", self.type_str(), x.type_str()); } diff --git a/kclvm/runtime/src/value/val_dict.rs b/kclvm/runtime/src/value/val_dict.rs index cc8a1293b..ff7b2a6fc 100644 --- a/kclvm/runtime/src/value/val_dict.rs +++ b/kclvm/runtime/src/value/val_dict.rs @@ -311,7 +311,7 @@ impl ValueRef { dict.values.insert(key.to_string(), v.clone()); dict.ops.insert(key.to_string(), op); dict.insert_indexs.insert(key.to_string(), insert_index); - self.union( + self.union_entry( &ValueRef::from(Value::dict_value(Box::new(dict))), true, false, diff --git a/kclvm/runtime/src/value/val_union.rs b/kclvm/runtime/src/value/val_union.rs index 5e2a2299a..9f98ceffd 100644 --- a/kclvm/runtime/src/value/val_union.rs +++ b/kclvm/runtime/src/value/val_union.rs @@ -42,14 +42,13 @@ impl ValueRef { { panic!("conflicting values on the attribute '{}' between {:?} and {:?}", k, self, x); } - let value = obj.values.get_mut(k).unwrap().union( + obj.values.get_mut(k).unwrap().union( v, false, should_list_override, should_idempotent_check, should_config_resolve, ); - obj.values.insert(k.clone(), value); } ConfigEntryOperationKind::Override => { if index < 0 { @@ -67,20 +66,18 @@ impl ValueRef { } } ConfigEntryOperationKind::Insert => { - let value = v.deep_copy(); let origin_value = obj.values.get_mut(k).unwrap(); if origin_value.is_none_or_undefined() { let list = ValueRef::list(None); obj.values.insert(k.to_string(), list); } let origin_value = obj.values.get_mut(k).unwrap(); - match ( - &mut *origin_value.rc.borrow_mut(), - &mut *value.rc.borrow_mut(), - ) { + match (&mut *origin_value.rc.borrow_mut(), &*v.rc.borrow()) { (Value::list_value(origin_value), Value::list_value(value)) => { if index == -1 { - origin_value.values.append(&mut value.clone().values); + for elem in value.values.iter() { + origin_value.values.push(elem.clone()); + } } else if index >= 0 { let mut insert_index = index; for v in &value.values { @@ -183,8 +180,7 @@ impl ValueRef { } self.clone() } - - pub fn union( + fn union( &mut self, x: &Self, or_mode: bool, @@ -207,13 +203,12 @@ impl ValueRef { should_config_resolve, ); } else if or_mode { - match (&mut *self.rc.borrow_mut(), &*x.rc.borrow()) { - (Value::int_value(a), Value::int_value(b)) => { - *a |= *b; - return self.clone(); - } - _ => {} - } + if let (Value::int_value(a), Value::int_value(b)) = + (&mut *self.rc.borrow_mut(), &*x.rc.borrow()) + { + *a |= *b; + return self.clone(); + }; panic!( "unsupported operand type(s) for |: '{:?}' and '{:?}'", self.type_str(), @@ -224,6 +219,24 @@ impl ValueRef { } self.clone() } + + // Deep copy the right value of the union call to avoid the left value part refering to the right value + pub fn union_entry( + &mut self, + x: &Self, + or_mode: bool, + should_list_override: bool, + should_idempotent_check: bool, + should_config_resolve: bool, + ) -> Self { + self.union( + &x.deep_copy(), + or_mode, + should_list_override, + should_idempotent_check, + should_config_resolve, + ) + } } #[cfg(test)] diff --git a/test/grammar/schema/duplicated_key/duplicated_key1/main.k b/test/grammar/schema/duplicated_key/duplicated_key1/main.k new file mode 100644 index 000000000..21297884d --- /dev/null +++ b/test/grammar/schema/duplicated_key/duplicated_key1/main.k @@ -0,0 +1,12 @@ +schema Config: + cpu?: int + +schema A: + k?: Config + +schema C(A): + k?: Config + +a = C { + k: Config {} +} diff --git a/test/grammar/schema/duplicated_key/duplicated_key1/stdout.golden b/test/grammar/schema/duplicated_key/duplicated_key1/stdout.golden new file mode 100644 index 000000000..85c9aa5d6 --- /dev/null +++ b/test/grammar/schema/duplicated_key/duplicated_key1/stdout.golden @@ -0,0 +1,2 @@ +a: + k: {} \ No newline at end of file diff --git a/test/grammar/schema/duplicated_key/duplicated_key2/main.k b/test/grammar/schema/duplicated_key/duplicated_key2/main.k new file mode 100644 index 000000000..7e2160d8f --- /dev/null +++ b/test/grammar/schema/duplicated_key/duplicated_key2/main.k @@ -0,0 +1,12 @@ +schema Config: + cpu?: int + +schema A: + k?: Config + +schema C(A): + k: Config + +a = C { + k: Config {cpu: 1} +} \ No newline at end of file diff --git a/test/grammar/schema/duplicated_key/duplicated_key2/stdout.golden b/test/grammar/schema/duplicated_key/duplicated_key2/stdout.golden new file mode 100644 index 000000000..5c51d173b --- /dev/null +++ b/test/grammar/schema/duplicated_key/duplicated_key2/stdout.golden @@ -0,0 +1,3 @@ +a: + k: + cpu: 1 \ No newline at end of file diff --git a/test/grammar/schema/duplicated_key/duplicated_key3/main.k b/test/grammar/schema/duplicated_key/duplicated_key3/main.k new file mode 100644 index 000000000..320461cd3 --- /dev/null +++ b/test/grammar/schema/duplicated_key/duplicated_key3/main.k @@ -0,0 +1,12 @@ +schema Container: + cpu?: int + +schema App: + container: Container + +container = Container {cpu: 1} + +app = App { + container: container + container: container +} \ No newline at end of file diff --git a/test/grammar/schema/duplicated_key/duplicated_key3/stdout.golden b/test/grammar/schema/duplicated_key/duplicated_key3/stdout.golden new file mode 100644 index 000000000..3a43c7137 --- /dev/null +++ b/test/grammar/schema/duplicated_key/duplicated_key3/stdout.golden @@ -0,0 +1,5 @@ +container: + cpu: 1 +app: + container: + cpu: 1 \ No newline at end of file From ad5f074021fc07f4f8840df14eb598f1c98c9060 Mon Sep 17 00:00:00 2001 From: never Date: Tue, 8 Nov 2022 11:32:38 +0800 Subject: [PATCH 2/2] refactor : use cargo clippy to remove warning --- kclvm/runtime/src/context/mod.rs | 4 ++-- kclvm/runtime/src/units/units.rs | 2 +- kclvm/runtime/src/value/val_bin.rs | 7 +++---- kclvm/runtime/src/value/val_bin_aug.rs | 8 ++++---- kclvm/runtime/src/value/val_type.rs | 9 +++------ 5 files changed, 13 insertions(+), 17 deletions(-) diff --git a/kclvm/runtime/src/context/mod.rs b/kclvm/runtime/src/context/mod.rs index 5e84677c8..d449028a6 100644 --- a/kclvm/runtime/src/context/mod.rs +++ b/kclvm/runtime/src/context/mod.rs @@ -181,7 +181,7 @@ impl crate::Context { // check dup for i in 0..self.option_helps.len() { if self.option_helps[i].name == name { - if typ.is_empty() && !required && default_value == None && help.is_empty() { + if typ.is_empty() && !required && default_value.is_none() && help.is_empty() { return; } @@ -192,7 +192,7 @@ impl crate::Context { if !self.option_helps[i].required { self.option_helps[i].required = required; } - if self.option_helps[i].default_value == None { + if self.option_helps[i].default_value.is_none() { self.option_helps[i].default_value = default_value; } if self.option_helps[i].help.is_empty() { diff --git a/kclvm/runtime/src/units/units.rs b/kclvm/runtime/src/units/units.rs index 856b97a5e..12d3e7368 100644 --- a/kclvm/runtime/src/units/units.rs +++ b/kclvm/runtime/src/units/units.rs @@ -362,7 +362,7 @@ pub fn to_quantity(quantity: &str) -> i64 { 1000 }; let exponent = EXPONENTS.get(&suffix[0..1]).unwrap(); - number * (base.pow(*exponent as u32)) as i64 + number * (base.pow(*exponent as u32)) } /// Calculate number based on value and binary suffix. diff --git a/kclvm/runtime/src/value/val_bin.rs b/kclvm/runtime/src/value/val_bin.rs index 08ced368b..e61c639d8 100644 --- a/kclvm/runtime/src/value/val_bin.rs +++ b/kclvm/runtime/src/value/val_bin.rs @@ -292,10 +292,9 @@ impl ValueRef { } pub fn bin_bit_or(&self, x: &Self) -> Self { - match (&*self.rc.borrow(), &*x.rc.borrow()) { - (Value::int_value(a), Value::int_value(b)) => return Self::int(*a | *b), - _ => {} - } + if let (Value::int_value(a), Value::int_value(b)) = (&*self.rc.borrow(), &*x.rc.borrow()) { + return Self::int(*a | *b); + }; self.deep_copy().union_entry(x, true, false, true, true) } diff --git a/kclvm/runtime/src/value/val_bin_aug.rs b/kclvm/runtime/src/value/val_bin_aug.rs index 3a70a60f1..520c99ae5 100644 --- a/kclvm/runtime/src/value/val_bin_aug.rs +++ b/kclvm/runtime/src/value/val_bin_aug.rs @@ -247,7 +247,7 @@ impl ValueRef { if strict_range_check_32 && is_f32_overflow_pow(*a, *b) { panic_f32_overflow!(a.powf(*b)); } - *a = a.powf(*b as f64); + *a = a.powf(*b); true } (Value::int_value(a), Value::float_value(b)) => { @@ -355,7 +355,7 @@ impl ValueRef { pub fn bin_aug_bit_and(&mut self, x: &Self) -> &mut Self { let valid = match (&mut *self.rc.borrow_mut(), &*x.rc.borrow()) { (Value::int_value(a), Value::int_value(b)) => { - *a &= *b as i64; + *a &= *b; true } _ => false, @@ -369,7 +369,7 @@ impl ValueRef { pub fn bin_aug_bit_xor(&mut self, x: &Self) -> &mut Self { let valid = match (&mut *self.rc.borrow_mut(), &*x.rc.borrow()) { (Value::int_value(a), Value::int_value(b)) => { - *a ^= *b as i64; + *a ^= *b; true } _ => false, @@ -383,7 +383,7 @@ impl ValueRef { pub fn bin_aug_bit_or(&mut self, x: &Self) -> &mut Self { let valid = match (&mut *self.rc.borrow_mut(), &*x.rc.borrow()) { (Value::int_value(a), Value::int_value(b)) => { - *a |= *b as i64; + *a |= *b; true } _ => false, diff --git a/kclvm/runtime/src/value/val_type.rs b/kclvm/runtime/src/value/val_type.rs index 132b16a81..3a9af729a 100644 --- a/kclvm/runtime/src/value/val_type.rs +++ b/kclvm/runtime/src/value/val_type.rs @@ -264,12 +264,9 @@ pub fn convert_collection_value(value: &ValueRef, tpe: &str) -> ValueRef { } None => { for (_, mapping) in &ctx.import_names { - match mapping.get(pkgname) { - Some(pkgpath) => { - schema_type_name = format!("{}.{}", pkgpath, name); - break; - } - None => {} + if let Some(pkgpath) = mapping.get(pkgname) { + schema_type_name = format!("{}.{}", pkgpath, name); + break; } } }