fix(compile/hir): five Vercel CLI corpus blockers — CJS comma-first requires, body-local enums, D005/T002 false positives, .node diagnostic - #6885
Merged
Conversation
proggeramlug
force-pushed
the
fix/vercel-corpus-compat
branch
from
July 27, 2026 03:44
cf2db49 to
a791d17
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (18)
🚧 Files skipped from review as they are similar to previous changes (16)
📝 WalkthroughWalkthroughThe changes add function-body enum lowering, improve CJS wrapper handling, expand dependency compatibility checks, reject native addon binaries earlier, serialize environment-sensitive tests, and update release metadata. ChangesCompatibility and release updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FunctionBody
participant LoweringContext
participant lower_module_full
participant Module
FunctionBody->>LoweringContext: queue lowered enum
lower_module_full->>LoweringContext: drain pending_body_enums
lower_module_full->>Module: register non-duplicate enums
Module-->>FunctionBody: resolve enum members during code generation
Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
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/cjs_wrap/extract_requires.rs (1)
75-118: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winComment between
require(...)and the continuation comma defeats the new lookahead.The whitespace-skip loop at Lines 112-115 only skips literal whitespace bytes. A trailing line comment (or block comment) between the closing paren and the continuation comma — e.g.
require('./compile') // note\n , resolve = require(...)— stops the scan at the comment text, so the comma is never found, the declarator is (incorrectly) treated as a standalone alias, gets blanked, and the surviving continuation dangles at statement position: the exact TS1109 failure shape this PR is fixing, just reached via a comment instead of a member access.
cjs_wrap::detect::strip_comments_and_strings(already used analogously indeps.rs's newdynamic_import_lines) can mask this out cheaply.🐛 Proposed fix: mask comments before the comma lookahead
let bytes = source.as_bytes(); + // Mask comments/strings so a trailing comment between the require call + // and a comma-first continuation doesn't hide the comma from the + // lookahead below (same failure shape as the unmasked comma case). + let masked = super::detect::strip_comments_and_strings(source); + let scan_bytes: &[u8] = if masked.len() == bytes.len() { + masked.as_bytes() + } else { + bytes + }; let mut seen = Vec::new(); let mut out = Vec::new(); for cap in re.captures_iter(source) { if let (Some(alias), Some(spec), Some(whole)) = (cap.get(1), cap.get(2), cap.get(0)) { ... let mut p = whole.end(); - while p < bytes.len() && (bytes[p] as char).is_whitespace() { + while p < scan_bytes.len() && (scan_bytes[p] as char).is_whitespace() { p += 1; } - if p < bytes.len() && bytes[p] == b',' { + if p < scan_bytes.len() && scan_bytes[p] == b',' { continue; }🤖 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/cjs_wrap/extract_requires.rs` around lines 75 - 118, Update the comma-continuation lookahead in extract_require_aliases_with_ranges to ignore comments between the matched require expression and a following comma, using cjs_wrap::detect::strip_comments_and_strings or the existing analogous masking approach. Ensure whitespace and both line/block comments are skipped before checking for the comma, so commented multi-line declarators are treated as whole declarations and not registered as aliases.
🧹 Nitpick comments (1)
crates/perry/src/commands/compile/collect_modules/native_addon.rs (1)
142-159: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for the direct
.nodeguard.The supplied tests cover the
compilePackagesrejection path, but not this helper’s intended sidecar-package path. Add a fixture with a regular resolved package containingpackage.jsonandaddon.node, and assert that the error includes the package name and native-addon guidance; otherwise this path can regress to the old UTF-8 failure unnoticed.🤖 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/native_addon.rs` around lines 142 - 159, Add a regression fixture for refuse_node_addon_binary using a resolved package directory containing package.json and addon.node, then invoke the helper with the addon path and assert the failure includes the package name and native-addon guidance. Keep the existing compilePackages rejection coverage unchanged and ensure the test exercises package-name resolution rather than a direct standalone path.
🤖 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-hir/src/lower_decl/body_stmt.rs`:
- Around line 1984-2012: Preserve lexical identity for body-local enums instead
of treating every same-named declaration as global: update enum resolution and
registration around the body-local lowering logic in
crates/perry-hir/src/lower_decl/body_stmt.rs (anchor lines 1984-2012), retain
each identity without name-only deduplication in
crates/perry-hir/src/lower/lower_module_fn.rs (sibling lines 1188-1198), add
separate same-named local enums with different values and a local enum shadowing
a module enum in test-files/test_gap_enum_in_function_body.ts (sibling lines
63-74), and remove the obsolete same-name limitation from
changelog.d/6885-vercel-cli-corpus-compat.md (sibling lines 21-28).
In `@crates/perry/src/test_env_lock.rs`:
- Around line 18-26: The env_lock function currently recovers poisoned mutexes
even though callers may leave PATH modified during unwinding. Make environment
restoration in the optimized library tests panic-safe so cleanup runs during
unwinding, or change env_lock to fail closed on poisoning after verifying every
caller restores the environment. Ensure a panic cannot expose subsequent tests
to a modified or deleted PATH.
---
Outside diff comments:
In `@crates/perry/src/commands/compile/cjs_wrap/extract_requires.rs`:
- Around line 75-118: Update the comma-continuation lookahead in
extract_require_aliases_with_ranges to ignore comments between the matched
require expression and a following comma, using
cjs_wrap::detect::strip_comments_and_strings or the existing analogous masking
approach. Ensure whitespace and both line/block comments are skipped before
checking for the comma, so commented multi-line declarators are treated as whole
declarations and not registered as aliases.
---
Nitpick comments:
In `@crates/perry/src/commands/compile/collect_modules/native_addon.rs`:
- Around line 142-159: Add a regression fixture for refuse_node_addon_binary
using a resolved package directory containing package.json and addon.node, then
invoke the helper with the addon path and assert the failure includes the
package name and native-addon guidance. Keep the existing compilePackages
rejection coverage unchanged and ensure the test exercises package-name
resolution rather than a direct standalone path.
🪄 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: 8aac83e7-1ae4-41bb-963f-6e13a272207d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
CLAUDE.mdCargo.tomlchangelog.d/6885-vercel-cli-corpus-compat.mdcrates/perry-hir/src/lower/context.rscrates/perry-hir/src/lower/lower_module_fn.rscrates/perry-hir/src/lower/lowering_context.rscrates/perry-hir/src/lower_decl/body_stmt.rscrates/perry/src/commands/compile.rscrates/perry/src/commands/compile/cjs_wrap/extract_requires.rscrates/perry/src/commands/compile/cjs_wrap/mod.rscrates/perry/src/commands/compile/collect_modules.rscrates/perry/src/commands/compile/collect_modules/native_addon.rscrates/perry/src/commands/compile/optimized_libs/tests.rscrates/perry/src/commands/deps.rscrates/perry/src/commands/install/lifecycle.rscrates/perry/src/main.rscrates/perry/src/test_env_lock.rstest-files/test_gap_enum_in_function_body.ts
…equires, body-local enums, D005/T002 false positives, .node diagnostic Found by compiling the Vercel CLI (real-apps/vercel, 1137 TS files / 168k LOC) end to end. - cjs_wrap: a comma-first `var a = require(x)\n , b = require(y);` list matched only declarator #0 (the alias regex ends at `(?m)$`), and blanking that range left `, b = ...` at statement position -> TS1109. Same failure shape as #845, reached via a comma instead of a member access. Multi-declarator statements are now skipped entirely, falling back to the IIFE-bound `require`. Hit by ajv 6.x. - hir: `enum` inside a function body is valid TS but bailed. Member access is materialized from `ctx.lookup_enum`, but codegen resolves `Expr::EnumMember` against `Module::enums`, which body-local decls could not reach. Added `LoweringContext::pending_body_enums`, drained in `lower_module_full`. A same-named body-local enum with different members still diagnoses rather than silently reusing the first registration. - deps/D005: the check was a per-line `!line.contains("import('")` test, so it keyed off formatting, not the argument — a prettier-wrapped multi-line `await import('./x')` was flagged while the same call on one line was not. It also scanned `import(` inside string literals. Now resolved across newlines on a comment/string-masked copy. - deps/T002: honor `types`/`typings` in package.json and a `types` condition in `exports`, not just three hardcoded paths. date-fns, tldts and @inquirer/* were all reported as untyped. - collect_modules: a `.node` addon reached as a module reported "stream did not contain valid UTF-8". The existing refusal only covers packages that resolved to a compilePackages root, missing the napi-rs sidecar layout. The read is now guarded directly. Corpus effect: `perry check --check-deps` goes from 4 errors / 20 T002 warnings to 0 / 5, and the 5 remaining are genuine. `perry compile` now reaches the native-addon boundary with ajv unpatched and the enum unhoisted. Tests: 10 new unit tests (cjs_wrap + deps) and test-files/test_gap_enum_in_function_body.ts. perry bin suite 745/745 green; the one perry-hir failure (logical_property_assignment_short_circuits_the_ store_4586) reproduces on origin/main with these changes stashed.
proggeramlug
force-pushed
the
fix/vercel-corpus-compat
branch
from
July 27, 2026 03:57
a791d17 to
57d935d
Compare
Contributor
Author
|
Thanks — went through both.
Body-local enum lexical identity — real, but deferring. Agreed this is incomplete: two functions declaring Deferring rather than fixing here, for two reasons:
Happy to open a follow-up issue with that analysis if useful. The changelog fragment documents the limitation explicitly so it isn't silently inherited. |
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
1 participant
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.
Found by compiling the Vercel CLI end to end (
real-apps/vercel, 1137 TS files / 168k LOC, 23 workspace packages). Each fix is the thing that was actually blocking the next step, in the order it was hit.Fixes
1. CJS→ESM wrap: comma-first
requiredeclarator lists (the blocker)ajv 6.x uses the pre-ES6 style:
The alias regex ends at
(?m)$, so only declarator #0 matched. Blanking that range left, resolve = …at statement position →TS1109 (Expression expected)— the same failure shape as the.EventEmitter;case in #845, reached via a comma instead of a member access.Multi-declarator statements are now skipped entirely: nothing is blanked, the body keeps the valid original, and the IIFE-bound
requireresolves each specifier at runtime — the same fallback the wrap already takes when it refuses an adoption.I deliberately did not "blank the whole
VariableDeclaration". Adoption is decided per-alias, so blanking only the adopted declarators of a list reintroduces the identical dangling-comma bug. The single-line formvar a = require('x'), b = 42;never matched to begin with (\s*(?:;|$)rejects the,), so this only affects the multi-line style.2.
enuminside a function body now compilesValid TypeScript that bailed with "declare it at module scope". Member access is materialized from
ctx.lookup_enum, but codegen resolvesExpr::EnumMemberagainstModule::enums, which body-local declarations had no route to — registering alone yieldsenum member Section.GEM not found in enums table. AddedLoweringContext::pending_body_enums, drained inlower_module_fullonce every function is lowered.Two same-named body-local enums with different members still raise a diagnostic rather than silently reusing the first registration.
3.
D005no longer fires on static specifiersThe check was a per-line
!line.contains("import('")test, so it keyed off formatting rather than the argument:It also scanned
import(inside string literals — flagging a loader script built as a template literal and written to disk for a child Node process. The specifier is now resolved across newlines on a comment/string-masked copy. The argument is read from the original source, because the masker blanks string delimiters too.4.
T002honors the canonicaltypes/typingsfieldThe probe checked three hardcoded paths (
index.d.ts,dist/index.d.ts,types/) and ignored package.json entirely, sodate-fns(./typings.d.ts),tldts(dist/types/index.d.ts) and@inquirer/*(./dist/cjs/types/index.d.ts) were all reported as untyped. Now checkstypes/typings(incl. the extensionless form), atypescondition anywhere inexports, the legacy layouts, then a bounded.d.tsscan.5. A
.nodeaddon reached as a module reports what it isBoth module read paths call
fs::read_to_string, so a compiled N-API addon surfaced asstream did not contain valid UTF-8.refuse_compile_package_native_addononly covers addons whose package root resolved to acompilePackagesentry, which misses the napi-rs sidecar layout (@napi-rs/keyring→@napi-rs/keyring-darwin-arm64, containing nothing but the.nodefile and a package.json). The read is now guarded directly.Corpus effect
All 5 remaining
T002s are genuine — four are exactly the packages the Vercel repo hand-writes ambient declarations for inpackages/cli/types/.perry compilenow reaches the native-addon boundary with ajv unpatched and the enum unhoisted, and reports it clearly instead of a UTF-8 decode failure.Tests
cjs_wrap+deps), including the multi-line/single-line D005 pair and the fourtypeslayouts.test-files/test_gap_enum_in_function_body.ts— 5 cases incl. numeric reverse mapping and nesting. Note this auto-skips inrun_parity_tests.sh(Node's strip-only mode cannot execute enums at all, same as the existingtest_gap_4510_enum_forward_ref.ts); it is covered bycompile-smoke.perrybin suite 751/751 green, stable across 3 consecutive runs.One internal fix included
install::lifecycle'srun_lifecycle_executes_scriptreads process-globalPATH(viaaugment_path) to resolvesh, whileoptimized_libs::testsswapsPATHfor a fakesh-less directory behind a module-private mutex. The lock was never shared, so the two could race. Latent before this branch — the added tests shifted scheduling enough to surface it asfailed to spawn shell … No such file or directory. The guard moved tocrate::test_env_lockand both sides take it.Notes for review
mod cjs_wrapandmod detectwere widened topub(crate)sodeps.rscan reusedetect's comment/string masker rather than duplicate a subtle scanner. Happy to swap for a local copy if you'd rather not expose those.cargo fmt --all --checkreports one hunk incollect_modules.rs(a comment re-indent around line 766) that is pre-existing onmainunder rustfmt 1.9.0 — verified by running fmt with the pristine file swapped in. Left untouched so this PR stays scoped.perry-hir'slogical_property_assignment_short_circuits_the_store_4586(o.x ??= 2) fails, and also fails onmainwith this branch stashed. Unrelated and pre-existing.6875-…; rename to the real PR number if it differs.Summary by CodeRabbit
require()declarator patterns.import(...)detection to reduce false positives and handle multi-line specifiers..nodeadd-ons during compilation.types/typingsandexportsresolution.