Apply automatic clippy fixes in HIR#5423
Merged
Merged
Conversation
ℹ️ 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 due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthroughA broad cleanup of Changesperry-hir: Import cleanup, re-export narrowing, and match-guard refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/perry-hir/src/lower/module_decl.rs (1)
1957-1961:⚠️ Potential issue | 🔴 CriticalAdd
declarenamespace guard tonested_namespace_nameor guard the unguarded match arm at line 2304.The function
nested_namespace_name(line 1956) documents that it should returnNonefor bodilessdeclaremodules, but it only checks the body—not thedeclareflag itself. Meanwhile, at line 2304, an unguardedast::Decl::TsModule(ts_module) =>arm callslower_nested_namespacedirectly, bypassing theif !ts_module.declareguards present in all other nested namespace callsites (lines 2102, 2111, 2172). If adeclare namespacehas a body, the function could incorrectly process it as a runtime namespace.Add an explicit check:
Suggested fix
fn nested_namespace_name(ts_module: &ast::TsModuleDecl) -> Option<String> { + if ts_module.declare { + return None; + } ts_module.body.as_ref()?; match &ts_module.id { ast::TsModuleName::Ident(ident) => Some(ident.sym.to_string()), ast::TsModuleName::Str(_) => None, } }🤖 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-hir/src/lower/module_decl.rs` around lines 1957 - 1961, The function `nested_namespace_name` checks only whether a namespace has a body but does not verify the `declare` flag, yet the documentation states it should return None for bodiless declare modules. Additionally, the unguarded match arm for `ast::Decl::TsModule` at line 2304 calls `lower_nested_namespace` without checking the `declare` flag, unlike all other callsites (lines 2102, 2111, 2172) which have `if !ts_module.declare` guards. Add an explicit check for the `declare` flag in the `nested_namespace_name` function or add an `if !ts_module.declare` guard at the unguarded match arm at line 2304 to prevent declare namespaces from being incorrectly processed as runtime namespaces.
🤖 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.
Outside diff comments:
In `@crates/perry-hir/src/lower/module_decl.rs`:
- Around line 1957-1961: The function `nested_namespace_name` checks only
whether a namespace has a body but does not verify the `declare` flag, yet the
documentation states it should return None for bodiless declare modules.
Additionally, the unguarded match arm for `ast::Decl::TsModule` at line 2304
calls `lower_nested_namespace` without checking the `declare` flag, unlike all
other callsites (lines 2102, 2111, 2172) which have `if !ts_module.declare`
guards. Add an explicit check for the `declare` flag in the
`nested_namespace_name` function or add an `if !ts_module.declare` guard at the
unguarded match arm at line 2304 to prevent declare namespaces from being
incorrectly processed as runtime namespaces.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c5cbe22-1029-4176-8a80-3c85b1549443
📒 Files selected for processing (47)
crates/perry-hir/src/analysis.rscrates/perry-hir/src/capability.rscrates/perry-hir/src/destructuring/helpers.rscrates/perry-hir/src/destructuring/mod.rscrates/perry-hir/src/destructuring/var_decl.rscrates/perry-hir/src/egress.rscrates/perry-hir/src/js_transform/cross_module_natives.rscrates/perry-hir/src/js_transform/imports.rscrates/perry-hir/src/js_transform/local_natives.rscrates/perry-hir/src/js_transform/mod.rscrates/perry-hir/src/lower/eval_super_scan.rscrates/perry-hir/src/lower/expr_assign.rscrates/perry-hir/src/lower/expr_call/array_only_methods.rscrates/perry-hir/src/lower/expr_call/globals.rscrates/perry-hir/src/lower/expr_call/imported_array_methods.rscrates/perry-hir/src/lower/expr_call/inline_array_methods.rscrates/perry-hir/src/lower/expr_call/local_array_methods.rscrates/perry-hir/src/lower/expr_call/mod.rscrates/perry-hir/src/lower/expr_call/module_class_static.rscrates/perry-hir/src/lower/expr_call/module_static.rscrates/perry-hir/src/lower/expr_call/native_module.rscrates/perry-hir/src/lower/expr_call/nested_namespace.rscrates/perry-hir/src/lower/expr_call/regex_string.rscrates/perry-hir/src/lower/expr_call/static_and_instance.rscrates/perry-hir/src/lower/expr_call/textencoder.rscrates/perry-hir/src/lower/expr_call/url_date_instance.rscrates/perry-hir/src/lower/expr_call/wasm_exports.rscrates/perry-hir/src/lower/expr_member.rscrates/perry-hir/src/lower/expr_new.rscrates/perry-hir/src/lower/lower_expr.rscrates/perry-hir/src/lower/mod.rscrates/perry-hir/src/lower/module_decl.rscrates/perry-hir/src/lower/stmt.rscrates/perry-hir/src/lower_decl/block.rscrates/perry-hir/src/lower_decl/body_stmt.rscrates/perry-hir/src/lower_decl/class_captures.rscrates/perry-hir/src/lower_decl/class_decl.rscrates/perry-hir/src/lower_decl/class_members.rscrates/perry-hir/src/lower_decl/class_validation.rscrates/perry-hir/src/lower_decl/enum_decl.rscrates/perry-hir/src/lower_decl/fn_decl.rscrates/perry-hir/src/lower_decl/helpers.rscrates/perry-hir/src/lower_decl/interface_decl.rscrates/perry-hir/src/lower_decl/mod.rscrates/perry-hir/src/lower_decl/private_members.rscrates/perry-hir/src/lower_decl/type_alias.rscrates/perry-hir/src/monomorph/mod.rs
💤 Files with no reviewable changes (2)
- crates/perry-hir/src/lower_decl/mod.rs
- crates/perry-hir/src/js_transform/mod.rs
- Re-add mangle_type re-export (gated #[cfg(test)]) consumed by monomorph tests - Revert clippy collapsible_if/match-guard rewrite in egress check_url/check_host that changed semantics: an allowlisted literal now incorrectly fell through to the dynamic-host violation arm Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…fixes # Conflicts: # crates/perry-hir/src/destructuring/mod.rs # crates/perry-hir/src/lower/expr_call/array_only_methods.rs
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/destructuring/var_decl.rs`:
- Around line 1907-1937: The PutValueSet pattern match transformation is
extracting and storing the value in a temporary variable before target/key are
evaluated, which reverses the normal assignment evaluation order. Reorder the
temporary variable assignments and result statements so that target and key
expressions are evaluated first, then the value is extracted into the temporary,
ensuring the original left-to-right evaluation semantics are preserved. This
means creating temporaries for target/key evaluation before extracting the value
into the __nx_member_init temporary.
- Around line 1915-1919: The issue is that the compiler-generated temporary
local variable uses a hardcoded constant name "__nx_member_init" which can
collide with user-defined bindings in the same scope. Instead of using the fixed
string for the name field in the Stmt::Let, generate a unique synthetic name for
this temporary variable. The tmp_id returned from ctx.define_local should be
used to derive a unique name (such as prefixing with an underscore and including
the ID) to avoid any potential collisions while keeping the variable
distinguishable in debug output.
🪄 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: 538f66f6-f9fb-4c50-82f8-71fee8e64323
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
crates/perry-hir/src/destructuring/helpers.rscrates/perry-hir/src/destructuring/mod.rscrates/perry-hir/src/destructuring/var_decl.rscrates/perry-hir/src/lower/expr_call/array_only_methods.rscrates/perry-hir/src/lower/expr_call/globals.rscrates/perry-hir/src/lower/expr_call/local_array_methods.rscrates/perry-hir/src/lower/expr_call/native_module.rscrates/perry-hir/src/lower/expr_call/regex_string.rscrates/perry-hir/src/lower/expr_member.rscrates/perry-hir/src/lower/expr_new.rscrates/perry-hir/src/lower/lower_expr.rscrates/perry-hir/src/lower/module_decl.rscrates/perry-hir/src/lower_decl/block.rscrates/perry-hir/src/lower_decl/body_stmt.rscrates/perry-hir/src/lower_decl/class_decl.rscrates/perry-hir/src/lower_decl/class_members.rscrates/perry-hir/src/lower_decl/fn_decl.rscrates/perry-hir/src/lower_decl/private_members.rs
✅ Files skipped from review due to trivial changes (5)
- crates/perry-hir/src/lower_decl/private_members.rs
- crates/perry-hir/src/lower_decl/fn_decl.rs
- crates/perry-hir/src/destructuring/mod.rs
- crates/perry-hir/src/lower/lower_expr.rs
- crates/perry-hir/src/lower_decl/body_stmt.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- crates/perry-hir/src/destructuring/helpers.rs
- crates/perry-hir/src/lower/expr_call/regex_string.rs
- crates/perry-hir/src/lower/expr_member.rs
- crates/perry-hir/src/lower/expr_call/array_only_methods.rs
- crates/perry-hir/src/lower_decl/block.rs
- crates/perry-hir/src/lower_decl/class_decl.rs
- crates/perry-hir/src/lower/expr_new.rs
- crates/perry-hir/src/lower_decl/class_members.rs
- crates/perry-hir/src/lower/expr_call/globals.rs
- crates/perry-hir/src/lower/module_decl.rs
- crates/perry-hir/src/lower/expr_call/native_module.rs
- crates/perry-hir/src/lower/expr_call/local_array_methods.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 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/destructuring/var_decl.rs`:
- Around line 1907-1937: The PutValueSet pattern match transformation is
extracting and storing the value in a temporary variable before target/key are
evaluated, which reverses the normal assignment evaluation order. Reorder the
temporary variable assignments and result statements so that target and key
expressions are evaluated first, then the value is extracted into the temporary,
ensuring the original left-to-right evaluation semantics are preserved. This
means creating temporaries for target/key evaluation before extracting the value
into the __nx_member_init temporary.
- Around line 1915-1919: The issue is that the compiler-generated temporary
local variable uses a hardcoded constant name "__nx_member_init" which can
collide with user-defined bindings in the same scope. Instead of using the fixed
string for the name field in the Stmt::Let, generate a unique synthetic name for
this temporary variable. The tmp_id returned from ctx.define_local should be
used to derive a unique name (such as prefixing with an underscore and including
the ID) to avoid any potential collisions while keeping the variable
distinguishable in debug output.
🪄 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: 538f66f6-f9fb-4c50-82f8-71fee8e64323
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
crates/perry-hir/src/destructuring/helpers.rscrates/perry-hir/src/destructuring/mod.rscrates/perry-hir/src/destructuring/var_decl.rscrates/perry-hir/src/lower/expr_call/array_only_methods.rscrates/perry-hir/src/lower/expr_call/globals.rscrates/perry-hir/src/lower/expr_call/local_array_methods.rscrates/perry-hir/src/lower/expr_call/native_module.rscrates/perry-hir/src/lower/expr_call/regex_string.rscrates/perry-hir/src/lower/expr_member.rscrates/perry-hir/src/lower/expr_new.rscrates/perry-hir/src/lower/lower_expr.rscrates/perry-hir/src/lower/module_decl.rscrates/perry-hir/src/lower_decl/block.rscrates/perry-hir/src/lower_decl/body_stmt.rscrates/perry-hir/src/lower_decl/class_decl.rscrates/perry-hir/src/lower_decl/class_members.rscrates/perry-hir/src/lower_decl/fn_decl.rscrates/perry-hir/src/lower_decl/private_members.rs
✅ Files skipped from review due to trivial changes (5)
- crates/perry-hir/src/lower_decl/private_members.rs
- crates/perry-hir/src/lower_decl/fn_decl.rs
- crates/perry-hir/src/destructuring/mod.rs
- crates/perry-hir/src/lower/lower_expr.rs
- crates/perry-hir/src/lower_decl/body_stmt.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- crates/perry-hir/src/destructuring/helpers.rs
- crates/perry-hir/src/lower/expr_call/regex_string.rs
- crates/perry-hir/src/lower/expr_member.rs
- crates/perry-hir/src/lower/expr_call/array_only_methods.rs
- crates/perry-hir/src/lower_decl/block.rs
- crates/perry-hir/src/lower_decl/class_decl.rs
- crates/perry-hir/src/lower/expr_new.rs
- crates/perry-hir/src/lower_decl/class_members.rs
- crates/perry-hir/src/lower/expr_call/globals.rs
- crates/perry-hir/src/lower/module_decl.rs
- crates/perry-hir/src/lower/expr_call/native_module.rs
- crates/perry-hir/src/lower/expr_call/local_array_methods.rs
🛑 Comments failed to post (2)
crates/perry-hir/src/destructuring/var_decl.rs (2)
1907-1937:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve assignment evaluation order in the PutValueSet split.
This transformation evaluates RHS
valuebeforetarget/key, which can change runtime behavior for side-effectful LHS/RHS expressions compared to normal assignment semantics.Suggested direction
- result.push(Stmt::Let { - id: tmp_id, - name: "__nx_member_init".to_string(), - ty: Type::Any, - mutable: false, - init: Some(*value), - }); - result.push(Stmt::Expr(Expr::PutValueSet { - target, - key, - value: Box::new(Expr::LocalGet(tmp_id)), - receiver, - strict, - })); + // Keep original assignment ordering: + // 1) evaluate target/key reference parts first + // 2) evaluate value + // 3) perform put + bind result + // Use temporaries for target/key if needed, then value. + // (Implementation can reuse an existing helper that linearizes + // assignment side effects into ordered temps.)🤖 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-hir/src/destructuring/var_decl.rs` around lines 1907 - 1937, The PutValueSet pattern match transformation is extracting and storing the value in a temporary variable before target/key are evaluated, which reverses the normal assignment evaluation order. Reorder the temporary variable assignments and result statements so that target and key expressions are evaluated first, then the value is extracted into the temporary, ensuring the original left-to-right evaluation semantics are preserved. This means creating temporaries for target/key evaluation before extracting the value into the __nx_member_init temporary.
1915-1919:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid fixed synthetic local name for compiler-generated temp.
Using a constant local name (
"__nx_member_init") risks collisions with user bindings in the same scope.Proposed fix
- let tmp_id = ctx.define_local("__nx_member_init".to_string(), Type::Any); + let tmp_name = format!("__nx_member_init_{}", ctx.next_synthetic_id()); + let tmp_id = ctx.define_local(tmp_name.clone(), Type::Any); result.push(Stmt::Let { id: tmp_id, - name: "__nx_member_init".to_string(), + name: tmp_name, ty: Type::Any, mutable: false, init: Some(*value), });🤖 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-hir/src/destructuring/var_decl.rs` around lines 1915 - 1919, The issue is that the compiler-generated temporary local variable uses a hardcoded constant name "__nx_member_init" which can collide with user-defined bindings in the same scope. Instead of using the fixed string for the name field in the Stmt::Let, generate a unique synthetic name for this temporary variable. The tmp_id returned from ctx.define_local should be used to derive a unique name (such as prefixing with an underscore and including the ID) to avoid any potential collisions while keeping the variable distinguishable in debug output.
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
cargo clippy --fixsuggestions incrates/perry-hir.Validation
cargo fmt --all -- --checkcargo clippy -p perry-hir -- -A clippy::not_unsafe_ptr_arg_derefRemaining clippy output is pre-existing warning debt not covered by automatic fixes in this branch.
Summary by CodeRabbit
mangle_typeis available only during tests.