Do not use eval.module_variables in eval.get_slot_module

Summary: See the top diff D58068387 for explanations.

Reviewed By: JakobDegen

Differential Revision: D58068275

fbshipit-source-id: 86d42249bad18b50794845611a8830c91b5ac0a2

starlark-rust/starlark/src/eval/compiler/def.rs

@@ -535,7 +535,7 @@ pub(crate) struct DefGen<V> {
     /// When the module is not frozen yet, this field contains `None`, and function's module
     /// can be accessed from evaluator's module.
     #[allocative(skip)]
-    module: AtomicFrozenRefOption<FrozenModuleData>,
+    pub(crate) module: AtomicFrozenRefOption<FrozenModuleData>,
     /// This field is only used in `FrozenDef`. It is populated in `post_freeze`.
     #[derivative(Debug = "ignore")]
     #[allocative(skip)]

starlark-rust/starlark/src/eval/runtime/cheap_call_stack.rs

@@ -197,17 +197,14 @@ impl<'v> CheapCallStack<'v> {
     }
 
     /// `n`-th element from the top of the stack.
-    pub(crate) fn top_nth_function(&self, n: usize) -> crate::Result<Value<'v>> {
-        let index = self
-            .count
-            .checked_sub(1)
-            .and_then(|x| x.checked_sub(n))
-            .ok_or_else(|| {
-                crate::Error::new_other(CallStackError::StackIsTooShallowForNthTopFrame(
-                    n, self.count,
-                ))
-            })?;
-        Ok(self.stack[index].function)
+    pub(crate) fn top_nth_function(&self, n: usize) -> anyhow::Result<Value<'v>> {
+        self.top_nth_function_opt(n)
+            .ok_or_else(|| CallStackError::StackIsTooShallowForNthTopFrame(n, self.count).into())
+    }
+
+    pub(crate) fn top_nth_function_opt(&self, n: usize) -> Option<Value<'v>> {
+        let index = self.count.checked_sub(1).and_then(|x| x.checked_sub(n))?;
+        Some(self.stack[index].function)
     }
 
     pub(crate) fn to_diagnostic_frames(&self, inlined_frames: InlinedFrames) -> CallStack {

starlark-rust/starlark/src/eval/runtime/evaluator.rs

@@ -515,21 +515,22 @@ impl<'v, 'a, 'e: 'a> Evaluator<'v, 'a, 'e> {
         #[cold]
         #[inline(never)]
         fn error<'v>(eval: &Evaluator<'v, '_, '_>, slot: ModuleSlotId) -> crate::Error {
-            let name = match &eval.module_variables {
-                None => eval
+            let name = match eval.top_frame_def_frozen_module(false) {
+                Err(e) => Some(format!("<internal error: {e}>")),
+                Ok(None) => eval
                     .module_env
                     .mutable_names()
                     .get_slot(slot)
                     .map(|s| s.as_str().to_owned()),
-                Some(e) => e.get_slot_name(slot).map(|s| s.as_str().to_owned()),
+                Ok(Some(e)) => e.get_slot_name(slot).map(|s| s.as_str().to_owned()),
             }
             .unwrap_or_else(|| "<unknown>".to_owned());
             crate::Error::new_other(EvaluatorError::LocalVariableReferencedBeforeAssignment(
                 name,
             ))
         }
 
-        match &self.module_variables {
+        match self.top_frame_def_frozen_module(false)? {
             None => self.module_env.slots().get_slot(slot),
             Some(e) => e.get_slot(slot).map(Value::new_frozen),
         }
@@ -690,19 +691,34 @@ impl<'v, 'a, 'e: 'a> Evaluator<'v, 'a, 'e> {
         self.func_to_def_info(func)
     }
 
+    pub(crate) fn top_frame_def_frozen_module(
+        &self,
+        for_debugger: bool,
+    ) -> anyhow::Result<Option<FrozenRef<'static, FrozenModuleData>>> {
+        let func = self.top_frame_maybe_for_debugger(for_debugger)?;
+        if let Some(func) = func.downcast_ref::<FrozenDef>() {
+            Ok(func.module.load_relaxed())
+        } else if let Some(func) = func.downcast_ref::<Def>() {
+            Ok(func.module.load_relaxed())
+        } else {
+            Ok(None)
+        }
+    }
+
+    fn top_frame_maybe_for_debugger(&self, for_debugger: bool) -> anyhow::Result<Value<'v>> {
+        let func = self.call_stack.top_nth_function(0)?;
+        if for_debugger && func.downcast_ref::<NativeFunction>().is_some() {
+            // If top frame is `breakpoint` or `debug_evaluate`, it will be skipped.
+            self.call_stack.top_nth_function(1)
+        } else {
+            Ok(func)
+        }
+    }
+
     /// Gets the "top frame" for debugging. If the real top frame is `breakpoint` or `debug_evaluate`
     /// it will be skipped. This should only be used for the starlark debugger.
     pub(crate) fn top_frame_def_info_for_debugger(&self) -> crate::Result<FrozenRef<DefInfo>> {
-        let func = {
-            let top = self.call_stack.top_nth_function(0)?;
-            if top.downcast_ref::<NativeFunction>().is_some() {
-                // we are in `breakpoint` or `debug_evaluate` function, get the next frame.
-                self.call_stack.top_nth_function(1)?
-            } else {
-                top
-            }
-        };
-
+        let func = self.top_frame_maybe_for_debugger(true)?;
         self.func_to_def_info(func)
     }