feat(compile): lower static literal require in user modules#5447
Closed
JagritGumber wants to merge 1 commit into
Closed
feat(compile): lower static literal require in user modules#5447JagritGumber wants to merge 1 commit into
JagritGumber wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughReplaces ChangesStatic Require Transform Replacement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry/src/commands/compile/collect_modules.rs (1)
430-446:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPrefix-line correction drops negative deltas, which can mis-map debug lines.
At Line 440,
saturating_subonly handles added lines. If the static-require rewrite removes line breaks (e.g., multilinerequire(...)call collapsed to one identifier), the delta is negative and currently treated as0, soprefix_line_countbecomes too large.Suggested fix
- let lines_after_transform = source.bytes().filter(|&b| b == b'\n').count(); - let added_lines = lines_after_transform.saturating_sub(lines_before_transform) as u32; + let lines_after_transform = source.bytes().filter(|&b| b == b'\n').count(); + let line_delta = lines_after_transform as i64 - lines_before_transform as i64; + let adjusted_prefix = if line_delta >= 0 { + prefix_lines.saturating_add(line_delta as u32) + } else { + prefix_lines.saturating_sub((-line_delta) as u32) + }; ctx.cjs_wrap_debug_sources.insert( canonical.clone(), super::types::CjsWrapDebugSource { wrapped_source: source.clone(), - prefix_line_count: prefix_lines + added_lines, + prefix_line_count: adjusted_prefix, }, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry/src/commands/compile/collect_modules.rs` around lines 430 - 446, The calculation of the line delta in the block after transform_static_literal_requires is called uses saturating_sub which treats removed lines as zero, causing incorrect prefix_line_count when the static-require transform collapses multiline requires into single identifiers. Replace the saturating_sub calculation of added_lines with logic that properly computes the signed delta between lines_after_transform and lines_before_transform, allowing both positive and negative deltas, then apply this delta appropriately to prefix_lines to ensure prefix_line_count accounts for lines that were both added and removed during the transform.
🧹 Nitpick comments (1)
crates/perry/src/commands/compile/collect_modules/static_require_transform.rs (1)
28-53: ⚡ Quick win
HashSetiteration makes generated import/temp ordering nondeterministic.At Line 28, iterating
require_aliasesdirectly can vary between runs, changing import insertion order and temp-id assignment. That hurts reproducibility and can create noisy cache/output diffs.Suggested fix
- for alias in require_aliases { + let mut aliases: Vec<_> = require_aliases.into_iter().collect(); + aliases.sort(); + for alias in aliases { let call_re = literal_require_call_re(&alias); for cap in call_re.captures_iter(source) { ... } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry/src/commands/compile/collect_modules/static_require_transform.rs` around lines 28 - 53, The iteration over require_aliases at the start of the loop is nondeterministic because HashSet iteration order varies between runs, causing the order of imports and temp-id assignments to change unpredictably. Sort the require_aliases collection before iterating over it in the for loop to ensure a consistent, deterministic ordering of the imports and temporary variable names generated during each compilation run.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@crates/perry/src/commands/compile/collect_modules/static_require_transform.rs`:
- Around line 16-18: The current implementation of
require_is_shadowed_by_non_create_require uses a file-global boolean check that
does not respect lexical scoping, causing it to miss true local shadowing (such
as require as a function parameter) and incorrectly suppress valid rewrites in
outer scopes. Refactor the shadowing detection logic to be scope-aware by
implementing token or AST-based analysis that checks whether require is shadowed
at the specific lexical position of each require call, rather than maintaining a
single file-wide shadow flag. This ensures that the condition checked in the if
statement at line 16-18 accurately validates shadowing based on local bindings
at the exact location where require is being rewritten.
---
Outside diff comments:
In `@crates/perry/src/commands/compile/collect_modules.rs`:
- Around line 430-446: The calculation of the line delta in the block after
transform_static_literal_requires is called uses saturating_sub which treats
removed lines as zero, causing incorrect prefix_line_count when the
static-require transform collapses multiline requires into single identifiers.
Replace the saturating_sub calculation of added_lines with logic that properly
computes the signed delta between lines_after_transform and
lines_before_transform, allowing both positive and negative deltas, then apply
this delta appropriately to prefix_lines to ensure prefix_line_count accounts
for lines that were both added and removed during the transform.
---
Nitpick comments:
In
`@crates/perry/src/commands/compile/collect_modules/static_require_transform.rs`:
- Around line 28-53: The iteration over require_aliases at the start of the loop
is nondeterministic because HashSet iteration order varies between runs, causing
the order of imports and temp-id assignments to change unpredictably. Sort the
require_aliases collection before iterating over it in the for loop to ensure a
consistent, deterministic ordering of the imports and temporary variable names
generated during each compilation run.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aebfa977-37fd-4d7e-87f8-acc9a32c063a
📒 Files selected for processing (6)
crates/perry/src/commands/compile/cjs_wrap/mod.rscrates/perry/src/commands/compile/collect_modules.rscrates/perry/src/commands/compile/collect_modules/create_require_transform.rscrates/perry/src/commands/compile/collect_modules/static_require_transform.rscrates/perry/tests/create_require_package.rscrates/perry/tests/module_import_forms.rs
💤 Files with no reviewable changes (1)
- crates/perry/src/commands/compile/collect_modules/create_require_transform.rs
Comment on lines
+16
to
+18
| if !require_is_shadowed_by_non_create_require(source, &require_aliases) { | ||
| require_aliases.insert("require".to_string()); | ||
| } |
There was a problem hiding this comment.
Shadow handling is not lexical, so some require(...) calls are rewritten incorrectly.
At Line 166, the current check is a file-global boolean. It can miss true local shadowing (e.g., function f(require) { ... }) and also over-suppress valid top-level rewrites when shadowing exists only in nested scopes. This breaks the “local require shadowing is respected” contract.
Suggested direction
Use scope-aware detection (token/AST-based) so each match is validated against lexical bindings at that position, instead of a single file-wide shadow flag.
Also applies to: 166-179
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@crates/perry/src/commands/compile/collect_modules/static_require_transform.rs`
around lines 16 - 18, The current implementation of
require_is_shadowed_by_non_create_require uses a file-global boolean check that
does not respect lexical scoping, causing it to miss true local shadowing (such
as require as a function parameter) and incorrectly suppress valid rewrites in
outer scopes. Refactor the shadowing detection logic to be scope-aware by
implementing token or AST-based analysis that checks whether require is shadowed
at the specific lexical position of each require call, rather than maintaining a
single file-wide shadow flag. This ensures that the condition checked in the if
statement at line 16-18 accurately validates shadowing based on local bindings
at the exact location where require is being rewritten.
proggeramlug
pushed a commit
that referenced
this pull request
Jun 19, 2026
Static-literal require(...) in user-compiled modules now lowers into the import graph instead of staying a kept-local let. Clears Next.js W6 (#5437): const uw = require('.../shared-cache-controls.external.js') resolves via the module getter, so new uw.SharedCacheControls(...) no longer throws 'undefined is not a constructor' (bundle error count 18 -> 0). Authored by JagritGumber (#5447).
Contributor
|
Landed on |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Adds a broader static
require(...)lowering pass for user-compiled modules. The oldcreateRequire-only transform is replaced with a static literal require transform that runs before SWC/HIR lowering and feeds eligible require edges into Perry's normal import graph.Supported forms include:
const mod = require("./local")const { value } = require("./local")require("allowed-package").memberconst req = createRequire(import.meta.url); req("allowed-package")createRequire as makeRequireThe transform deliberately leaves builtins on the existing native builtin path, ignores comments/strings, respects local
requireshadowing, and only lowers package specifiers already allowed byperry.compilePackages.Non-goals
require(expr)require.resolve(...).nodeaddon loadingcompilePackages/allow.compilePackagesTest plan
cargo fmt --checkcargo test -p perry static_require_transformcargo test -p perry --test create_require_packagecargo test -p perry --test module_import_forms create_require_package_specifier_resolves_to_compiled_namespacecargo test -p perry --test issue_5257_require_adopt_no_default_namespaceSummary by CodeRabbit
New Features
require()call detection and conversion.Bug Fixes
require()calls when accessing compiled packages in CommonJS code.Tests