Refactor depset and testutils to do a few things: * Allow use of make_file and depset functions from other tests * Allow use of `depset` function in real production code * Stop running doctests that don't exist * Remove deref to starlark::Assert, so we don't accidentally mess with invariants maintained by testutils (eg. setup_eval doesn't work because it overwrites the context). Bug: 528225104 Change-Id: I2723c59ddc0497a73c70dd0ae05430656a6a6964 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/23960 Reviewed-by: Philipp Wollermann <philwo@google.com> Reviewed-by: Matt Stark <msta@google.com>
diff --git a/src/gn/starlark/crates/depset/Cargo.toml b/src/gn/starlark/crates/depset/Cargo.toml index 63e6c0d..cf0def1 100644 --- a/src/gn/starlark/crates/depset/Cargo.toml +++ b/src/gn/starlark/crates/depset/Cargo.toml
@@ -7,6 +7,7 @@ workspace = true [lib] +doctest = false [dependencies] types = { path = "../types" }
diff --git a/src/gn/starlark/crates/depset/src/depset.rs b/src/gn/starlark/crates/depset/src/depset.rs index c1a5fe2..fc98e27 100644 --- a/src/gn/starlark/crates/depset/src/depset.rs +++ b/src/gn/starlark/crates/depset/src/depset.rs
@@ -244,10 +244,7 @@ use testutils::Assert; use super::*; - use crate::{ - globals::tests::{new_assert, test_globals}, - UnpackFileDepset, - }; + use crate::{globals::tests::new_assert, UnpackFileDepset}; #[test] fn test_depset_deduplication() { @@ -288,8 +285,7 @@ let frozen_str_depset = a.pass("depset(['a', 'b'])"); let frozen_file_depset = a.pass("depset([make_file('a.txt'), make_file('b.txt')])"); - a.globals_add(move |builder: &mut starlark::environment::GlobalsBuilder| { - test_globals(builder); + a.modify_globals(move |builder: &mut starlark::environment::GlobalsBuilder| { builder.set("frozen_str_depset", frozen_str_depset.clone()); builder.set("frozen_file_depset", frozen_file_depset.clone()); });
diff --git a/src/gn/starlark/crates/depset/src/globals.rs b/src/gn/starlark/crates/depset/src/globals.rs index 9a032c8..f1830af 100644 --- a/src/gn/starlark/crates/depset/src/globals.rs +++ b/src/gn/starlark/crates/depset/src/globals.rs
@@ -136,6 +136,46 @@ } } +#[doc(hidden)] +pub mod __private { + pub use starlark; + pub use starlark_derive::starlark_module; + pub use types::EvaluatorContextExt; +} + +/// Helper macro to register the `depset` function. +/// Required because this module isn't aware of the real EvaluatorContext type. +#[macro_export] +macro_rules! depset_globals { + ($builder:expr, $ctx_type:ty) => {{ + // starlark_module requires that the function returns something named + // "starlark::Result". + use $crate::__private::starlark; + + #[$crate::__private::starlark_module] + fn register_depset_globals(builder: &mut starlark::environment::GlobalsBuilder) { + fn depset<'v>( + direct: Option<starlark::values::list::UnpackList<starlark::values::Value<'v>>>, + transitive: Option<starlark::values::list::UnpackList<$crate::UnpackDepset<'v>>>, + #[starlark(default = $crate::Order::Unspecified)] order: $crate::Order, + eval: &mut starlark::eval::Evaluator<'v, '_, '_>, + ) -> starlark::Result<starlark::values::Value<'v>> { + use $crate::__private::EvaluatorContextExt; + + $crate::depset_constructor::<$ctx_type>( + direct, + transitive, + order, + &eval.heap(), + eval.context_mut(), + ) + } + } + + register_depset_globals($builder); + }}; +} + #[cfg(test)] pub(crate) mod tests { use starlark::{environment::GlobalsBuilder, eval::Evaluator}; @@ -146,13 +186,6 @@ #[starlark_module] pub(crate) fn test_globals(builder: &mut GlobalsBuilder) { - fn make_file<'v>( - eval: &mut Evaluator<'v, '_, '_>, - path: String, - ) -> starlark::Result<Value<'v>> { - Ok(eval.heap().alloc(types::File::intern(&path))) - } - fn new_file_depset<'v>( files: UnpackList<&File>, eval: &mut Evaluator<'v, '_, '_>, @@ -162,26 +195,14 @@ let direct = files.items.into_iter().cloned().collect(); Depset::new_file_depset(direct, &heap, ctx) } - - fn depset<'v>( - direct: Option<UnpackList<Value<'v>>>, - transitive: Option<UnpackList<crate::UnpackDepset<'v>>>, - #[starlark(default = crate::Order::Unspecified)] order: crate::Order, - eval: &mut Evaluator<'v, '_, '_>, - ) -> starlark::Result<Value<'v>> { - crate::depset_constructor::<testutils::FakeEvalContext>( - direct, - transitive, - order, - &eval.heap(), - eval.context_mut(), - ) - } } pub(crate) fn new_assert() -> testutils::Assert { let mut a = testutils::Assert::default(); - a.globals_add(test_globals); + a.modify_globals(|builder| { + test_globals(builder); + depset_globals!(builder, testutils::FakeEvalContext); + }); a } }
diff --git a/src/gn/starlark/crates/depset/src/lib.rs b/src/gn/starlark/crates/depset/src/lib.rs index 7d62b63..9c341d7 100644 --- a/src/gn/starlark/crates/depset/src/lib.rs +++ b/src/gn/starlark/crates/depset/src/lib.rs
@@ -10,5 +10,5 @@ pub use depset::{Depset, DepsetGen, FrozenDepset, Kind, Order}; pub use errors::Error; -pub use globals::depset_constructor; +pub use globals::{__private, depset_constructor}; pub use unpack::{UnpackDepset, UnpackFileDepset};
diff --git a/src/gn/starlark/crates/loader/Cargo.toml b/src/gn/starlark/crates/loader/Cargo.toml index 51998e2..67ddb92 100644 --- a/src/gn/starlark/crates/loader/Cargo.toml +++ b/src/gn/starlark/crates/loader/Cargo.toml
@@ -6,6 +6,9 @@ [lints] workspace = true +[lib] +doctest = false + [dependencies] types = { path = "../types" } starlark = { workspace = true }
diff --git a/src/gn/starlark/crates/testutils/src/assert.rs b/src/gn/starlark/crates/testutils/src/assert.rs index 9bbb8f3..143c3ab 100644 --- a/src/gn/starlark/crates/testutils/src/assert.rs +++ b/src/gn/starlark/crates/testutils/src/assert.rs
@@ -2,16 +2,17 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. -use starlark::values::UnpackValue; +use starlark::{environment::GlobalsBuilder, values::UnpackValue}; use types::{EvaluatorContextExt, UnpackedOwnedValue}; -use crate::FakeEvalContext; +use crate::{register_globals, FakeEvalContext}; /// A simple wrapper around starlark::Assert that provides fake evaluation /// contexts. pub struct Assert { assert: starlark::assert::Assert<'static>, context: Box<FakeEvalContext>, + globals_configs: Vec<Box<dyn Fn(&mut GlobalsBuilder)>>, } impl Default for Assert { @@ -43,7 +44,25 @@ eval.set_context(context_mut); }); - Self { assert, context } + let mut s = Self { + assert, + context, + globals_configs: vec![], + }; + s.modify_globals(register_globals); + s + } + + /// Adds a modifier to globals. + /// This modifier is applied after all existing modifiers. + pub fn modify_globals(&mut self, f: impl Fn(&mut GlobalsBuilder) + 'static) { + self.globals_configs.push(Box::new(f)); + // globals_add overwrites all previous calls to globals_add. + self.assert.globals_add(|builder| { + for config in &self.globals_configs { + config(builder); + } + }); } /// Returns a read-only reference to the fake evaluation context. @@ -79,11 +98,10 @@ } // We explicitly implement `pass`, `fail`, and `fails` with `&mut self` - // signatures despite `Deref` existing. The inherited methods on - // `starlark::assert::Assert` only take `&self`, which would bypass the - // borrow checker and allow running the evaluator while holding an active - // context borrow (leading to UB). Exposing them as `&mut self` methods on - // the wrapper statically prevents this. + // signatures. The inherited methods on `starlark::assert::Assert` only + // take `&self`, which would bypass the borrow checker and allow running + // the evaluator while holding an active context borrow (leading to UB). + // Exposing them as `&mut self` methods on the wrapper statically prevents this. /// Evaluates code and returns the Starlark value. #[track_caller] @@ -103,18 +121,3 @@ self.assert.fails(code, expected_errors) } } - -// We implement deref to get for free all the methods on starlark::Assert. -impl std::ops::Deref for Assert { - type Target = starlark::assert::Assert<'static>; - - fn deref(&self) -> &Self::Target { - &self.assert - } -} - -impl std::ops::DerefMut for Assert { - fn deref_mut(&mut self) -> &mut Self::Target { - &mut self.assert - } -}
diff --git a/src/gn/starlark/crates/testutils/src/globals.rs b/src/gn/starlark/crates/testutils/src/globals.rs new file mode 100644 index 0000000..a6b8165 --- /dev/null +++ b/src/gn/starlark/crates/testutils/src/globals.rs
@@ -0,0 +1,17 @@ +// Copyright 2026 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +use starlark::{environment::GlobalsBuilder, eval::Evaluator, values::Value}; +use starlark_derive::starlark_module; +use types::File; + +#[starlark_module] +pub fn register_globals(builder: &mut GlobalsBuilder) { + fn make_file<'v>( + path: String, + eval: &mut Evaluator<'v, '_, '_>, + ) -> starlark::Result<Value<'v>> { + Ok(eval.heap().alloc(File::intern(&path))) + } +}
diff --git a/src/gn/starlark/crates/testutils/src/lib.rs b/src/gn/starlark/crates/testutils/src/lib.rs index 52a4b4c..e75ac97 100644 --- a/src/gn/starlark/crates/testutils/src/lib.rs +++ b/src/gn/starlark/crates/testutils/src/lib.rs
@@ -4,10 +4,12 @@ pub mod assert; pub mod eval_context; +pub mod globals; pub mod session; pub mod target; pub use assert::Assert; pub use eval_context::FakeEvalContext; +pub use globals::register_globals; pub use session::FakeSession; pub use target::{FakeTarget, FakeTargetRef};