Make EvalContext have interior mutability. This significantly simplifies the API to use EvalContext, and means that we can get rid of several unsafe APIs. We still use UnsafeCell internally for EvalContext, but since we removed the sync + send traits from EvalContext, EvalContext objects are now guarunteed by the compiler to be accessed single-threaded, so we don't need to worry about UnsafeCell actually being unsafe. Bug: 528225104 Change-Id: I397b6f108728267d33cf5d000ed6c9f66a6a6964 Reviewed-on: https://gn-review.googlesource.com/c/gn/+/24220 Reviewed-by: Takuto Ikuta <tikuta@google.com> Commit-Queue: Matt Stark <msta@google.com>
diff --git a/src/gn/starlark/crates/depset/src/depset.rs b/src/gn/starlark/crates/depset/src/depset.rs index fc98e27..db620f3 100644 --- a/src/gn/starlark/crates/depset/src/depset.rs +++ b/src/gn/starlark/crates/depset/src/depset.rs
@@ -119,12 +119,12 @@ pub fn new_file_depset<C: types::EvalContext>( direct: Vec<File>, heap: &Heap<'v>, - ctx: &mut C, + ctx: &C, ) -> starlark::Result<Self> { let phony = if direct.len() == 1 { Some(direct[0].clone()) } else if !direct.is_empty() { - Some(ctx.require_rule_impl_mut()?.new_phony(direct.clone())) + Some(ctx.require_rule_impl()?.new_phony(direct.clone())) } else { None }; @@ -358,7 +358,8 @@ let mut upto_phony = 0; // Collect the phonies we've seen since last time we called new_phonies. let mut new_phonies = |a: &Assert| { - let phonies = &a.context().rule_state.phonies; + use types::EvalContext as _; + let phonies = &a.context().require_rule_impl().unwrap().phonies; let result = &phonies[upto_phony..]; upto_phony = phonies.len(); result.to_vec()
diff --git a/src/gn/starlark/crates/depset/src/globals.rs b/src/gn/starlark/crates/depset/src/globals.rs index 70c4c47..06dbc7f 100644 --- a/src/gn/starlark/crates/depset/src/globals.rs +++ b/src/gn/starlark/crates/depset/src/globals.rs
@@ -18,7 +18,7 @@ transitive: Option<UnpackList<UnpackDepset<'v>>>, mut order: Order, heap: &Heap<'v>, - ctx: &mut C, + ctx: &C, ) -> starlark::Result<Value<'v>> { let mut kind = Kind::Empty; let mut set_kind = |k: Kind| -> Result<(), crate::Error> { @@ -117,7 +117,7 @@ let child_dep = UnpackDepset::unpack_value(*v).unwrap().unwrap(); deps.push(child_dep.phony().as_ref().unwrap().clone()); } - let state = ctx.require_rule_impl_mut()?; + let state = ctx.require_rule_impl()?; Some(state.new_phony(deps)) } else { None @@ -167,7 +167,7 @@ transitive, order, &eval.heap(), - eval.context_mut(), + eval.context(), ) } } @@ -191,7 +191,7 @@ eval: &mut Evaluator<'v, '_, '_>, ) -> starlark::Result<Depset<'v>> { let heap = eval.heap(); - let ctx = eval.context_mut::<testutils::eval_context::FakeEvalContext>(); + let ctx = eval.context::<testutils::eval_context::FakeEvalContext>(); let direct = files.items.into_iter().cloned().collect(); Depset::new_file_depset(direct, &heap, ctx) }
diff --git a/src/gn/starlark/crates/loader/src/loader.rs b/src/gn/starlark/crates/loader/src/loader.rs index b148751..6075689 100644 --- a/src/gn/starlark/crates/loader/src/loader.rs +++ b/src/gn/starlark/crates/loader/src/loader.rs
@@ -171,10 +171,10 @@ let loader = PreloadedLoader { modules: &deps_map }; Module::with_temp_heap(|module| { - let mut extra = make_eval_context(label.package()); + let extra = make_eval_context(label.package()); { let mut eval = Evaluator::new(&module); - eval.set_context(&mut *extra); + eval.set_context(&*extra); eval.set_loader(&loader); eval.eval_module(ast, globals)?; }
diff --git a/src/gn/starlark/crates/testutils/src/assert.rs b/src/gn/starlark/crates/testutils/src/assert.rs index cc6740c..cba6904 100644 --- a/src/gn/starlark/crates/testutils/src/assert.rs +++ b/src/gn/starlark/crates/testutils/src/assert.rs
@@ -34,16 +34,15 @@ // forces the framework to run the code only once (specifically, // under the "always GC" configuration), ensuring state is mutated only once. assert.always_gc(); - let mut context = Box::new(context); - let context_ptr = &mut *context as *mut FakeEvalContext; + let context = Box::new(context); + let context_ptr = &*context as *const FakeEvalContext; assert.setup_eval(move |eval| { // Safety: The context is owned by Assert, which outlives the evaluator run. // Since all evaluation methods on Assert require `&mut self`, this guarantees // exclusive access to `context` when the evaluator runs, so dereferencing // this pointer is safe and does not alias. - let context_mut = unsafe { &mut *context_ptr }; - eval.set_context(context_mut); + eval.set_context(unsafe { &*context_ptr }); }); let mut s = Self {
diff --git a/src/gn/starlark/crates/testutils/src/eval_context.rs b/src/gn/starlark/crates/testutils/src/eval_context.rs index 36c721c..d43fcab 100644 --- a/src/gn/starlark/crates/testutils/src/eval_context.rs +++ b/src/gn/starlark/crates/testutils/src/eval_context.rs
@@ -1,7 +1,7 @@ // 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 std::collections::HashMap; +use std::{cell::UnsafeCell, collections::HashMap}; use attr::{Attr, EvalContext as AttrEvalContext, EvalContextAttrExt, Session as AttrSession}; use starlark::{ @@ -54,7 +54,7 @@ pub path_resolver: PathResolver, /// The fake rule state. #[allocative(skip)] - pub rule_state: CtxState<FakeTargetRef>, + pub rule_state: UnsafeCell<CtxState<FakeTargetRef>>, /// The fake scope. #[allocative(skip)] pub scope: FakeScope, @@ -79,15 +79,15 @@ current_toolchain: session.default_toolchain.clone(), session, path_resolver: PathResolver::new_for_testing(), - rule_state: CtxState::new(FakeTargetRef::default()), + rule_state: CtxState::new(FakeTargetRef::default()).into(), scope: FakeScope::default(), } } } impl AttrEvalContext for FakeEvalContext { - type Session = FakeSession; type Scope = FakeScope; + type Session = FakeSession; fn session(&self) -> &Self::Session { &self.session @@ -113,14 +113,9 @@ Ok(()) } - fn require_rule_impl(&self) -> Result<&CtxState<<Self::Session as Session>::TargetRef>> { - Ok(&self.rule_state) - } - - fn require_rule_impl_mut( - &mut self, - ) -> Result<&mut CtxState<<Self::Session as Session>::TargetRef>> { - Ok(&mut self.rule_state) + fn require_rule_impl(&self) -> Result<&mut CtxState<<Self::Session as Session>::TargetRef>> { + // Safety: The eval context is single-threaded. + Ok(unsafe { &mut (*self.rule_state.get()) }) } }
diff --git a/src/gn/starlark/crates/testutils/src/target.rs b/src/gn/starlark/crates/testutils/src/target.rs index da1fa2c..94dcb2e 100644 --- a/src/gn/starlark/crates/testutils/src/target.rs +++ b/src/gn/starlark/crates/testutils/src/target.rs
@@ -41,16 +41,16 @@ && self.rule == other.rule && self.cxx_attrs.len() == other.cxx_attrs.len() && self.cxx_attrs.iter().all(|(k, v)| { - other.cxx_attrs.get(k).map_or(false, |ov| { - v.equals(*ov).unwrap_or(false) - }) + other + .cxx_attrs + .get(k) + .is_some_and(|ov| v.equals(*ov).unwrap_or(false)) }) && *self.dependencies.lock().unwrap() == *other.dependencies.lock().unwrap() } } impl Eq for FakeTarget {} - /// A reference to a fake target. #[derive(Debug, ProvidesStaticType, NoSerialize, Allocative, Clone)] pub struct FakeTargetRef(#[allocative(skip)] Arc<FakeTarget>);
diff --git a/src/gn/starlark/crates/types/src/eval_context.rs b/src/gn/starlark/crates/types/src/eval_context.rs index 73b8f87..9886372 100644 --- a/src/gn/starlark/crates/types/src/eval_context.rs +++ b/src/gn/starlark/crates/types/src/eval_context.rs
@@ -13,8 +13,6 @@ pub trait EvalContext: for<'v> starlark::values::ProvidesStaticType<'v, StaticType = Self> + allocative::Allocative - + Send - + Sync + 'static { /// The session type associated with this context. @@ -46,34 +44,29 @@ /// Asserts that the evaluator is executing a rule implementation, and /// returns the state of the rule implementation. + /// + /// We require EvalContext to have interior mutability, and thus it can + /// return a mutable reference to the state from an immutable reference. + #[allow(clippy::mut_from_ref)] fn require_rule_impl( &self, - ) -> starlark::Result<&crate::CtxState<<Self::Session as Session>::TargetRef>>; - - /// Asserts that the evaluator is executing a rule implementation, and - /// returns the mutable state of the rule implementation. - fn require_rule_impl_mut( - &mut self, ) -> starlark::Result<&mut crate::CtxState<<Self::Session as Session>::TargetRef>>; } -/// Extension trait to add the methods `.context` and `.context_mut` to the +/// Extension trait to add the methods `.context` and `.set_context` to the /// starlark Evaluator. pub trait EvaluatorContextExt<'v, 'a, 'e> { /// Returns a reference to the evaluation context. fn context<C: EvalContext>(&self) -> &C; - /// Returns a mutable reference to the evaluation context. - fn context_mut<C: EvalContext>(&mut self) -> &mut C; - /// Sets the evaluation context on the evaluator. - fn set_context<C: EvalContext>(&mut self, context: &'a mut C); + fn set_context<C: EvalContext>(&mut self, context: &'a C); } impl<'v, 'a, 'e> EvaluatorContextExt<'v, 'a, 'e> for starlark::eval::Evaluator<'v, 'a, 'e> { #[inline] fn context<C: EvalContext>(&self) -> &C { - let extra = self.extra_mut.as_ref(); + let extra = self.extra.as_ref(); debug_assert!(extra.is_some(), "evaluator context not set"); let dyn_any = unsafe { extra.unwrap_unchecked() }; debug_assert!(dyn_any.is::<C>(), "failed to downcast evaluator context"); @@ -81,16 +74,7 @@ } #[inline] - fn context_mut<C: EvalContext>(&mut self) -> &mut C { - let extra = self.extra_mut.as_mut(); - debug_assert!(extra.is_some(), "evaluator context not set"); - let dyn_any = unsafe { extra.unwrap_unchecked() }; - debug_assert!(dyn_any.is::<C>(), "failed to downcast evaluator context"); - unsafe { dyn_any.downcast_mut::<C>().unwrap_unchecked() } - } - - #[inline] - fn set_context<C: EvalContext>(&mut self, context: &'a mut C) { - self.extra_mut = Some(context); + fn set_context<C: EvalContext>(&mut self, context: &'a C) { + self.extra = Some(context); } }