fix(compile): keep an optional try-wrapped require as a runtime require (#6873)

[proggeramlug]

Fixes #6873.

Problem

The "optional generated/embedded asset" idiom aborted the build:

export const X = 1;
let BUNDLE = null;
try { BUNDLE = require("./stdlib-bundle").STDLIB; } catch {}

Error: Could not resolve namespace import `import * as ... from "./stdlib-bundle"` ...

transform_static_literal_requires hoists literal requires to a top-level
import * as <tmp>, discarding both the runtime guard and the catch, and
#629 makes an unresolvable namespace import a hard error.

The #629 rationale — an empty namespace silently no-ops and the user never
learns why — does not hold here. The author explicitly said this may not
exist, and Node/Bun either never evaluate the call or swallow the throw.
Worse, the error's three suggested remedies (switch to named imports / remove
the import / add to perry-stdlib) are all wrong for this shape.

Fix

A specifier is optional when every one of its call sites is inside a try.
An optional specifier that does not resolve on disk is no longer hoisted — the
call stays a runtime require, which is exactly what bare package specifiers
already do today, and reproduces Node: it throws, the catch fires, the binding
keeps its prior value.

Deliberately narrow:

• optional AND resolvable still hoists, so a module that is present is
  compiled in and loads. (Perry supports require in ESM-ish sources as a
  CJS-interop extension; plain node-ESM cannot require at all.)
• one unguarded call site anywhere makes the specifier load-bearing again
  and it keeps being hoisted.
