Visitar URL original
General code nits by ShaharNaveh · Pull Request #8107 · RustPython/RustPython · GitHub
Skip to content

General code nits #8107

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
youknowone merged 12 commits into from
Jun 22, 2026
Merged

General code nits #8107

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3702d56
General nits
ShaharNaveh Jun 15, 2026
955f68e
compiler
ShaharNaveh Jun 15, 2026
3ba9d6c
cformat
ShaharNaveh Jun 15, 2026
2f1287a
gc_state
ShaharNaveh Jun 15, 2026
2075c16
fix release compile
ShaharNaveh Jun 15, 2026
e212f16
Fix logic error
ShaharNaveh Jun 16, 2026
1119806
Merge remote-tracking branch 'upstream/main' into nits-7
ShaharNaveh Jun 16, 2026
fe84b5b
Merge remote-tracking branch 'upstream/main' into nits-7
ShaharNaveh Jun 16, 2026
9f53fff
revert bad logic error
ShaharNaveh Jun 16, 2026
948497b
Merge remote-tracking branch 'upstream/main' into nits-7
ShaharNaveh Jun 17, 2026
c8b9b0f
don't inline elapsed_time
ShaharNaveh Jun 17, 2026
56dc068
Merge remote-tracking branch 'upstream/main' into nits-7
ShaharNaveh Jun 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
compiler
  • Loading branch information
@ShaharNaveh
ShaharNaveh committed Jun 15, 2026
commit 955f68e1bbd1297f4907213f7aa2dec58513bdde
63 changes: 30 additions & 33 deletions crates/vm/src/compiler.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,45 +1,42 @@
#[cfg(all(not(feature = "compiler"), feature = "parser", feature = "codegen",))]
compile_error!("Use --features=compiler to enable both parser and codegen");
Comment on lines +1 to +2

@coderabbitai coderabbitai Bot Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Inspect the relevant feature declarations and cfg gates without building.

rg -n -C3 '\bcompiler\s*=|\bparser\s*=|\bcodegen\s*=' --iglob 'Cargo.toml'
rg -n -C3 'new_syntax_error(_maybe_incomplete)?|impl crate::convert::ToPyException for \(CompileError' --type=rust

Repository: RustPython/RustPython

Length of output: 14894

Change the ToPyException impl gates to match the VM method gates.

The new compile-time guard only rejects not(compiler) && parser && codegen, but codegen alone is a valid feature combination. Building with --features=codegen without parser or compiler would compile the ToPyException impls (lines 41-52) while the VM methods they call—new_syntax_error and new_syntax_error_maybe_incomplete—remain gated on any(feature = "parser", feature = "compiler"), causing a linking failure.

Fix: change the cfg gates on both impls from #[cfg(any(feature = "parser", feature = "codegen"))] to #[cfg(any(feature = "parser", feature = "compiler"))] to match the availability of the called methods.

Suggested diff 
-#[cfg(any(feature = "parser", feature = "codegen"))]
+#[cfg(any(feature = "parser", feature = "compiler"))]
 impl crate::convert::ToPyException for (CompileError, Option<&str>) {
     fn to_pyexception(&self, vm: &crate::VirtualMachine) -> crate::builtins::PyBaseExceptionRef {
         vm.new_syntax_error(&self.0, self.1)
     }
 }
 
-#[cfg(any(feature = "parser", feature = "codegen"))]
+#[cfg(any(feature = "parser", feature = "compiler"))]
 impl crate::convert::ToPyException for (CompileError, Option<&str>, bool) {
     fn to_pyexception(&self, vm: &crate::VirtualMachine) -> crate::builtins::PyBaseExceptionRef {
         vm.new_syntax_error_maybe_incomplete(&self.0, self.1, self.2)
     }
 }
