Apply automatic clippy fixes in runtime and FFI - #5426
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 (12)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (9)
📝 WalkthroughWalkthroughBroad mechanical refactoring across ChangesRuntime and FFI idiom modernization
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 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.
🧹 Nitpick comments (2)
crates/perry-runtime/src/dgram.rs (1)
1550-1576: ⚡ Quick winInconsistent finiteness validation between
set_ttl_implandset_multicast_ttl_impl.Line 1552 in
set_ttl_implretains the explicit!ttl.is_finite()check, while line 1566 inset_multicast_ttl_implrelies on implicit rejection via range containment. Although both will correctly reject NaN/Infinity (since they fail the range check), the inconsistency makes the code harder to maintain.Recommend explicitly checking
is_finite()inset_multicast_ttl_implfor symmetry and clarity:fn set_multicast_ttl_impl(socket: f64, args: &[f64]) -> f64 { let ttl = validate_number_arg(args.first().copied().unwrap_or_else(undefined_value), "ttl"); - if !(0.0..=255.0).contains(&ttl) { + if !ttl.is_finite() || !(0.0..=255.0).contains(&ttl) { throw_socket_errno("setMulticastTTL", "EINVAL"); }🤖 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-runtime/src/dgram.rs` around lines 1550 - 1576, The set_ttl_impl function explicitly checks is_finite() before the range validation, but set_multicast_ttl_impl relies only on implicit rejection via range containment. Add an explicit is_finite() check to set_multicast_ttl_impl before the range check to match the validation pattern in set_ttl_impl. This will make the validation logic consistent and more maintainable across both functions.crates/perry-runtime/src/web_storage.rs (1)
375-389: 💤 Low valueConsider removing the no-op rebinding on line 381.
Line 381 (
let ptr = ptr;) is a no-op rebinding that was left after theas i64cast was removed. While functionally correct (the subsequent pointer comparisons work as-is), this line serves no purpose and can be deleted entirely.fn storage_kind_from_value(value: f64) -> Option<StorageKind> { let this = value; let ptr = crate::value::js_nanbox_get_pointer(this); if ptr == 0 { return None; } - let ptr = ptr; if ptr == crate::object::LOCAL_STORAGE_PTR.load(Ordering::Acquire) {🤖 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-runtime/src/web_storage.rs` around lines 375 - 389, In the storage_kind_from_value function, remove the unnecessary rebinding statement `let ptr = ptr;` that appears after the null pointer check. This line is a no-op leftover from a previous refactoring and does not affect the subsequent pointer comparisons, so it can be safely deleted to clean up the code.
🤖 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.
Nitpick comments:
In `@crates/perry-runtime/src/dgram.rs`:
- Around line 1550-1576: The set_ttl_impl function explicitly checks is_finite()
before the range validation, but set_multicast_ttl_impl relies only on implicit
rejection via range containment. Add an explicit is_finite() check to
set_multicast_ttl_impl before the range check to match the validation pattern in
set_ttl_impl. This will make the validation logic consistent and more
maintainable across both functions.
In `@crates/perry-runtime/src/web_storage.rs`:
- Around line 375-389: In the storage_kind_from_value function, remove the
unnecessary rebinding statement `let ptr = ptr;` that appears after the null
pointer check. This line is a no-op leftover from a previous refactoring and
does not affect the subsequent pointer comparisons, so it can be safely deleted
to clean up the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 31494789-edb7-4b3f-abaf-9ae5aa59ba17
📒 Files selected for processing (101)
crates/perry-ffi/src/jsvalue.rscrates/perry-runtime/src/arena/walk.rscrates/perry-runtime/src/array/concat_reverse.rscrates/perry-runtime/src/array/from_concat.rscrates/perry-runtime/src/array/generic.rscrates/perry-runtime/src/array/indexing.rscrates/perry-runtime/src/array/iter_object.rscrates/perry-runtime/src/array/splice_slice.rscrates/perry-runtime/src/atomics.rscrates/perry-runtime/src/bigint.rscrates/perry-runtime/src/box.rscrates/perry-runtime/src/buffer/access.rscrates/perry-runtime/src/buffer/dataview.rscrates/perry-runtime/src/buffer/from.rscrates/perry-runtime/src/buffer/header.rscrates/perry-runtime/src/buffer/query.rscrates/perry-runtime/src/buffer/transcode.rscrates/perry-runtime/src/buffer/u8_codec.rscrates/perry-runtime/src/buffer/validate.rscrates/perry-runtime/src/builtins/console.rscrates/perry-runtime/src/builtins/globals.rscrates/perry-runtime/src/builtins/numbers.rscrates/perry-runtime/src/child_process/mod.rscrates/perry-runtime/src/child_process/sync_run.rscrates/perry-runtime/src/closure/dispatch.rscrates/perry-runtime/src/closure/dynamic_props.rscrates/perry-runtime/src/closure/mod.rscrates/perry-runtime/src/cluster_sched.rscrates/perry-runtime/src/collection_iter.rscrates/perry-runtime/src/date.rscrates/perry-runtime/src/dgram.rscrates/perry-runtime/src/fs/dir_glob_watch.rscrates/perry-runtime/src/fs/dirent.rscrates/perry-runtime/src/fs/mod.rscrates/perry-runtime/src/gc/barrier.rscrates/perry-runtime/src/gc/heap_snapshot.rscrates/perry-runtime/src/gc/oldgen.rscrates/perry-runtime/src/gc/roots.rscrates/perry-runtime/src/gc/telemetry.rscrates/perry-runtime/src/gc/trace.rscrates/perry-runtime/src/gc/types.rscrates/perry-runtime/src/i18n.rscrates/perry-runtime/src/json/parse_api.rscrates/perry-runtime/src/json/raw_json.rscrates/perry-runtime/src/json/replacer.rscrates/perry-runtime/src/json/reviver.rscrates/perry-runtime/src/json_tape.rscrates/perry-runtime/src/lib.rscrates/perry-runtime/src/map.rscrates/perry-runtime/src/native_abi.rscrates/perry-runtime/src/native_arena.rscrates/perry-runtime/src/node_stream.rscrates/perry-runtime/src/node_stream_constructors.rscrates/perry-runtime/src/node_stream_readwrite.rscrates/perry-runtime/src/node_submodules/diagnostics.rscrates/perry-runtime/src/node_submodules/stream_promises.rscrates/perry-runtime/src/node_submodules/trace_events.rscrates/perry-runtime/src/node_submodules/zlib.rscrates/perry-runtime/src/node_vm.rscrates/perry-runtime/src/object/alloc.rscrates/perry-runtime/src/object/array_object_ops.rscrates/perry-runtime/src/object/buffer_dispatch.rscrates/perry-runtime/src/object/class_registry.rscrates/perry-runtime/src/object/exotic_expando.rscrates/perry-runtime/src/object/field_get_set.rscrates/perry-runtime/src/object/field_set_by_name.rscrates/perry-runtime/src/object/global_this.rscrates/perry-runtime/src/object/instanceof.rscrates/perry-runtime/src/object/mod.rscrates/perry-runtime/src/object/native_call_method.rscrates/perry-runtime/src/object/native_module.rscrates/perry-runtime/src/object/native_module_dispatch.rscrates/perry-runtime/src/object/object_ops.rscrates/perry-runtime/src/object/prototype_helpers.rscrates/perry-runtime/src/os/signal.rscrates/perry-runtime/src/path.rscrates/perry-runtime/src/process.rscrates/perry-runtime/src/process/credentials.rscrates/perry-runtime/src/promise/combinators.rscrates/perry-runtime/src/proxy.rscrates/perry-runtime/src/punycode.rscrates/perry-runtime/src/readline_helpers.rscrates/perry-runtime/src/regex.rscrates/perry-runtime/src/regex/replace_expand.rscrates/perry-runtime/src/set.rscrates/perry-runtime/src/string/compare.rscrates/perry-runtime/src/string/format.rscrates/perry-runtime/src/string/split.rscrates/perry-runtime/src/symbol.rscrates/perry-runtime/src/temporal/zoned_date_time.rscrates/perry-runtime/src/timer.rscrates/perry-runtime/src/tui/ffi.rscrates/perry-runtime/src/typed_feedback/guards.rscrates/perry-runtime/src/typedarray/mod.rscrates/perry-runtime/src/typedarray_props.rscrates/perry-runtime/src/typedarray_view.rscrates/perry-runtime/src/ui_text_registry.rscrates/perry-runtime/src/url/node_compat.rscrates/perry-runtime/src/util_parse_env.rscrates/perry-runtime/src/web_storage.rscrates/perry-runtime/src/yoga.rs
💤 Files with no reviewable changes (3)
- crates/perry-runtime/src/array/iter_object.rs
- crates/perry-runtime/src/object/native_module_dispatch.rs
- crates/perry-runtime/src/builtins/numbers.rs
clippy --fix dropped `ObjectHeader` and `js_object_get_field_by_name_f64`
from the `use crate::object::{...}` in node_stream.rs because they are only
referenced by `#[cfg(test)]` code (node_stream_state_tests.rs et al. via
`use super::*`). That broke `cargo test -p perry-runtime` with E0425. Restore
them under a `#[cfg(test)]` import so the non-test build stays clippy-clean.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…fi-auto-fixes # Conflicts: # crates/perry-runtime/src/closure/dynamic_props.rs
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-runtime/src/object/object_ops.rs`:
- Around line 2811-2828: The native-module prototype handling logic is
duplicated across two branches in the object_ops.rs file (the `top16 == 0x7FFD`
branch and the `top16 == 0 && bits >= threshold` branch). This 18-line block
that checks if `(*obj).class_id == super::native_module::NATIVE_MODULE_CLASS_ID`
and returns either `Object.prototype` or `TAG_NULL` should be extracted into a
separate helper function. Create a helper function that encapsulates this check
and returns the appropriate prototype value, then replace both instances of the
duplicate block by calling this helper function instead. This eliminates code
duplication while maintaining the same behavior at both call sites.
🪄 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: 0aa3aaac-1d2f-4759-8b70-89d5c2c58fd9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
crates/perry-runtime/src/closure/dispatch.rscrates/perry-runtime/src/closure/dynamic_props.rscrates/perry-runtime/src/fs/mod.rscrates/perry-runtime/src/json/replacer.rscrates/perry-runtime/src/lib.rscrates/perry-runtime/src/object/class_registry.rscrates/perry-runtime/src/object/field_get_set.rscrates/perry-runtime/src/object/field_set_by_name.rscrates/perry-runtime/src/object/mod.rscrates/perry-runtime/src/object/native_call_method.rscrates/perry-runtime/src/object/object_ops.rscrates/perry-runtime/src/proxy.rs
✅ Files skipped from review due to trivial changes (2)
- crates/perry-runtime/src/fs/mod.rs
- crates/perry-runtime/src/json/replacer.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- crates/perry-runtime/src/proxy.rs
- crates/perry-runtime/src/object/mod.rs
- crates/perry-runtime/src/closure/dynamic_props.rs
- crates/perry-runtime/src/object/field_set_by_name.rs
- crates/perry-runtime/src/closure/dispatch.rs
- crates/perry-runtime/src/object/field_get_set.rs
- crates/perry-runtime/src/lib.rs
- crates/perry-runtime/src/object/native_call_method.rs
- crates/perry-runtime/src/object/class_registry.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: 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-runtime/src/object/object_ops.rs`:
- Around line 2811-2828: The native-module prototype handling logic is
duplicated across two branches in the object_ops.rs file (the `top16 == 0x7FFD`
branch and the `top16 == 0 && bits >= threshold` branch). This 18-line block
that checks if `(*obj).class_id == super::native_module::NATIVE_MODULE_CLASS_ID`
and returns either `Object.prototype` or `TAG_NULL` should be extracted into a
separate helper function. Create a helper function that encapsulates this check
and returns the appropriate prototype value, then replace both instances of the
duplicate block by calling this helper function instead. This eliminates code
duplication while maintaining the same behavior at both call sites.
🪄 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: 0aa3aaac-1d2f-4759-8b70-89d5c2c58fd9
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
crates/perry-runtime/src/closure/dispatch.rscrates/perry-runtime/src/closure/dynamic_props.rscrates/perry-runtime/src/fs/mod.rscrates/perry-runtime/src/json/replacer.rscrates/perry-runtime/src/lib.rscrates/perry-runtime/src/object/class_registry.rscrates/perry-runtime/src/object/field_get_set.rscrates/perry-runtime/src/object/field_set_by_name.rscrates/perry-runtime/src/object/mod.rscrates/perry-runtime/src/object/native_call_method.rscrates/perry-runtime/src/object/object_ops.rscrates/perry-runtime/src/proxy.rs
✅ Files skipped from review due to trivial changes (2)
- crates/perry-runtime/src/fs/mod.rs
- crates/perry-runtime/src/json/replacer.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- crates/perry-runtime/src/proxy.rs
- crates/perry-runtime/src/object/mod.rs
- crates/perry-runtime/src/closure/dynamic_props.rs
- crates/perry-runtime/src/object/field_set_by_name.rs
- crates/perry-runtime/src/closure/dispatch.rs
- crates/perry-runtime/src/object/field_get_set.rs
- crates/perry-runtime/src/lib.rs
- crates/perry-runtime/src/object/native_call_method.rs
- crates/perry-runtime/src/object/class_registry.rs
🛑 Comments failed to post (1)
crates/perry-runtime/src/object/object_ops.rs (1)
2811-2828: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Extract duplicate native-module prototype logic.
This identical 18-line block (including comments) appears in both the
top16 == 0x7FFDbranch (lines 2811-2828) and thetop16 == 0 && bits >= thresholdbranch (lines 2931-2948). The logic that checksclass_id == NATIVE_MODULE_CLASS_IDand returnsObject.prototype(orTAG_NULL) should be extracted into a helper function to eliminate the duplication.♻️ Suggested refactor
Extract a helper function:
fn native_module_prototype_or_none(obj: *const ObjectHeader) -> Option<f64> { unsafe { if (*obj).class_id == super::native_module::NATIVE_MODULE_CLASS_ID { let proto = crate::object::builtin_prototype_value("Object"); if proto.to_bits() != crate::value::TAG_UNDEFINED { return Some(proto); } return Some(f64::from_bits(TAG_NULL)); } } None }Then replace both blocks with:
+ if let Some(proto) = native_module_prototype_or_none(obj) { + return proto; + } - // A native-module namespace object (`require("path")` etc., - // class_id NATIVE_MODULE_CLASS_ID, the `__module__`-tagged - // object) is an ordinary object whose [[Prototype]] is - // %Object.prototype% — NOT itself. The `return obj_value` self- - // prototype fallback below makes turbopack's `interopEsm` - // proto-chain walk (`for(cur=raw; !LEAF.includes(cur); - // cur=getProto(cur))`) never terminate — getProto keeps - // returning the same object, so it creates export getters - // forever (the Next.js standalone startup runaway: unbounded - // memory growth, no `✓ Ready`). Return Object.prototype so the - // walk reaches a LEAF_PROTOTYPE and stops. - if (*obj).class_id == super::native_module::NATIVE_MODULE_CLASS_ID { - let proto = crate::object::builtin_prototype_value("Object"); - if proto.to_bits() != crate::value::TAG_UNDEFINED { - return proto; - } - return f64::from_bits(TAG_NULL); - }Apply the same replacement at both locations (lines 2811-2828 and 2931-2948).
Also applies to: 2931-2948
🤖 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-runtime/src/object/object_ops.rs` around lines 2811 - 2828, The native-module prototype handling logic is duplicated across two branches in the object_ops.rs file (the `top16 == 0x7FFD` branch and the `top16 == 0 && bits >= threshold` branch). This 18-line block that checks if `(*obj).class_id == super::native_module::NATIVE_MODULE_CLASS_ID` and returns either `Object.prototype` or `TAG_NULL` should be extracted into a separate helper function. Create a helper function that encapsulates this check and returns the appropriate prototype value, then replace both instances of the duplicate block by calling this helper function instead. This eliminates code duplication while maintaining the same behavior at both call sites.
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-runtimeandcrates/perry-ffi.arena/walk.rsto avoid reintroducingdangerous_implicit_autorefswarnings after rustfix.Validation
cargo fmt --all -- --checkcargo check -p perry-runtime -p perry-ffiNote:
cargo clippy -p perry-runtime -p perry-ffi -- -A clippy::not_unsafe_ptr_arg_derefcurrently stops on pre-existing denied lints in untouched files (array/sort.rsanddgram.rs), so this PR usescargo checkfor compile validation.Summary by CodeRabbit
is_multiple_ofto keep logic more consistent.ToIntegerOrInfinitybehavior and error propagation timing.