• try is matched as a keyword — retry { does not qualify.

Detection runs over the comment/string-masked source, so a miss either way is
safe: a false negative keeps today's hoisting, and a false positive only
downgrades an unresolvable module to a runtime require — which is Node's
behavior anyway.

Verification

Missing-module case is byte-identical to node --experimental-strip-types:

try { miss = require("./absent-generated").DATA; } catch {}
console.log(miss === null);     // node: true   perry: true  (was: build error)

Four new unit tests in static_require_transform: missing optional require is
left alone, resolvable optional require still hoists (via a tempfile dir),
a specifier also used outside try stays hoisted, and retry { does not open
a try block. All 11 tests in that module pass.

End to end: with src/stdlib-bundle.ts removed (its real gitignored state in a
fresh clone), the Milo compiler now compiles cleanly — exit 0, no error and no
unresolved-import warning, because nothing is synthesized at all.

How this was found

Compiling the Milo compiler (https://github.com/milo-language/milo). Its
src/stdlib-bundle.ts is gitignored and produced by a build step
(bun run scripts/bundle-stdlib.ts), so a fresh clone could not be compiled
with Perry at all — the first thing anyone hits, and it reads like "Perry
cannot build this project" rather than "you skipped a build step". The same
pattern appears twice more in that codebase (src/resolver.ts,
src/fmtbin.ts).

Summary by CodeRabbit

• Bug Fixes
  • Fixed builds for optional require() calls inside try/catch when the module specifier can’t be resolved.
  • Optional relative/absolute specifiers are no longer hoisted when they don’t resolve on disk, leaving them as runtime require() calls.
  • Resolvable modules still get optimized as before.
  • Improved detection of lexically matching try { ... } regions, including the retry { ... } non-matching edge case.
• Tests
  • Extended coverage for missing vs. resolvable optional modules and try detection edge cases.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 9c41265d-f899-4cc6-b041-d83ff021602b

📥 Commits

Reviewing files that changed from the base of the PR and between 6f301cc and badd1a6.

📒 Files selected for processing (1)

• crates/perry/src/commands/compile/collect_modules/static_require_transform.rs

🚧 Files skipped from review as they are similar to previous changes (1)

• crates/perry/src/commands/compile/collect_modules/static_require_transform.rs

---

📝 Walkthrough

Walkthrough

The compiler detects fully try-wrapped static require() calls, checks relative module resolvability from the source directory, and preserves unresolved optional calls at runtime. Resolvable calls remain hoisted, while mixed guarded and unguarded usage remains load-bearing.

Changes

Optional require compilation

Layer / File(s)	Summary
Module directory context crates/perry/src/commands/compile/collect_modules.rs, crates/perry/src/commands/compile/collect_modules/static_require_transform.rs	The module collector passes the source directory to static require transformation.
Optional require classification and resolution crates/perry/src/commands/compile/collect_modules/static_require_transform.rs	Requires are classified by try-block coverage; unresolved relative or absolute optional modules remain runtime calls, while resolvable modules continue to hoist.
Regression coverage and changelog crates/perry/src/commands/compile/collect_modules/static_require_transform.rs, changelog.d/6873-optional-require-try.md	Tests cover resolution, mixed call sites, keyword matching, comment and string handling, and updated helper calls; the changelog documents the compiler fix.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ModuleCollector
  participant StaticRequireTransformer
  participant Filesystem
  ModuleCollector->>StaticRequireTransformer: pass source, compile packages, and module_dir
  StaticRequireTransformer->>StaticRequireTransformer: classify require call sites inside try blocks
  StaticRequireTransformer->>Filesystem: check relative or absolute specifier resolution
  Filesystem-->>StaticRequireTransformer: resolved or unresolved module
  StaticRequireTransformer-->>ModuleCollector: hoisted import or runtime require

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main fix: optional try-wrapped require calls stay runtime requires.
Description check	✅ Passed	The description covers the problem, fix, and verification, though it does not follow the template headings exactly.
Linked Issues check	✅ Passed	The code matches #6873 by preserving guarded optional requires at runtime while keeping resolvable and unguarded cases unchanged.
Out of Scope Changes check	✅ Passed	The changes stay focused on the compiler fix and its changelog entry, with no clear unrelated edits.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/6873-optional-require-try

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

crates/perry/src/commands/compile/collect_modules/static_require_transform.rs (1)

227-248: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

EXTENSIONS omits .json.

An extensionless optional require that resolves to a sibling .json file (e.g. require("./config") → config.json) won't be detected as resolvable, since EXTENSIONS only lists TS/JS variants. The literal-extension case (require("./config.json")) is already covered by the base.is_file() check above, so impact is narrow, but worth aligning with the real resolver's extension set if it includes JSON.

🤖 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 227 - 248, The relative_specifier_resolves function must recognize
extensionless sibling and index JSON files when matching the resolver’s
supported extensions. Add json to its EXTENSIONS set while preserving the
existing base.is_file check and non-relative specifier behavior.

🤖 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 26-44: Update the optional classification loop around
literal_require_call_re to iterate the raw source only after filtering each
match with masked_source, matching the hoisting loop’s blank-span check. Ignore
captures whose call span is masked out as a comment or string, while preserving
classification for real require calls and the existing is_inside_try_block
behavior; add a regression test covering a commented specifier before a
try-guarded call if tests are available.

---

Nitpick comments:
In
`@crates/perry/src/commands/compile/collect_modules/static_require_transform.rs`:
- Around line 227-248: The relative_specifier_resolves function must recognize
extensionless sibling and index JSON files when matching the resolver’s
supported extensions. Add json to its EXTENSIONS set while preserving the
existing base.is_file check and non-relative specifier behavior.

🪄 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: 1b72f45a-880e-4786-ba70-21e263cb9ede

📥 Commits

Reviewing files that changed from the base of the PR and between c3dc207 and 6f301cc.

📒 Files selected for processing (3)

• changelog.d/6873-optional-require-try.md
• crates/perry/src/commands/compile/collect_modules.rs
• crates/perry/src/commands/compile/collect_modules/static_require_transform.rs

[proggeramlug]

Good catch — valid, and fixed in badd1a6.

The classification loop iterated raw-source captures without the masked-span filter the hoisting loop applies, so a require("./x") appearing only in a comment or string counted as a real call site. Sitting outside a try, it flipped the specifier to mandatory and the hoist reintroduced the exact hard error this PR removes.

Applied the suggested filter, and added comment_and_string_mentions_do_not_defeat_optional_classification covering both a commented and a quoted mention preceding the real try-guarded call. Confirmed it fails without the filter and passes with it, so it genuinely locks the behavior in rather than just passing.