Refactor Scope::copy_with API to return an associated type. This is required so that unowned C++ &Scope references passed from GN macro invocations can implement types::Scope and be returned by EvalContext::require_macro. Bug: 528225104 Change-Id: I6ab4e260229628f2c542742c6da140fb6a6a6964 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/24803 Reviewed-by: Takuto Ikuta <tikuta@google.com> Commit-Queue: Matt Stark <msta@google.com>
diff --git a/src/gn/starlark/crates/ffi/src/eval_context.rs b/src/gn/starlark/crates/ffi/src/eval_context.rs index bc8affa..1bd38aa 100644 --- a/src/gn/starlark/crates/ffi/src/eval_context.rs +++ b/src/gn/starlark/crates/ffi/src/eval_context.rs
@@ -36,7 +36,7 @@ } impl types::EvalContext for EvalContext { - type Scope = crate::scope::OwnedScope; + type Scope = crate::Scope; type Session = crate::session::Session; fn current_package(&self) -> &types::PackageRef {
diff --git a/src/gn/starlark/crates/ffi/src/lib.rs b/src/gn/starlark/crates/ffi/src/lib.rs index e2a9823..2d39a16 100644 --- a/src/gn/starlark/crates/ffi/src/lib.rs +++ b/src/gn/starlark/crates/ffi/src/lib.rs
@@ -38,7 +38,6 @@ }; pub use mutability::Immutable; pub use opaque::{NonOpaque, OpaqueSized}; -pub use scope::OwnedScope; pub use session::Session; pub use slice::{OwnedSlice, Slice}; pub use test_with_scope::TestWithScope;
diff --git a/src/gn/starlark/crates/ffi/src/scope.rs b/src/gn/starlark/crates/ffi/src/scope.rs index d74f3a3..e485517 100644 --- a/src/gn/starlark/crates/ffi/src/scope.rs +++ b/src/gn/starlark/crates/ffi/src/scope.rs
@@ -51,20 +51,20 @@ } } -pub struct OwnedScope(pub cxx::UniquePtr<Scope>); +impl types::Scope for Scope { + type Owned = cxx::UniquePtr<Self>; -impl types::Scope for OwnedScope { - fn copy_with<'a, 'v>( + fn copy_with<'b, 'v>( &self, - kv: impl Iterator<Item = (&'a str, StarlarkValue<'v>)>, - ) -> starlark::Result<Self> { - let parent = self.0.as_ref().unwrap(); + kv: impl Iterator<Item = (&'b str, StarlarkValue<'v>)>, + ) -> starlark::Result<Self::Owned> { + let parent = self; // Scope stores a map from string_view to value. Since we don't know the // lifetime of the string we were given, we must intern it in order to // guarantee it can be safely dereferenced. let (keys, vals): (Vec<&str>, Vec<StarlarkValue<'v>>) = kv.map(|(s, v)| (intern_string(s), v)).unzip(); - let (mut child_scope, mut placeholders) = Scope::new(parent, &keys); + let (mut child_scope, mut placeholders) = Self::new(parent, &keys); let child_pin = child_scope.as_mut().unwrap(); for (placeholder, val) in placeholders.as_slice_mut().iter_mut().zip(vals) { @@ -73,12 +73,11 @@ .assign(val, None, child_pin.settings(), Default::default())?; } - Ok(Self(child_scope)) + Ok(child_scope) } fn get<'v>(&self, key: &str, heap: &Heap<'v>) -> Option<StarlarkValue<'v>> { - let parent = self.0.as_ref().unwrap(); - let val_ptr = crate::bridge::GetValue(parent, key); + let val_ptr = crate::bridge::GetValue(self, key); // Safety: val_ptr is either null or a valid pointer backed by the parent scope // lifetime. unsafe { val_ptr.as_ref() }.map(|val| val.to_rust(heap)) @@ -98,24 +97,25 @@ let parent_scope = setup.scope(); let (child_ptr, _) = Scope::new(&*parent_scope, &[]); - let owned_scope = OwnedScope(child_ptr); - starlark::environment::Module::with_temp_heap(|module| { let heap = module.heap(); let val_int = heap.alloc(42); let val_str = heap.alloc("hello"); - let grandchild = owned_scope + let grandchild_ptr = child_ptr .copy_with(vec![("foo", val_int), ("bar", val_str)].into_iter()) .unwrap(); - assert_eq!(grandchild.get("foo", &heap).unwrap().unpack_i32(), Some(42)); assert_eq!( - grandchild.get("bar", &heap).unwrap().unpack_str(), + grandchild_ptr.get("foo", &heap).unwrap().unpack_i32(), + Some(42) + ); + assert_eq!( + grandchild_ptr.get("bar", &heap).unwrap().unpack_str(), Some("hello") ); - assert!(grandchild.get("baz", &heap).is_none()); + assert!(grandchild_ptr.get("baz", &heap).is_none()); }); } }
diff --git a/src/gn/starlark/crates/rule/src/frozen_rule.rs b/src/gn/starlark/crates/rule/src/frozen_rule.rs index c7aff2e..c29fedd 100644 --- a/src/gn/starlark/crates/rule/src/frozen_rule.rs +++ b/src/gn/starlark/crates/rule/src/frozen_rule.rs
@@ -118,7 +118,7 @@ // implementation. let kwargs: SmallMap<String, Value<'v>> = param_parser.next()?; let child_scope = scope.copy_with(kwargs.iter().map(|(k, v)| (k.as_str(), *v)))?; - context.create_target(Some(builtin), target_name, &child_scope, me, attrs)? + context.create_target(Some(builtin), target_name, &*child_scope, me, attrs)? } else { context.create_target(None, target_name, scope, me, attrs)? };
diff --git a/src/gn/starlark/crates/testutils/src/eval_context.rs b/src/gn/starlark/crates/testutils/src/eval_context.rs index eac8062..570f750 100644 --- a/src/gn/starlark/crates/testutils/src/eval_context.rs +++ b/src/gn/starlark/crates/testutils/src/eval_context.rs
@@ -19,7 +19,12 @@ pub struct FakeScope(HashMap<String, Value<'static>>); impl Scope for FakeScope { - fn copy_with<'a, 'v>(&self, kv: impl Iterator<Item = (&'a str, Value<'v>)>) -> Result<Self> { + type Owned = Box<Self>; + + fn copy_with<'a, 'v>( + &self, + kv: impl Iterator<Item = (&'a str, Value<'v>)>, + ) -> Result<Self::Owned> { let mut values = self.0.clone(); for (k, v) in kv { // Safety: Transmuting 'v to 'static is safe because this mock scope @@ -28,7 +33,7 @@ let static_val = unsafe { std::mem::transmute::<Value<'v>, Value<'static>>(v) }; values.insert(k.to_owned(), static_val); } - Ok(Self(values)) + Ok(Box::new(Self(values))) } fn get<'v>(&self, key: &str, _heap: &Heap<'v>) -> Option<Value<'v>> {
diff --git a/src/gn/starlark/crates/types/src/scope.rs b/src/gn/starlark/crates/types/src/scope.rs index e3e9490..8a514b0 100644 --- a/src/gn/starlark/crates/types/src/scope.rs +++ b/src/gn/starlark/crates/types/src/scope.rs
@@ -10,13 +10,13 @@ /// the API we actually wish to use from rust, which may be an abstraction /// over that. pub trait Scope { + type Owned: std::ops::Deref<Target = Self>; + /// Creates a copy of the scope with some additional values set. fn copy_with<'a, 'v>( &self, kv: impl Iterator<Item = (&'a str, Value<'v>)>, - ) -> starlark::Result<Self> - where - Self: Sized; + ) -> starlark::Result<Self::Owned>; /// Retrieves a value from the key-value store. /// May allocate the value it retrieves on the heap.