Cleanup CommonJS changes (#3112)bun-v0.6.5

* Add more GC in test

* Fix handling of functions and re-assignments in CommonJS

* Increase timeout

---------

Co-authored-by: Jarred Sumner <709451+Jarred-Sumner@users.noreply.github.com>

1 files changed, 220 insertions, 43 deletions

diff --git a/src/bun.js/bindings/CommonJSModuleRecord.cpp b/src/bun.js/bindings/CommonJSModuleRecord.cpp
index c77027983..8b4fdf60a 100644
--- a/src/bun.js/bindings/CommonJSModuleRecord.cpp
+++ b/src/bun.js/bindings/CommonJSModuleRecord.cpp
@@ -1,3 +1,38 @@
+/**
+ * How this works
+ *
+ * CommonJS modules are transpiled by Bun's transpiler to the following:
+ *
+ * (function (exports, require, module) { ... code })(exports, require, module)
+ *
+ * Then, at runtime, we create a JSCommonJSModule object.
+ *
+ * On this special object, we override the setter for the "exports" property in
+ * a non-observable way (`static bool put ...`)
+ *
+ * When the setter is called, we set the internal "exports" property to the
+ * value passed in and we also update the requireMap with the new value.
+ *
+ * After the CommonJS module is executed, we:
+ * - Store the exports value in the requireMap (again)
+ * - Loop through the keys of the exports object and re-export as ES Module
+ *   named exports
+ *
+ * If an exception occurs, we remove the entry from the requireMap.
+ *
+ * We tried using a CustomGetterSetter instead of overriding `put`, but it led
+ * to returning the getter itself
+ *
+ * How cyclical dependencies are handled
+ *
+ * Before executing the CommonJS module, we set the exports object in the
+ * requireMap to an empty object. When the CommonJS module is required again, we
+ * return the exports object from the requireMap. The values should be in sync
+ * while the module is being executed, unless module.exports is re-assigned to a
+ * different value. In that case, it will have a stale value.
+ *
+ */
+
 #include "root.h"
 #include "headers-handwritten.h"
 #include "ZigGlobalObject.h"
@@ -31,13 +66,136 @@
 namespace Bun {
 using namespace JSC;
 
-JSC::Structure* createCommonJSModuleStructure(
+static Structure* internalCreateCommonJSModuleStructure(
+    Zig::GlobalObject* globalObject);
+
+class JSCommonJSModule final : public JSC::JSNonFinalObject {
+public:
+    using Base = JSC::JSNonFinalObject;
+    static constexpr unsigned StructureFlags = Base::StructureFlags | JSC::OverridesPut;
+
+    mutable JSC::WriteBarrier<JSC::Unknown> m_exportsObject;
+    mutable JSC::WriteBarrier<JSC::JSString> m_id;
+
+    void finishCreation(JSC::VM& vm, JSC::JSValue exportsObject, JSC::JSString* id)
+    {
+        Base::finishCreation(vm);
+        ASSERT(inherits(vm, info()));
+        m_exportsObject.set(vm, this, exportsObject);
+        m_id.set(vm, this, id);
+
+        this->putDirectOffset(
+            vm,
+            0,
+            exportsObject);
+
+        this->putDirectOffset(
+            vm,
+            1,
+            id);
+
+        this->putDirectOffset(
+            vm,
+            2,
+            id);
+    }
+
+    static JSCommonJSModule* create(
+        JSC::VM& vm,
+        JSC::Structure* structure,
+        JSC::JSValue exportsObject,
+        JSC::JSString* id)
+    {
+        JSCommonJSModule* cell = new (NotNull, JSC::allocateCell<JSCommonJSModule>(vm)) JSCommonJSModule(vm, structure);
+        cell->finishCreation(vm, exportsObject, id);
+        return cell;
+    }
+
+    JSValue exportsObject()
+    {
+        return m_exportsObject.get();
+    }
+
+    JSValue id()
+    {
+        return m_id.get();
+    }
+
+    DECLARE_VISIT_CHILDREN;
+
+    static bool put(
+        JSC::JSCell* cell,
+        JSC::JSGlobalObject* globalObject,
+        JSC::PropertyName propertyName,
+        JSC::JSValue value,
+        JSC::PutPropertySlot& slot)
+    {
+        JSCommonJSModule* thisObject = jsCast<JSCommonJSModule*>(cell);
+        ASSERT_GC_OBJECT_INHERITS(thisObject, info());
+        auto& vm = globalObject->vm();
+        auto throwScope = DECLARE_THROW_SCOPE(vm);
+
+        auto* clientData = WebCore::clientData(vm);
+        bool result = Base::put(thisObject, globalObject, propertyName, value, slot);
+        if (result) {
+            // Whenever you call module.exports = ... in a module, we need to:
+            //
+            // - Update the internal exports object
+            // - Update the require map
+            //
+            if (propertyName == clientData->builtinNames().exportsPublicName()) {
+                thisObject->m_exportsObject.set(vm, thisObject, value);
+                Zig::GlobalObject* zigGlobalObject = jsCast<Zig::GlobalObject*>(globalObject);
+                zigGlobalObject->requireMap()->set(globalObject, thisObject->id(), value);
+                RETURN_IF_EXCEPTION(throwScope, false);
+            }
+        }
+
+        RELEASE_AND_RETURN(throwScope, result);
+    }
+
+    static JSC::Structure* createStructure(
+        JSC::JSGlobalObject* globalObject)
+    {
+        return internalCreateCommonJSModuleStructure(reinterpret_cast<Zig::GlobalObject*>(globalObject));
+    }
+
+    DECLARE_INFO;
+    template<typename, SubspaceAccess mode> static JSC::GCClient::IsoSubspace* subspaceFor(JSC::VM& vm)
+    {
+        if constexpr (mode == JSC::SubspaceAccess::Concurrently)
+            return nullptr;
+        return WebCore::subspaceForImpl<JSCommonJSModule, WebCore::UseCustomHeapCellType::No>(
+            vm,
+            [](auto& spaces) { return spaces.m_clientSubspaceForCommonJSModuleRecord.get(); },
+            [](auto& spaces, auto&& space) { spaces.m_clientSubspaceForCommonJSModuleRecord = std::forward<decltype(space)>(space); },
+            [](auto& spaces) { return spaces.m_subspaceForCommonJSModuleRecord.get(); },
+            [](auto& spaces, auto&& space) { spaces.m_subspaceForCommonJSModuleRecord = std::forward<decltype(space)>(space); });
+    }
+
+    JSCommonJSModule(JSC::VM& vm, JSC::Structure* structure)
+        : Base(vm, structure)
+    {
+    }
+};
+
+Structure* createCommonJSModuleStructure(
+    Zig::GlobalObject* globalObject)
+{
+    return JSCommonJSModule::createStructure(globalObject);
+}
+
+static Structure* internalCreateCommonJSModuleStructure(
     Zig::GlobalObject* globalObject)
 {
     auto& vm = globalObject->vm();
-    JSC::Structure* structure = globalObject->structureCache().emptyObjectStructureForPrototype(
+    JSC::Structure* structure = JSC::Structure::create(
+        vm,
         globalObject,
         globalObject->objectPrototype(),
+        JSC::TypeInfo(JSC::ObjectType, JSCommonJSModule::StructureFlags),
+        JSCommonJSModule::info(),
+        JSC::NonArray,
         4);
 
     JSC::PropertyOffset offset;
@@ -74,34 +232,34 @@ JSC::Structure* createCommonJSModuleStructure(
     return structure;
 }
 
-JSC::JSObject* createCommonJSModuleObject(
+template<typename Visitor>
+void JSCommonJSModule::visitChildrenImpl(JSCell* cell, Visitor& visitor)
+{
+    JSCommonJSModule* thisObject = jsCast<JSCommonJSModule*>(cell);
+    ASSERT_GC_OBJECT_INHERITS(thisObject, info());
+    Base::visitChildren(thisObject, visitor);
+    visitor.append(thisObject->m_exportsObject);
+    visitor.append(thisObject->m_id);
+}
+
+DEFINE_VISIT_CHILDREN(JSCommonJSModule);
+const JSC::ClassInfo JSCommonJSModule::s_info = { "Module"_s, &Base::s_info, nullptr, nullptr, CREATE_METHOD_TABLE(JSCommonJSModule) };
+
+JSCommonJSModule* createCommonJSModuleObject(
     Zig::GlobalObject* globalObject,
-    const ResolvedSource& source, const WTF::String& sourceURL, JSC::JSValue exportsObjectValue, JSC::JSValue requireFunctionValue)
+    const ResolvedSource& source,
+    const WTF::String& sourceURL,
+    JSC::JSValue exportsObjectValue,
+    JSC::JSValue requireFunctionValue)
 {
     auto& vm = globalObject->vm();
     auto scope = DECLARE_THROW_SCOPE(vm);
-
-    JSC::JSObject* moduleObject = JSC::constructEmptyObject(
-        vm,
-        globalObject->CommonJSModuleObjectStructure());
-
-    RETURN_IF_EXCEPTION(scope, nullptr);
-
-    moduleObject->putDirectOffset(
-        vm,
-        0,
-        exportsObjectValue);
-
     auto* jsSourceURL = JSC::jsString(vm, sourceURL);
-    moduleObject->putDirectOffset(
-        vm,
-        1,
-        jsSourceURL);
 
-    moduleObject->putDirectOffset(
+    JSCommonJSModule* moduleObject = JSCommonJSModule::create(
         vm,
-        2,
-        // TODO: filename should be substring
+        globalObject->CommonJSModuleObjectStructure(),
+        exportsObjectValue,
         jsSourceURL);
 
     moduleObject->putDirectOffset(
@@ -146,21 +304,33 @@ JSC::SourceCode createCommonJSModule(
                 auto& vm = globalObject->vm();
                 auto throwScope = DECLARE_THROW_SCOPE(vm);
                 auto sourceCodeString = Zig::toString(source.source_code);
+                auto* requireMapKey = jsString(vm, sourceURL);
+
+                JSC::JSObject* exportsObject = source.commonJSExportsLen < 64
+                    ? JSC::constructEmptyObject(globalObject, globalObject->objectPrototype(), source.commonJSExportsLen)
+                    : JSC::constructEmptyObject(globalObject, globalObject->objectPrototype());
+
+                if (!globalObject->requireMap()->has(globalObject, requireMapKey)) {
+                    globalObject->requireMap()->set(globalObject, requireMapKey, exportsObject);
+                }
+
                 JSC::SourceCode inputSource(
                     JSC::StringSourceProvider::create(sourceCodeString,
                         JSC::SourceOrigin(WTF::URL::fileURLWithFileSystemPath(sourceURL)),
                         sourceURL, TextPosition()));
 
+                JSC::Structure* scopeExtensionObjectStructure = globalObject->commonJSFunctionArgumentsStructure();
                 JSC::JSObject* scopeExtensionObject = JSC::constructEmptyObject(
                     vm,
-                    globalObject->commonJSFunctionArgumentsStructure());
+                    scopeExtensionObjectStructure);
 
                 auto* requireFunction = Zig::ImportMetaObject::createRequireFunction(vm, globalObject, sourceURL);
 
-                JSC::JSObject* exportsObject = source.commonJSExportsLen < 64
-                    ? JSC::constructEmptyObject(globalObject, globalObject->objectPrototype(), source.commonJSExportsLen)
-                    : JSC::constructEmptyObject(globalObject, globalObject->objectPrototype());
-                auto* moduleObject = createCommonJSModuleObject(globalObject, source, sourceURL, exportsObject, requireFunction);
+                auto* moduleObject = createCommonJSModuleObject(globalObject,
+                    source,
+                    sourceURL,
+                    exportsObject,
+                    requireFunction);
                 scopeExtensionObject->putDirectOffset(
                     vm,
                     0,
@@ -180,47 +350,54 @@ JSC::SourceCode createCommonJSModule(
                     globalObject, inputSource, DerivedContextType::None, NeedsClassFieldInitializer::No, PrivateBrandRequirement::None,
                     false, false, EvalContextType::None, nullptr, nullptr, ECMAMode::sloppy());
 
-                RETURN_IF_EXCEPTION(throwScope, void());
-
-                if (UNLIKELY(!executable)) {
+                if (UNLIKELY(!executable && !throwScope.exception())) {
                     throwSyntaxError(globalObject, throwScope, "Failed to compile CommonJS module."_s);
+                }
+
+                if (UNLIKELY(throwScope.exception())) {
+                    globalObject->requireMap()->remove(globalObject, requireMapKey);
                     return;
                 }
 
-                auto* contextScope = JSC::JSWithScope::create(vm, globalObject, globalObject->globalScope(), scopeExtensionObject);
-                auto* requireMapKey = jsString(vm, sourceURL);
+                auto catchScope = DECLARE_CATCH_SCOPE(vm);
 
-                globalObject->requireMap()->set(globalObject, requireMapKey, exportsObject);
+                JSC::JSWithScope* withScope = JSC::JSWithScope::create(vm, globalObject, globalObject->globalScope(), scopeExtensionObject);
+
+                vm.interpreter.executeEval(executable, globalObject, withScope);
 
-                auto catchScope = DECLARE_CATCH_SCOPE(vm);
-                vm.interpreter.executeEval(executable, globalObject, contextScope);
                 if (UNLIKELY(catchScope.exception())) {
                     auto returnedException = catchScope.exception();
                     catchScope.clearException();
-                    globalObject->requireMap()->remove(globalObject, requireMapKey);
                     JSC::throwException(globalObject, throwScope, returnedException);
                 }
 
-                if (throwScope.exception())
+                if (throwScope.exception()) {
+                    globalObject->requireMap()->remove(globalObject, requireMapKey);
                     return;
+                }
 
-                JSValue result = moduleObject->getDirect(0);
+                JSValue result = moduleObject->exportsObject();
 
-                if (result != exportsObject)
-                    globalObject->requireMap()->set(globalObject, requireMapKey, result);
+                globalObject->requireMap()->set(globalObject, requireMapKey, result);
 
                 exportNames.append(vm.propertyNames->defaultKeyword);
                 exportValues.append(result);
+
+                // This exists to tell ImportMetaObject.ts that this is a CommonJS module.
                 exportNames.append(Identifier::fromUid(vm.symbolRegistry().symbolForKey("CommonJS"_s)));
                 exportValues.append(jsNumber(0));
 
+                // This strong reference exists because otherwise it will crash when the finalizer runs.
+                exportNames.append(Identifier::fromUid(vm.symbolRegistry().symbolForKey("module"_s)));
+                exportValues.append(moduleObject);
+
                 if (result.isObject()) {
                     auto* exports = asObject(result);
 
                     auto* structure = exports->structure();
                     uint32_t size = structure->inlineSize() + structure->outOfLineSize();
-                    exportNames.reserveCapacity(size);
-                    exportValues.ensureCapacity(size);
+                    exportNames.reserveCapacity(size + 3);
+                    exportValues.ensureCapacity(size + 3);
 
                     if (canPerformFastEnumeration(structure)) {
                         exports->structure()->forEachProperty(vm, [&](const PropertyTableEntry& entry) -> bool {