Remove double indirection in module types (#3640)

* Remove double indirection in module types

* Fix tests

* cargo fmt

core/engine/src/error.rs

@@ -46,16 +46,12 @@ use thiserror::Error;
 /// let kind = &native_error.as_native().unwrap().kind;
 /// assert!(matches!(kind, JsNativeErrorKind::Type));
 /// ```
-#[derive(Debug, Clone, Finalize, PartialEq, Eq)]
+#[derive(Debug, Clone, PartialEq, Eq, Trace, Finalize)]
+#[boa_gc(unsafe_no_drop)]
 pub struct JsError {
     inner: Repr,
 }
 
-// SAFETY: just mirroring the default derive to allow destructuring.
-unsafe impl Trace for JsError {
-    custom_trace!(this, mark, mark(&this.inner));
-}
-
 /// Internal representation of a [`JsError`].
 ///
 /// `JsError` is represented by an opaque enum because it restricts
@@ -66,24 +62,13 @@ unsafe impl Trace for JsError {
 /// This should never be used outside of this module. If that's not the case,
 /// you should add methods to either `JsError` or `JsNativeError` to
 /// represent that special use case.
-#[derive(Debug, Clone, Finalize, PartialEq, Eq)]
+#[derive(Debug, Clone, PartialEq, Eq, Trace, Finalize)]
+#[boa_gc(unsafe_no_drop)]
 enum Repr {
     Native(JsNativeError),
     Opaque(JsValue),
 }
 
-// SAFETY: just mirroring the default derive to allow destructuring.
-unsafe impl Trace for Repr {
-    custom_trace!(
-        this,
-        mark,
-        match &this {
-            Self::Native(err) => mark(err),
-            Self::Opaque(val) => mark(val),
-        }
-    );
-}
-
 /// The error type returned by the [`JsError::try_native`] method.
 #[derive(Debug, Clone, Error)]
 pub enum TryNativeError {

core/engine/src/module/mod.rs

@@ -63,7 +63,6 @@ impl std::fmt::Debug for Module {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         f.debug_struct("Module")
             .field("realm", &self.inner.realm.addr())
-            .field("environment", &self.inner.environment)
             .field("namespace", &self.inner.namespace)
             .field("kind", &self.inner.kind)
             .finish()
@@ -73,7 +72,6 @@ impl std::fmt::Debug for Module {
 #[derive(Trace, Finalize)]
 struct ModuleRepr {
     realm: Realm,
-    environment: GcRefCell<Option<Gc<DeclarativeEnvironment>>>,
     namespace: GcRefCell<Option<JsObject>>,
     kind: ModuleKind,
     host_defined: HostDefined,
@@ -88,6 +86,16 @@ pub(crate) enum ModuleKind {
     Synthetic(SyntheticModule),
 }
 
+impl ModuleKind {
+    /// Returns the inner `SourceTextModule`.
+    pub(crate) fn as_source_text(&self) -> Option<&SourceTextModule> {
+        match self {
+            ModuleKind::SourceText(src) => Some(src),
+            ModuleKind::Synthetic(_) => None,
+        }
+    }
+}
+
 /// Return value of the [`Module::resolve_export`] operation.
 ///
 /// Indicates how to access a specific export in a module.
@@ -126,7 +134,7 @@ struct GraphLoadingState {
     capability: PromiseCapability,
     loading: Cell<bool>,
     pending_modules: Cell<usize>,
-    visited: RefCell<HashSet<SourceTextModule>>,
+    visited: RefCell<HashSet<Module>>,
 }
 
 #[derive(Debug, Clone, Copy)]
@@ -151,19 +159,16 @@ impl Module {
         parser.set_identifier(context.next_parser_identifier());
         let module = parser.parse_module(context.interner_mut())?;
 
-        let inner = Gc::new_cyclic(|weak| {
+        let src = SourceTextModule::new(module, context.interner());
-            let src = SourceTextModule::new(module, weak.clone(), context.interner());
 
-            ModuleRepr {
+        Ok(Self {
+            inner: Gc::new(ModuleRepr {
                 realm: realm.unwrap_or_else(|| context.realm().clone()),
-                environment: GcRefCell::default(),
                 namespace: GcRefCell::default(),
                 kind: ModuleKind::SourceText(src),
                 host_defined: HostDefined::default(),
-            }
+            }),
-        });
+        })
-
-        Ok(Self { inner })
     }
 
     /// Abstract operation [`CreateSyntheticModule ( exportNames, evaluationSteps, realm )`][spec].
@@ -181,19 +186,16 @@ impl Module {
     ) -> Self {
         let names = export_names.iter().cloned().collect();
         let realm = realm.unwrap_or_else(|| context.realm().clone());
-        let inner = Gc::new_cyclic(|weak| {
+        let synth = SyntheticModule::new(names, evaluation_steps);
-            let synth = SyntheticModule::new(names, evaluation_steps, weak.clone());
 
-            ModuleRepr {
+        Self {
+            inner: Gc::new(ModuleRepr {
                 realm,
-                environment: GcRefCell::default(),
                 namespace: GcRefCell::default(),
                 kind: ModuleKind::Synthetic(synth),
                 host_defined: HostDefined::default(),
-            }
+            }),
-        });
+        }
-
-        Self { inner }
     }
 
     /// Gets the realm of this `Module`.
@@ -217,9 +219,12 @@ impl Module {
         &self.inner.kind
     }
 
-    /// Gets the environment of this `Module`.
+    /// Gets the declarative environment of this `Module`.
     pub(crate) fn environment(&self) -> Option<Gc<DeclarativeEnvironment>> {
-        self.inner.environment.borrow().clone()
+        match self.kind() {
+            ModuleKind::SourceText(src) => src.environment(),
+            ModuleKind::Synthetic(syn) => syn.environment(),
+        }
     }
 
     /// Abstract method [`LoadRequestedModules ( [ hostDefined ] )`][spec].
@@ -279,7 +284,7 @@ impl Module {
 
         if let ModuleKind::SourceText(src) = self.kind() {
             // continues on `inner_load
-            src.inner_load(state, context);
+            src.inner_load(self, state, context);
             if !state.loading.get() {
                 return;
             }
@@ -320,11 +325,11 @@ impl Module {
     /// [spec]: https://tc39.es/ecma262/#table-abstract-methods-of-module-records
     fn get_exported_names(
         &self,
-        export_star_set: &mut Vec<SourceTextModule>,
+        export_star_set: &mut Vec<Module>,
         interner: &Interner,
     ) -> FxHashSet<JsString> {
         match self.kind() {
-            ModuleKind::SourceText(src) => src.get_exported_names(export_star_set, interner),
+            ModuleKind::SourceText(src) => src.get_exported_names(self, export_star_set, interner),
             ModuleKind::Synthetic(synth) => synth.get_exported_names(),
         }
     }
@@ -348,8 +353,10 @@ impl Module {
         interner: &Interner,
     ) -> Result<ResolvedBinding, ResolveExportError> {
         match self.kind() {
-            ModuleKind::SourceText(src) => src.resolve_export(&export_name, resolve_set, interner),
+            ModuleKind::SourceText(src) => {
-            ModuleKind::Synthetic(synth) => synth.resolve_export(export_name),
+                src.resolve_export(self, &export_name, resolve_set, interner)
+            }
+            ModuleKind::Synthetic(synth) => synth.resolve_export(self, export_name),
         }
     }
 
@@ -367,9 +374,9 @@ impl Module {
     #[inline]
     pub fn link(&self, context: &mut Context) -> JsResult<()> {
         match self.kind() {
-            ModuleKind::SourceText(src) => src.link(context),
+            ModuleKind::SourceText(src) => src.link(self, context),
             ModuleKind::Synthetic(synth) => {
-                synth.link(context);
+                synth.link(self, context);
                 Ok(())
             }
         }
@@ -380,16 +387,16 @@ impl Module {
     /// [spec]: https://tc39.es/ecma262/#sec-InnerModuleLinking
     fn inner_link(
         &self,
-        stack: &mut Vec<SourceTextModule>,
+        stack: &mut Vec<Module>,
         index: usize,
         context: &mut Context,
     ) -> JsResult<usize> {
         match self.kind() {
-            ModuleKind::SourceText(src) => src.inner_link(stack, index, context),
+            ModuleKind::SourceText(src) => src.inner_link(self, stack, index, context),
             // If module is not a Cyclic Module Record, then
             ModuleKind::Synthetic(synth) => {
                 // a. Perform ? module.Link().
-                synth.link(context);
+                synth.link(self, context);
                 // b. Return index.
                 Ok(index)
             }
@@ -411,8 +418,8 @@ impl Module {
     #[inline]
     pub fn evaluate(&self, context: &mut Context) -> JsPromise {
         match self.kind() {
-            ModuleKind::SourceText(src) => src.evaluate(context),
+            ModuleKind::SourceText(src) => src.evaluate(self, context),
-            ModuleKind::Synthetic(synth) => synth.evaluate(context),
+            ModuleKind::Synthetic(synth) => synth.evaluate(self, context),
         }
     }
 
@@ -421,16 +428,16 @@ impl Module {
     /// [spec]: https://tc39.es/ecma262/#sec-InnerModuleLinking
     fn inner_evaluate(
         &self,
-        stack: &mut Vec<SourceTextModule>,
+        stack: &mut Vec<Module>,
         index: usize,
         context: &mut Context,
     ) -> JsResult<usize> {
         match self.kind() {
-            ModuleKind::SourceText(src) => src.inner_evaluate(stack, index, None, context),
+            ModuleKind::SourceText(src) => src.inner_evaluate(self, stack, index, None, context),
             // 1. If module is not a Cyclic Module Record, then
             ModuleKind::Synthetic(synth) => {
                 // a. Let promise be ! module.Evaluate().
-                let promise: JsPromise = synth.evaluate(context);
+                let promise: JsPromise = synth.evaluate(self, context);
                 let state = promise.state();
                 match state {
                     PromiseState::Pending => {
@@ -562,7 +569,7 @@ impl Module {
 impl PartialEq for Module {
     #[inline]
     fn eq(&self, other: &Self) -> bool {
-        std::ptr::eq(self.inner.as_ref(), other.inner.as_ref())
+        Gc::ptr_eq(&self.inner, &other.inner)
     }
 }
 

core/engine/src/module/synthetic.rs

@@ -1,19 +1,19 @@
 use std::rc::Rc;
 
-use boa_gc::{Finalize, Gc, GcRefCell, Trace, WeakGc};
+use boa_gc::{Finalize, Gc, GcRefCell, Trace};
 use rustc_hash::FxHashSet;
 
 use crate::{
     builtins::promise::ResolvingFunctions,
     bytecompiler::ByteCompiler,
-    environments::{CompileTimeEnvironment, EnvironmentStack},
+    environments::{CompileTimeEnvironment, DeclarativeEnvironment, EnvironmentStack},
     js_string,
     object::JsPromise,
     vm::{ActiveRunnable, CallFrame, CodeBlock},
     Context, JsNativeError, JsResult, JsString, JsValue, Module,
 };
 
-use super::{BindingName, ModuleRepr, ResolveExportError, ResolvedBinding};
+use super::{BindingName, ResolveExportError, ResolvedBinding};
 
 trait TraceableCallback: Trace {
     fn call(&self, module: &SyntheticModule, context: &mut Context) -> JsResult<()>;
@@ -135,62 +135,93 @@ impl SyntheticModuleInitializer {
 
     /// Calls this `SyntheticModuleInitializer`, forwarding the arguments to the corresponding function.
     #[inline]
-    pub fn call(&self, module: &SyntheticModule, context: &mut Context) -> JsResult<()> {
+    pub(crate) fn call(&self, module: &SyntheticModule, context: &mut Context) -> JsResult<()> {
         self.inner.call(module, context)
     }
 }
 
+/// Current status of a [`SyntheticModule`].
+#[derive(Debug, Trace, Finalize, Default)]
+#[boa_gc(unsafe_no_drop)]
+enum ModuleStatus {
+    #[default]
+    Unlinked,
+    Linked {
+        environment: Gc<DeclarativeEnvironment>,
+        eval_context: (EnvironmentStack, Gc<CodeBlock>),
+    },
+    Evaluated {
+        environment: Gc<DeclarativeEnvironment>,
+        promise: JsPromise,
+    },
+}
+
+impl ModuleStatus {
+    /// Transition from one state to another, taking the current state by value to move data
+    /// between states.
+    fn transition<F>(&mut self, f: F)
+    where
+        F: FnOnce(Self) -> Self,
+    {
+        *self = f(std::mem::take(self));
+    }
+}
+
 /// ECMAScript's [**Synthetic Module Records**][spec].
 ///
 /// [spec]: https://tc39.es/proposal-json-modules/#sec-synthetic-module-records
-#[derive(Clone, Trace, Finalize)]
+#[derive(Trace, Finalize)]
 pub struct SyntheticModule {
-    inner: Gc<Inner>,
+    #[unsafe_ignore_trace]
+    export_names: FxHashSet<JsString>,
+    eval_steps: SyntheticModuleInitializer,
+    state: GcRefCell<ModuleStatus>,
 }
 
 impl std::fmt::Debug for SyntheticModule {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         f.debug_struct("SyntheticModule")
-            .field("export_names", &self.inner.export_names)
+            .field("export_names", &self.export_names)
-            .field("eval_steps", &self.inner.eval_steps)
+            .field("eval_steps", &self.eval_steps)
             .finish_non_exhaustive()
     }
 }
 
-#[derive(Trace, Finalize)]
-struct Inner {
-    parent: WeakGc<ModuleRepr>,
-    #[unsafe_ignore_trace]
-    export_names: FxHashSet<JsString>,
-    eval_context: GcRefCell<Option<(EnvironmentStack, Gc<CodeBlock>)>>,
-    eval_steps: SyntheticModuleInitializer,
-}
-
 impl SyntheticModule {
-    /// Gets the parent module of this source module.
+    /// Abstract operation [`SetSyntheticModuleExport ( module, exportName, exportValue )`][spec].
-    fn parent(&self) -> Module {
+    ///
-        Module {
+    /// Sets or changes the exported value for `exportName` in the synthetic module.
-            inner: self
+    ///
-                .inner
+    /// # Note
-                .parent
+    ///
-                .upgrade()
+    /// The default export corresponds to the name `"default"`, but note that it needs to
-                .expect("parent module must be live"),
+    /// be passed to the list of exported names in [`Module::synthetic`] beforehand.
-        }
+    ///
+    /// [spec]: https://tc39.es/proposal-json-modules/#sec-createsyntheticmodule
+    pub fn set_export(&self, export_name: &JsString, export_value: JsValue) -> JsResult<()> {
+        let env = self.environment().ok_or_else(|| {
+            JsNativeError::typ().with_message(format!(
+                "cannot set name `{}` in an unlinked synthetic module",
+                export_name.to_std_string_escaped()
+            ))
+        })?;
+        let locator = env.compile_env().get_binding(export_name).ok_or_else(|| {
+            JsNativeError::reference().with_message(format!(
+                "cannot set name `{}` which was not included in the list of exports",
+                export_name.to_std_string_escaped()
+            ))
+        })?;
+        env.set(locator.binding_index(), export_value);
+
+        Ok(())
     }
 
     /// Creates a new synthetic module.
-    pub(super) fn new(
+    pub(super) fn new(names: FxHashSet<JsString>, eval_steps: SyntheticModuleInitializer) -> Self {
-        names: FxHashSet<JsString>,
-        eval_steps: SyntheticModuleInitializer,
-        parent: WeakGc<ModuleRepr>,
-    ) -> Self {
         Self {
-            inner: Gc::new(Inner {
+            export_names: names,
-                parent,
+            eval_steps,
-                export_names: names,
+            state: GcRefCell::default(),
-                eval_steps,
-                eval_context: GcRefCell::default(),
-            }),
         }
     }
 
@@ -207,7 +238,7 @@ impl SyntheticModule {
     /// [spec]: https://tc39.es/proposal-json-modules/#sec-smr-getexportednames
     pub(super) fn get_exported_names(&self) -> FxHashSet<JsString> {
         // 1. Return module.[[ExportNames]].
-        self.inner.export_names.clone()
+        self.export_names.clone()
     }
 
     /// Concrete method [`ResolveExport ( exportName )`][spec]
@@ -216,12 +247,13 @@ impl SyntheticModule {
     #[allow(clippy::mutable_key_type)]
     pub(super) fn resolve_export(
         &self,
+        module_self: &Module,
         export_name: JsString,
     ) -> Result<ResolvedBinding, ResolveExportError> {
-        if self.inner.export_names.contains(&export_name) {
+        if self.export_names.contains(&export_name) {
             // 2. Return ResolvedBinding Record { [[Module]]: module, [[BindingName]]: exportName }.
             Ok(ResolvedBinding {
-                module: self.parent(),
+                module: module_self.clone(),
                 binding_name: BindingName::Name(export_name),
             })
         } else {
@@ -233,12 +265,16 @@ impl SyntheticModule {
     /// Concrete method [`Link ( )`][spec].
     ///
     /// [spec]: https://tc39.es/ecma262/#sec-moduledeclarationlinking
-    pub(super) fn link(&self, context: &mut Context) {
+    pub(super) fn link(&self, module_self: &Module, context: &mut Context) {
-        let parent = self.parent();
+        if !matches!(&*self.state.borrow(), ModuleStatus::Unlinked) {
+            // Already linked and/or evaluated.
+            return;
+        }
+
         // 1. Let realm be module.[[Realm]].
         // 2. Let env be NewModuleEnvironment(realm.[[GlobalEnv]]).
         // 3. Set module.[[Environment]] to env.
-        let global_env = parent.realm().environment().clone();
+        let global_env = module_self.realm().environment().clone();
         let global_compile_env = global_env.compile_env();
         let module_compile_env = Rc::new(CompileTimeEnvironment::new(global_compile_env, true));
 
@@ -255,7 +291,6 @@ impl SyntheticModule {
 
         // 4. For each String exportName in module.[[ExportNames]], do
         let exports = self
-            .inner
             .export_names
             .iter()
             .map(|name| {
@@ -278,9 +313,18 @@ impl SyntheticModule {
             );
         }
 
-        *parent.inner.environment.borrow_mut() = envs.current().as_declarative().cloned();
+        let env = envs
+            .current()
+            .as_declarative()
+            .cloned()
+            .expect("should have the module environment");
 
-        *self.inner.eval_context.borrow_mut() = Some((envs, cb));
+        self.state
+            .borrow_mut()
+            .transition(|_| ModuleStatus::Linked {
+                environment: env,
+                eval_context: (envs, cb),
+            });
 
         // 5. Return unused.
     }
@@ -288,23 +332,34 @@ impl SyntheticModule {
     /// Concrete method [`Evaluate ( )`][spec].
     ///
     /// [spec]: https://tc39.es/proposal-json-modules/#sec-smr-Evaluate
-    pub(super) fn evaluate(&self, context: &mut Context) -> JsPromise {
+    pub(super) fn evaluate(&self, module_self: &Module, context: &mut Context) -> JsPromise {
+        let (environments, codeblock) = match &*self.state.borrow() {
+            ModuleStatus::Unlinked => {
+                let (promise, ResolvingFunctions { reject, .. }) = JsPromise::new_pending(context);
+                reject
+                    .call(
+                        &JsValue::undefined(),
+                        &[JsNativeError::typ()
+                            .with_message("cannot evaluate unlinked synthetic module")
+                            .to_opaque(context)
+                            .into()],
+                        context,
+                    )
+                    .expect("native resolving functions cannot throw");
+                return promise;
+            }
+            ModuleStatus::Linked { eval_context, .. } => eval_context.clone(),
+            ModuleStatus::Evaluated { promise, .. } => return promise.clone(),
+        };
         // 1. Let moduleContext be a new ECMAScript code execution context.
 
-        let parent = self.parent();
+        let realm = module_self.realm().clone();
-        let realm = parent.realm().clone();
-        let (environments, codeblock) = self
-            .inner
-            .eval_context
-            .borrow()
-            .clone()
-            .expect("should have been initialized on `link`");
 
         let env_fp = environments.len() as u32;
         let callframe = CallFrame::new(
             codeblock,
             // 4. Set the ScriptOrModule of moduleContext to module.
-            Some(ActiveRunnable::Module(parent)),
+            Some(ActiveRunnable::Module(module_self.clone())),
             // 5. Set the VariableEnvironment of moduleContext to module.[[Environment]].
             // 6. Set the LexicalEnvironment of moduleContext to module.[[Environment]].
             environments,
@@ -322,7 +377,7 @@ impl SyntheticModule {
 
         // 9. Let steps be module.[[EvaluationSteps]].
         // 10. Let result be Completion(steps(module)).
-        let result = self.inner.eval_steps.call(self, context);
+        let result = self.eval_steps.call(self, context);
 
         // 11. Suspend moduleContext and remove it from the execution context stack.
         // 12. Resume the context that is now on the top of the execution context stack as the running execution context.
@@ -340,36 +395,23 @@ impl SyntheticModule {
         }
         .expect("default resolving functions cannot throw");
 
+        self.state.borrow_mut().transition(|state| match state {
+            ModuleStatus::Linked { environment, .. } => ModuleStatus::Evaluated {
+                environment,
+                promise: promise.clone(),
+            },
+            _ => unreachable!("checks above ensure the module is linked"),
+        });
+
         // 16. Return pc.[[Promise]].
         promise
     }
 
-    /// Abstract operation [`SetSyntheticModuleExport ( module, exportName, exportValue )`][spec].
+    pub(crate) fn environment(&self) -> Option<Gc<DeclarativeEnvironment>> {
-    ///
+        match &*self.state.borrow() {
-    /// Sets or changes the exported value for `exportName` in the synthetic module.
+            ModuleStatus::Unlinked => None,
-    ///
+            ModuleStatus::Linked { environment, .. }
-    /// # Note
+            | ModuleStatus::Evaluated { environment, .. } => Some(environment.clone()),
-    ///
+        }
-    /// The default export corresponds to the name `"default"`, but note that it needs to
-    /// be passed to the list of exported names in [`Module::synthetic`] beforehand.
-    ///
-    /// [spec]: https://tc39.es/proposal-json-modules/#sec-createsyntheticmodule
-    pub fn set_export(&self, export_name: &JsString, export_value: JsValue) -> JsResult<()> {
-        let environment = self
-            .parent()
-            .environment()
-            .expect("this must be initialized before evaluating");
-        let locator = environment
-            .compile_env()
-            .get_binding(export_name)
-            .ok_or_else(|| {
-                JsNativeError::reference().with_message(format!(
-                    "cannot set name `{}` which was not included in the list of exports",
-                    export_name.to_std_string_escaped()
-                ))
-            })?;
-        environment.set(locator.binding_index(), export_value);
-
-        Ok(())
     }
 }

core/gc/src/trace.rs

@@ -1,7 +1,7 @@
 use std::{
     any::TypeId,
     borrow::{Cow, ToOwned},
-    cell::Cell,
+    cell::{Cell, OnceCell},
     collections::{BTreeMap, BTreeSet, BinaryHeap, HashMap, HashSet, LinkedList, VecDeque},
     hash::{BuildHasher, Hash},
     marker::PhantomData,
@@ -491,6 +491,16 @@ unsafe impl<T: Trace> Trace for Cell<Option<T>> {
     });
 }
 
+impl<T: Trace> Finalize for OnceCell<T> {}
+// SAFETY: We only trace the inner cell if the cell has a value.
+unsafe impl<T: Trace> Trace for OnceCell<T> {
+    custom_trace!(this, mark, {
+        if let Some(v) = this.get() {
+            mark(v);
+        }
+    });
+}
+
 #[cfg(feature = "icu")]
 mod icu {
     use icu_locid::{LanguageIdentifier, Locale};