No actionable comments were generated in the recent review. 🎉

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro Plus

Run ID: 3f7cb52a-f8d4-4426-9e50-2fea90e7c0d7

Reviewing files that changed from the base of the PR and between 03bfd70 and ef45416.

• crates/vm/src/vm/mod.rs

Walkthrough

The PR simplifies wchar_len calculation in codec error handling and adds targeted Clippy allowances for intentional drain-and-collect operations in the VM trampoline.

Changes

Tail-call reference handling

Code-page error handling

Estimated code review effort: 1 (Trivial) | ~5 minutes

Possibly related PRs

• RustPython/RustPython#7043: Both modify targeted Clippy handling in crates/vm/src/vm/mod.rs.
• RustPython/RustPython#8354: Both affect trampoline and frame-execution code in vm/mod.rs.

Suggested reviewers: youknowone, shaharnaveh

• Create PR with unit tests

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.

• X
• Mastodon
• Reddit
• LinkedIn

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

@ShaharNaveh I went through the code in depth to check if the lint was necessary. It turns out that Clippy is overzealous here. The original code pre-allocates a buffer to reuse. The drain/collect idiom is intentional and correct. Instead, I documented the behavior and allowed the lint so that anyone else who encounters it knows not to fix the lint. I also fixed a totally unrelated but small lint. 😁

I used #[allow] instead of #[expect] because the lint doesn't trigger on my stable compiler but does trigger on nightly. CI would fail with #[expect].

@ShaharNaveh I went through the code in depth to check if the lint was necessary. It turns out that Clippy is overzealous here. The original code pre-allocates a buffer to reuse. The drain/collect idiom is intentional and correct. Instead, I documented the behavior and allowed the lint so that anyone else who encounters it knows not to fix the lint. I also fixed a totally unrelated but small lint. 😁

I used #[allow] instead of #[expect] because the lint doesn't trigger on my stable compiler but does trigger on nightly. CI would fail with #[expect].

tysm for looking into it!

@joshuamegnauth54 can we still add this rule? so future places would get flagged

I used #[allow] instead of #[expect] because the lint doesn't trigger on my stable compiler but does trigger on nightly. CI would fail with #[expect].

@coderabbitai Can #[expect] be used within #[cfg] so that #[expect] only applies when built with the nightly compiler?

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '== Rust toolchain and project policy files =='
fd -HI -t f '^(rust-toolchain(\.toml)?|Cargo\.toml|clippy\.toml|CONTRIBUTING\.md|README\.md)$' . \
  | sed -n '1,160p'

printf '%s\n' '== Compiler-version and nightly configuration references =='
rg -n -i -C 3 \
  '(rust-version|rustup|nightly|stable|rustversion|check-cfg|unexpected_cfgs|drain_collect)' \
  -g 'Cargo.toml' -g 'rust-toolchain*' -g 'clippy.toml' -g '*.rs' -g '*.md' . \
  | sed -n '1,280p'

printf '%s\n' '== Tail-call source map =='
ast-grep outline crates/vm/src/vm/mod.rs --items all --match 'trampoline|tail|call' --view expanded || true

printf '%s\n' '== Changed lines in relevant files =='
git diff -- crates/vm/src/vm/mod.rs crates/vm/src/stdlib/_codecs.rs

Length of output: 18513

🤖 web_search executed:

site:doc.rust-lang.org/reference conditional compilation cfg Rust built-in cfg nightly stable compiler channel

Length of output: 5066

🤖 web_search executed:

site:doc.rust-lang.org rustc lint expect attribute unfulfilled_lint_expect documentation

Length of output: 8212

@fanninpm Yes, but Rust does not define a built-in cfg predicate for the compiler channel.

This attribute is valid if the build defines a custom nightly configuration option:

#[cfg_attr(nightly, expect(clippy::drain_collect))]
let refs: Vec<_> = refs.drain(..).collect();

A build script or CI must then define nightly only for nightly builds. A build script must also declare the option for check-cfg:

println!("cargo::rustc-check-cfg=cfg(nightly)");
println!("cargo::rustc-cfg=nightly");

The second line must run only when the selected rustc is nightly.

I do not recommend this for this PR. RustPython selects stable in rust-toolchain.toml and supports the latest stable Rust. The Clippy lint does not trigger consistently across those compiler versions. The documented #[allow(clippy::drain_collect)] therefore states the intended behavior without adding custom compiler-channel detection. #[expect] is best when the lint is expected to trigger on every supported toolchain.

You are interacting with an AI system.

It turns out that Clippy is overzealous here. The original code pre-allocates a buffer to reuse. The drain/collect idiom is intentional and correct.

See rust-lang/rust-clippy#12410

@ShaharNaveh Do you mean enabling the lint or denying the lint? The lint should already be enabled by default, yet I'm unsure why it didn't trigger. Here's a Playground lint that demonstrates that it triggers on stable. 🤔

@ShaharNaveh Do you mean enabling the lint or denying the lint? The lint should already be enabled by default, yet I'm unsure why it didn't trigger. Here's a Playground lint that demonstrates that it triggers on stable. 🤔

oh, it didn't trigger due to clippy bug, nvm...

tysm!