-
-
Notifications
You must be signed in to change notification settings - Fork 161
fix(compiler): require.main is the process entry module only #10749
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
d3e0581
fix(compiler): require.main is the process entry module only
perry-bot eec8a25
fix(compiler): publish require.main before any module init runs
perry-bot 974da5d
lint: fit the require.main change under the file-size cap; re-audit t…
perry-bot 784c35d
diag(gc): count and log CJS_MAIN_MODULE cache rewrites under PERRY_GC…
perry-bot ac9850e
test(gc): witness that CJS_MAIN_MODULE's cache is actually rewritten …
perry-bot 6a545c2
changelog: fragment for #10749 (require.main entry-only fix)
perry-bot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| Fixed `require.main === module` being trivially `true` in **every** compiled | ||
| CommonJS module, not just the process entry point (#10735). | ||
| `cjs_wrap`'s preamble unconditionally emitted `require.main = module;`, so | ||
| the standard "am I the entry, or merely imported?" idiom took its CLI | ||
| branch in every dependency that used it — including bundled packages | ||
| (dotenv 18.0.1's `dist/index.cjs` is a confirmed real-world example). | ||
|
|
||
| The fix threads the compiler's existing entry-module knowledge (the same | ||
| comparison `import.meta.main` uses) through `cjs_wrap`, but that alone is | ||
| insufficient: `cjs_wrap` transpiles a statically-known | ||
| `require('./relative')` into a hoisted ESM import, and ESM import | ||
| evaluation runs a module's static-import dependencies *before* the | ||
| importing module's own top-level code. So a CJS entry's own dependencies | ||
| initialize before the entry's own preamble runs, which means "the entry | ||
| publishes `require.main` in its own preamble" is too late for any | ||
| dependency reached via a hoisted static import. Fixed by publishing a | ||
| placeholder object as the shared "main module" from the program's `main()` | ||
| itself, before any module's `__init` runs at all (gated on the entry being | ||
| CJS-wrapped, so an ESM entry correctly leaves `require.main` `undefined` | ||
| for CJS modules it imports); the entry later reclaims that exact object | ||
| and fills in its real fields, preserving identity for dependencies that | ||
| captured `require.main` before the entry's own code ran. | ||
|
|
||
| The new runtime-side cache backing this (`CJS_MAIN_MODULE`) is registered | ||
| with the GC's mutable-root-scanner machinery and verified under forced | ||
| evacuation (`PERRY_GC_SCHEDULE_SEED`/`PERRY_GC_FORCE_EVACUATE`/ | ||
| `PERRY_GC_PROTECT_FROMSPACE`): the placeholder moved, the cache followed | ||
| it, and every identity assertion held across 8,006 forced collections. A | ||
| new test (`gc::tests::cjs_main_module`) asserts the rewrite counter is | ||
| non-zero under a real evacuating minor, so a future regression that stops | ||
| rewriting the cache fails a test instead of silently reintroducing this | ||
| bug's failure mode. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| //! #10735 witness: `scan_cjs_main_module_root_mut`'s rewrite counter is a | ||
| //! diagnostic nothing else reads, which is exactly the kind of thing that | ||
| //! rots silently -- a future change that stops rewriting the cache would | ||
| //! report `total_rewrites=0` forever while looking perfectly healthy. This | ||
| //! test forces a REAL evacuating collection (not a passive scan that never | ||
| //! sees a move) and asserts both that the placeholder's address actually | ||
| //! changed and that the counter tracked it, per CLAUDE.md's "a gate must | ||
| //! assert its subject was live" rule. | ||
| //! | ||
| //! Lives here rather than in `module_require.rs` because a real evacuating | ||
| //! minor needs `CopyingNurseryTestGuard`'s preflight setup (generated write | ||
| //! barriers reporting "active", the conservative-full-scan test default | ||
| //! turned off, a clean shadow stack / remembered set) -- machinery private | ||
| //! to this `gc::tests` tree. That guard also clears the thread's mutable | ||
| //! scanner registry so unrelated GC tests see only the roots they install, | ||
| //! which would remove the very scanner under test, so this file | ||
| //! re-registers it explicitly after constructing the guard. | ||
|
|
||
| use super::super::*; | ||
| use super::support::CopyingNurseryTestGuard; | ||
|
|
||
| #[test] | ||
| fn placeholder_move_under_forced_evacuation_increments_the_rewrite_counter() { | ||
| let _nursery = CopyingNurseryTestGuard::new(0); | ||
| let _evac = knob_overrides::ForcedEvacuationTestGuard::on(); | ||
| let _diag = GcDiagTestGuard::force_on(); | ||
| // `CopyingNurseryTestGuard::new` clears the thread's scanner registry so | ||
| // this collection sees exactly the roots the test installs -- put the | ||
| // one under test back. | ||
| gc_register_mutable_root_scanner(crate::module_require::scan_cjs_main_module_root_mut); | ||
|
|
||
| crate::module_require::js_bootstrap_cjs_main_module_placeholder(); | ||
| let before_bits = | ||
| crate::module_require::test_cjs_main_module_bits().expect("placeholder must be published"); | ||
| let rewrites_before = crate::module_require::test_cjs_main_module_rewrite_count(); | ||
|
|
||
| js_gc_collect(); | ||
|
|
||
| let after_bits = crate::module_require::test_cjs_main_module_bits() | ||
| .expect("placeholder must survive the collection"); | ||
| let rewrites_after = crate::module_require::test_cjs_main_module_rewrite_count(); | ||
|
|
||
| assert_ne!( | ||
| before_bits, after_bits, | ||
| "forced evacuation must have moved the placeholder -- if the \ | ||
| address is unchanged, this test's premise (a real move happened) \ | ||
| is false, and the counter assertion below would be checking \ | ||
| nothing (before={before_bits:#x} after={after_bits:#x})" | ||
| ); | ||
| assert!( | ||
| rewrites_after > rewrites_before, | ||
| "scan_cjs_main_module_root_mut must have counted the rewrite \ | ||
| (before={rewrites_before} after={rewrites_after}); a zero delta \ | ||
| here means the cache stopped following the object it caches -- \ | ||
| the exact regression #10735's identity guarantee depends on never \ | ||
| happening" | ||
| ); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 22015
Initialize the shared
require.mainplaceholder before dependency initialization.The CJS entry publishes a bare object before the non-entry
__initloop runs. A dependency can therefore readrequire.main.exports,require.main.loaded,require.main.filename, orrequire.main.requirewhile these properties are stillundefined. Populate the placeholder with the entry module’s initial record fields, including itsexportsobject, before running dependency initialization. Preserve the shared object identity so the entry preamble can complete the same record later.🤖 Prompt for AI Agents