-
Notifications
You must be signed in to change notification settings - Fork 404
bridge(feat): track created handles and enable batch cleanup #3195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: canary
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,8 +10,8 @@ use indexmap::IndexMap; | |
|
|
||
| use crate::{ | ||
| baml::cffi::{ | ||
| InboundClassValue, InboundEnumValue, InboundListValue, InboundMapEntry, InboundMapValue, | ||
| InboundValue, inbound_value::Value as InboundValueVariant, | ||
| BamlHandleType, InboundClassValue, InboundEnumValue, InboundListValue, InboundMapEntry, | ||
| InboundMapValue, InboundValue, inbound_value::Value as InboundValueVariant, | ||
| }, | ||
| error::CtypesError, | ||
| handle_table::HandleTable, | ||
|
|
@@ -39,12 +39,27 @@ pub fn inbound_to_external( | |
| let value = handle_table | ||
| .resolve(handle.key) | ||
| .ok_or(CtypesError::InvalidHandleKey(handle.key))?; | ||
| let actual = value.handle_type() as i32; | ||
| validate_handle_type(handle.key, handle.handle_type, actual)?; | ||
| Ok(BexExternalValue::from((*value).clone())) | ||
| } | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| fn validate_handle_type(key: u64, expected: i32, actual: i32) -> Result<(), CtypesError> { | ||
| match BamlHandleType::try_from(expected) { | ||
| Ok(BamlHandleType::HandleUnspecified | BamlHandleType::HandleUnknown) => Ok(()), | ||
| Ok(_) if expected == actual => Ok(()), | ||
| Err(_) => Ok(()), | ||
| Ok(_) => Err(CtypesError::InvalidHandleType { | ||
| key, | ||
| expected, | ||
| actual, | ||
| }), | ||
| } | ||
| } | ||
|
|
||
| fn convert_list( | ||
| list: InboundListValue, | ||
| handle_table: &HandleTable, | ||
|
|
@@ -136,3 +151,59 @@ pub fn kwargs_to_bex_values( | |
| } | ||
| Ok(result) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use crate::{ | ||
| baml::cffi::{BamlHandle, inbound_value::Value as InboundValueVariant}, | ||
| handle_table::HandleTableValue, | ||
| }; | ||
|
|
||
| fn inbound_handle(key: u64, handle_type: BamlHandleType) -> InboundValue { | ||
| InboundValue { | ||
| value: Some(InboundValueVariant::Handle(BamlHandle { | ||
| key, | ||
| handle_type: handle_type as i32, | ||
| })), | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn handle_type_matches() { | ||
| let table = HandleTable::new(); | ||
| let key = table.insert(HandleTableValue::FunctionRef { global_index: 1 }); | ||
| let inbound = inbound_handle(key, BamlHandleType::FunctionRef); | ||
| let out = inbound_to_external(inbound, &table).unwrap(); | ||
| assert!(matches!( | ||
| out, | ||
| BexExternalValue::FunctionRef { global_index: 1 } | ||
| )); | ||
| } | ||
|
|
||
| #[test] | ||
| fn handle_type_unknown_is_allowed() { | ||
| let table = HandleTable::new(); | ||
| let key = table.insert(HandleTableValue::FunctionRef { global_index: 2 }); | ||
| let inbound = inbound_handle(key, BamlHandleType::HandleUnknown); | ||
| let out = inbound_to_external(inbound, &table).unwrap(); | ||
| assert!(matches!( | ||
| out, | ||
| BexExternalValue::FunctionRef { global_index: 2 } | ||
| )); | ||
| } | ||
|
|
||
| #[test] | ||
| fn handle_type_mismatch_errors() { | ||
| let table = HandleTable::new(); | ||
| let key = table.insert(HandleTableValue::FunctionRef { global_index: 3 }); | ||
| let inbound = inbound_handle(key, BamlHandleType::AdtMediaImage); | ||
| let err = inbound_to_external(inbound, &table).unwrap_err(); | ||
| match err { | ||
| CtypesError::InvalidHandleType { key: err_key, .. } => { | ||
| assert_eq!(err_key, key); | ||
| } | ||
| _ => panic!("expected InvalidHandleType"), | ||
| } | ||
| } | ||
|
Comment on lines
+196
to
+208
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Add a unit test for unrecognized numeric handle types. The permissive 💡 Suggested test addition #[test]
fn handle_type_unknown_is_allowed() {
@@
}
+#[test]
+fn handle_type_unrecognized_numeric_is_allowed() {
+ let table = HandleTable::new();
+ let key = table.insert(HandleTableValue::FunctionRef { global_index: 4 });
+ let inbound = InboundValue {
+ value: Some(InboundValueVariant::Handle(BamlHandle {
+ key,
+ handle_type: i32::MAX,
+ })),
+ };
+ let out = inbound_to_external(inbound, &table).unwrap();
+ assert!(matches!(
+ out,
+ BexExternalValue::FunctionRef { global_index: 4 }
+ ));
+}As per coding guidelines: "Prefer writing Rust unit tests over integration tests where possible." |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Add a dedicated unit test for
release_manybehavior.The method is straightforward, but this is new cleanup-critical API surface and should have explicit coverage (present keys removed, missing keys ignored, duplicates tolerated).
🧪 Suggested unit test
#[test] fn double_release_returns_false() { @@ } +#[test] +fn release_many_releases_present_and_ignores_missing() { + let table = HandleTable::new(); + let key1 = table.insert(make_function_ref()); + let key2 = table.insert(make_function_ref()); + + table.release_many([key1, 9_999_999, key2, key1]); + + assert!(table.resolve(key1).is_none()); + assert!(table.resolve(key2).is_none()); +}As per coding guidelines: "Prefer writing Rust unit tests over integration tests where possible."