fix(codegen): resolve aliased built-in class imports (e.g. AsyncLocalStorage) in new-lowering - #5556
Merged
Merged
Conversation
… `new`
A renamed named import of a node built-in constructor
(`import { AsyncLocalStorage as xQ5 } from "async_hooks"; new xQ5()`)
lowered `new` against the syntactic alias `xQ5`. The built-in
constructor arms in `lower_builtin_new` are keyed on the canonical
export name (`"AsyncLocalStorage"`), so the alias matched nothing and
construction fell through to the empty-object placeholder. The instance
then had no `.run`/`.getStore`/`.enterWith` methods, so
`xQ5().getStore()` threw `TypeError: getStore is not a function` — the
exact wall a minified CLI hit in context-storage plumbing.
Build a per-module alias → original-export-name map from `hir.imports`
(renamed named imports only) and thread it into the lowering context.
When `lower_builtin_new` returns None for the alias, retry once with the
recovered canonical name. The retry sits inside the existing
`!ctx.classes.contains_key(class_name)` guard, so a renamed import can
never shadow a real user-defined class.
Adds regression coverage: aliased `AsyncLocalStorage` run/getStore
round-trip, and an alias-vs-unaliased identity check.
📝 WalkthroughWalkthroughAdds an ChangesAliased Native Class Import Fix
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
🤖 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-codegen/src/lower_call/new.rs`:
- Around line 374-377: The lower_builtin_new call at line 375 uses the alias
name from imported_class_original_names without verifying it's actually a
builtin, which can cause non-builtin aliased imports that share a builtin export
name to be incorrectly dispatched to the builtin constructor path. Instead of
directly passing the original alias name to lower_builtin_new, first verify that
the original name is actually a builtin constructor (or use a source-qualified
lookup) before attempting the builtin path, otherwise fall through to handle it
as a regular imported constructor.
🪄 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: cbee77bc-7553-4404-8a88-513a69ec24d7
📒 Files selected for processing (9)
crates/perry-codegen/src/codegen/closure.rscrates/perry-codegen/src/codegen/entry.rscrates/perry-codegen/src/codegen/function.rscrates/perry-codegen/src/codegen/method.rscrates/perry-codegen/src/codegen/mod.rscrates/perry-codegen/src/codegen/opts.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/lower_call/new.rscrates/perry/tests/aliased_native_class_import.rs
Comment on lines
+374
to
+377
| if let Some(original) = ctx.imported_class_original_names.get(class_name).cloned() { | ||
| if original != class_name { | ||
| if let Some(val) = lower_builtin_new(ctx, &original, args)? { | ||
| return Ok(val); |
There was a problem hiding this comment.
Alias fallback should be source-qualified to avoid wrong constructor dispatch.
On Line 374, retrying by alias name alone can misroute non-builtin aliased imports that happen to share a builtin export name, causing new to bypass the real imported constructor path.
Suggested fix
- if let Some(original) = ctx.imported_class_original_names.get(class_name).cloned() {
- if original != class_name {
- if let Some(val) = lower_builtin_new(ctx, &original, args)? {
- return Ok(val);
- }
- }
- }
+ if let Some(original) = ctx.imported_class_original_names.get(class_name) {
+ if original != class_name {
+ let alias_is_builtin_import = ctx
+ .import_function_node_submodule
+ .get(class_name)
+ .is_some_and(|(_, exported_name)| exported_name == original);
+ if alias_is_builtin_import {
+ if let Some(val) = lower_builtin_new(ctx, original, args)? {
+ return Ok(val);
+ }
+ }
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if let Some(original) = ctx.imported_class_original_names.get(class_name).cloned() { | |
| if original != class_name { | |
| if let Some(val) = lower_builtin_new(ctx, &original, args)? { | |
| return Ok(val); | |
| if let Some(original) = ctx.imported_class_original_names.get(class_name) { | |
| if original != class_name { | |
| let alias_is_builtin_import = ctx | |
| .import_function_node_submodule | |
| .get(class_name) | |
| .is_some_and(|(_, exported_name)| exported_name == original); | |
| if alias_is_builtin_import { | |
| if let Some(val) = lower_builtin_new(ctx, original, args)? { | |
| return Ok(val); | |
| } | |
| } | |
| } | |
| } |
🤖 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-codegen/src/lower_call/new.rs` around lines 374 - 377, The
lower_builtin_new call at line 375 uses the alias name from
imported_class_original_names without verifying it's actually a builtin, which
can cause non-builtin aliased imports that share a builtin export name to be
incorrectly dispatched to the builtin constructor path. Instead of directly
passing the original alias name to lower_builtin_new, first verify that the
original name is actually a builtin constructor (or use a source-qualified
lookup) before attempting the builtin path, otherwise fall through to handle it
as a regular imported constructor.
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.
Problem
newon a built-in class imported under an alias falls through to a generic empty object, so its methods are missing:The runtime
AsyncLocalStorageimplementation (run/getStore/enterWith/exit/disable) was already correct — only the aliasednewwas broken. Un-aliasednew AsyncLocalStorage()worked. Also affectsAsyncResourceand any other codegen-arm built-in constructor used under an alias.Root cause
lower_builtin_new(crates/perry-codegen/src/lower_call/builtin.rs) matches the built-in constructor arm on the canonical class name ("AsyncLocalStorage"). Underimport { AsyncLocalStorage as ALS }, the construct site'sclass_nameis the alias"ALS", so the arm never fires andnew ALS()produces the generic empty-object placeholder — no methods installed.Fix
imported_class_original_namesmap (alias → original export name) fromhir.imports(renamed named imports only), incodegen/mod.rs, and thread it through the lowering context.new-lowering (lower_call/new.rs): whenlower_builtin_new(alias)returnsNone, retry once with the recovered canonical name. This sits inside the existing!ctx.classes.contains_key(class_name)guard, so a renamed import can never shadow a real user-defined class of the same local name.No runtime/stdlib changes.
Tests
crates/perry/tests/aliased_native_class_import.rs:aliased_async_local_storage_run_getstore_round_trip(store returned insiderun,undefinedoutside) andaliased_async_local_storage_matches_unaliased(alias path equals canonical path).cargo test -p perry-runtime --lib: 1072 passed.Scope note
The retry passes the original name to
lower_builtin_new; its source-gated arms (Client/Pool/Database/Redis/MongoClient/Decimal) look up by import source and won't match the recovered name — so aliased imports of those specific ambiguous names remain routed by their existing source-gated path (pre-existing, by design). Un-gated built-ins (AsyncLocalStorage, AsyncResource, …) are fully covered.Summary by CodeRabbit
Bug Fixes
import { AsyncLocalStorage as xQ5 }) to properly resolve constructor behavior at runtime, preventing method-not-found errors for renamed built-in Node.js classes.Tests