🤖 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/vm/src/compiler.rs` around lines 1 - 2, The ToPyException impl blocks
at lines 41-52 have feature gates that don't match the VM method gates they
depend on. Locate the two ToPyException impl blocks that call the methods
new_syntax_error and new_syntax_error_maybe_incomplete, and change their cfg
attribute from any(feature = "parser", feature = "codegen") to any(feature =
"parser", feature = "compiler") to match the gate on those called VM methods,
preventing linking failures when building with --features=codegen alone.


#[cfg(feature = "codegen")]
pub use rustpython_codegen::CompileOpts;

#[cfg(feature = "compiler")]
pub use rustpython_compiler::*;

#[cfg(not(feature = "compiler"))]
pub use rustpython_compiler_core::Mode;
cfg_select! {
feature = "compiler" => {
pub use rustpython_compiler::*;
}
_ => {
pub use ruff_python_parser as parser;

#[cfg(not(feature = "compiler"))]
pub use rustpython_compiler_core as core;
pub use rustpython_compiler_core::Mode;
pub use rustpython_compiler_core as core;
}
}

#[cfg(not(feature = "compiler"))]
pub use ruff_python_parser as parser;
#[derive(Debug, thiserror::Error)]
pub enum CompileErrorType {
#[cfg(feature = "codegen")]
#[error(transparent)]
Codegen(#[from] super::codegen::error::CodegenErrorType),
#[cfg(feature = "parser")]
#[error(transparent)]
Parse(#[from] super::parser::ParseErrorType),
Comment on lines +24 to +27

@coderabbitai coderabbitai Bot Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Inspect whether `super::parser` / `super::codegen` can resolve from `crate::compiler`.

rg -n -C3 'super::(parser|codegen)::|pub use ruff_python_parser as parser|rustpython_codegen::' --type=rust
rg -n -C3 '^\s*(pub\s+)?mod\s+(parser|codegen)\b|^\s*pub\s+use\s+.*\s+as\s+(parser|codegen)\b' --type=rust

Repository: RustPython/RustPython

Length of output: 4574

🏁 Script executed:

#!/bin/bash
# Check if these are truly at top level or in a nested module
head -50 crates/vm/src/compiler.rs | cat -n

Repository: RustPython/RustPython

Length of output: 2043

🏁 Script executed:

#!/bin/bash
# Look for where codegen module might be defined or re-exported
rg -n 'mod codegen|pub use.*codegen' crates/vm/src/compiler.rs

Repository: RustPython/RustPython

Length of output: 108

🏁 Script executed:

#!/bin/bash
# Check rustpython_codegen crate structure for the error types
fd -t f '\.rs$' crates/codegen/src/ 2>/dev/null | head -20 || echo "codegen crate not found, checking if it's external"

Repository: RustPython/RustPython

Length of output: 307

🏁 Script executed:

#!/bin/bash
# Verify what types are actually available from ruff_python_parser via the parser alias
rg -n 'ParseError' crates/compiler/src/ | head -10

Repository: RustPython/RustPython

Length of output: 893

🏁 Script executed:

#!/bin/bash
# Check what error types rustpython_codegen exports from its error module
rg -n 'pub.*CodegenError' crates/codegen/src/error.rs | head -5

Repository: RustPython/RustPython

Length of output: 260

🏁 Script executed:

#!/bin/bash
# Check what ParseErrorType is exported as in ruff_python_parser
# Since ruff_python_parser is external, check how it's used in crates/compiler
rg -n 'ParseErrorType|ParseError' crates/compiler/src/lib.rs | grep -E '(use|import|pub)'

Repository: RustPython/RustPython

Length of output: 287

🏁 Script executed:

#!/bin/bash
# Verify that super::parser and super::codegen don't resolve by checking if these modules exist in vm crate
ls -la crates/vm/src/ | grep -E 'codegen|parser' || echo "No codegen or parser directories found"

Repository: RustPython/RustPython

Length of output: 104

🏁 Script executed:

#!/bin/bash
# Check if the file actually compiles with these broken paths
# by looking for any build/compilation context
cd crates/vm && cargo check --no-default-features --features parser 2>&1 | head -50 || echo "Cannot build in sandbox"

Repository: RustPython/RustPython

Length of output: 151

🏁 Script executed:

#!/bin/bash
# Double-check the exact exports from rustpython_codegen error module
rg -n 'pub (enum|struct) (CodegenError|CodegenErrorType)' crates/codegen/src/error.rs

Repository: RustPython/RustPython

Length of output: 125

🏁 Script executed:

#!/bin/bash
# Verify the cfg_select conditions that guard the parser alias availability
rg -B5 -A10 'pub use ruff_python_parser as parser' crates/vm/src/compiler.rs

Repository: RustPython/RustPython

Length of output: 449

Update stale super:: paths after promoting the error enums.

The super::codegen::error::* and super::parser::* paths are broken. These enums are at the top level of compiler.rs with no parent module containing codegen or parser subdirectories. Use the local parser alias (defined on line 12) and the absolute rustpython_codegen::error::* path instead.

Proposed fix 
     #[cfg(feature = "codegen")]
     #[error(transparent)]
-    Codegen(#[from] super::codegen::error::CodegenErrorType),
+    Codegen(#[from] rustpython_codegen::error::CodegenErrorType),
     #[cfg(feature = "parser")]
     #[error(transparent)]
-    Parse(#[from] super::parser::ParseErrorType),
+    Parse(#[from] parser::ParseErrorType),
 }
@@
     #[cfg(feature = "codegen")]
     #[error(transparent)]
-    Codegen(#[from] super::codegen::error::CodegenError),
+    Codegen(#[from] rustpython_codegen::error::CodegenError),
     #[cfg(feature = "parser")]
     #[error(transparent)]
-    Parse(#[from] super::parser::ParseError),
+    Parse(#[from] parser::ParseError),
 }
🤖 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/vm/src/compiler.rs` around lines 24 - 27, The module paths in the
error enum variant definitions are stale and no longer valid. The Codegen
variant references super::codegen::error::CodegenErrorType which should be
replaced with the absolute path rustpython_codegen::error::CodegenErrorType. The
Parse variant references super::parser::ParseErrorType which should be replaced
with the local parser alias that is already defined on line 12, changing it to
simply parser::ParseErrorType.

}

#[cfg(not(feature = "compiler"))]
mod error {
#[cfg(all(feature = "parser", feature = "codegen"))]
panic!("Use --features=compiler to enable both parser and codegen");

#[derive(Debug, thiserror::Error)]
pub enum CompileErrorType {
#[cfg(feature = "codegen")]
#[error(transparent)]
Codegen(#[from] super::codegen::error::CodegenErrorType),
#[cfg(feature = "parser")]
#[error(transparent)]
Parse(#[from] super::parser::ParseErrorType),
}

#[derive(Debug, thiserror::Error)]
pub enum CompileError {
#[cfg(feature = "codegen")]
#[error(transparent)]
Codegen(#[from] super::codegen::error::CodegenError),
#[cfg(feature = "parser")]
#[error(transparent)]
Parse(#[from] super::parser::ParseError),
}
#[derive(Debug, thiserror::Error)]
pub enum CompileError {
#[cfg(feature = "codegen")]
#[error(transparent)]
Codegen(#[from] super::codegen::error::CodegenError),
#[cfg(feature = "parser")]
#[error(transparent)]
Parse(#[from] super::parser::ParseError),
}
#[cfg(not(feature = "compiler"))]
pub use error::{CompileError, CompileErrorType};

#[cfg(any(feature = "parser", feature = "codegen"))]
impl crate::convert::ToPyException for (CompileError, Option<&str>) {
Expand Down