Use ruff_python_ast::name::Name for symtable and compiler#8344
Use ruff_python_ast::name::Name for symtable and compiler#8344ShaharNaveh wants to merge 1 commit into
ruff_python_ast::name::Name for symtable and compiler#8344Conversation
📝 WalkthroughWalkthroughThe compiler and symbol-table implementation migrate identifier storage, lookup, mangling, scope analysis, and lowering from ChangesName-backed code generation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/codegen/src/compile.rs (1)
2594-2606: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winUnnecessary String allocation on every name/varname lookup.
get_index_of::<String>(&name.as_str().into())allocates a freshStringfor every call toname()/varname()(i.e. for essentially every identifier compiled), even thoughIndexSet<String>already supports lookup by&strviaBorrow<str>. This defeats part of the point of migrating toNamefor reduced overhead.⚡ Proposed fix to avoid the redundant allocation
fn _name_inner( &mut self, name: &str, cache: impl FnOnce(&mut ir::CodeInfo) -> &mut IndexSet<String>, ) -> u32 { let target = name.into(); let name = self.mangle(&target); let cache = cache(self.current_code_info()); cache - .get_index_of::<String>(&name.as_str().into()) + .get_index_of(name.as_str()) .unwrap_or_else(|| cache.insert_full(name.to_string()).0) .to_u32() }🤖 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/codegen/src/compile.rs` around lines 2594 - 2606, Update _name_inner to perform the IndexSet lookup using the mangled name as a borrowed &str, removing the temporary String conversion in get_index_of while preserving the existing insertion and returned index behavior.
🧹 Nitpick comments (1)
crates/codegen/src/symboltable.rs (1)
1214-1230: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOptional: consider
Name::new_staticfor well-known special names here too.
.type_params,.generic_base,.defaults,.kwdefaults,__classdict__are constructed via&"...".into()here, while equivalent well-known names elsewhere in the file (.format,__annotate__,__classdict__at other sites) useName::new_static(...). Since the migration's goal is reducing memory footprint, usingName::new_staticconsistently for compile-time-known special names would avoid any redundant construction overhead, though the benefit is likely marginal since these are short strings.🤖 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/codegen/src/symboltable.rs` around lines 1214 - 1230, Use Name::new_static for the compile-time-known special names registered in this block, including __classdict__, .type_params, .generic_base, .defaults, and .kwdefaults, replacing the current string .into() construction while preserving their existing SymbolUsage and registration order.
🤖 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/codegen/src/compile.rs`:
- Around line 3065-3073: Cache the converted private name when each code scope
is created, changing CodeInfo.private to Option<Name> and updating its
initialization sites accordingly. Then update mangle to borrow the cached Name
directly instead of calling Name::from on every invocation, while preserving the
existing maybe_mangle_name behavior.
---
Outside diff comments:
In `@crates/codegen/src/compile.rs`:
- Around line 2594-2606: Update _name_inner to perform the IndexSet lookup using
the mangled name as a borrowed &str, removing the temporary String conversion in
get_index_of while preserving the existing insertion and returned index
behavior.
---
Nitpick comments:
In `@crates/codegen/src/symboltable.rs`:
- Around line 1214-1230: Use Name::new_static for the compile-time-known special
names registered in this block, including __classdict__, .type_params,
.generic_base, .defaults, and .kwdefaults, replacing the current string .into()
construction while preserving their existing SymbolUsage and registration order.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: ae711afa-e767-4dee-bf0d-1492e599d67e
📒 Files selected for processing (3)
crates/codegen/src/compile.rscrates/codegen/src/symboltable.rscrates/vm/src/stdlib/_symtable.rs
| fn mangle<'a>(&self, name: &'a Name) -> Cow<'a, Name> { | ||
| // Use private from current code unit for name mangling | ||
| let private = self | ||
| .code_stack | ||
| .last() | ||
| .and_then(|info| info.private.as_deref()); | ||
| .and_then(|info| info.private.as_ref()); | ||
| let mangled_names = self.current_symbol_table().mangled_names.as_ref(); | ||
| symboltable::maybe_mangle_name(private, mangled_names, name) | ||
| symboltable::maybe_mangle_name(private.map(Name::from).as_ref(), mangled_names, name) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -e rs ir.rs -x cat -n {} \; 2>/dev/null | head -0
FILE=$(fd -e rs '^ir\.rs$' crates/codegen/src | head -1)
if [ -n "$FILE" ]; then
rg -n 'private' "$FILE" -C2
else
echo "ir.rs not found under crates/codegen/src"
fiRepository: RustPython/RustPython
Length of output: 740
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the mangle implementation and its callers.
FILE=crates/codegen/src/compile.rs
sed -n '3048,3085p' "$FILE"
printf '\n--- callers ---\n'
rg -n 'mangle\(' "$FILE" -C2
printf '\n--- Name type / conversion ---\n'
# Find the definition/conversion for Name.
rg -n 'type Name|struct Name|impl From<.*String.*Name|impl From<.*&str.*Name|impl From<.*Name.*String' crates -g '*.rs' -C2Repository: RustPython/RustPython
Length of output: 5879
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- Name definition/conversions ---\n'
rg -n 'pub (struct|type) Name|impl From<.*Name|impl From<.*String|impl From<.*&str' crates -g '*.rs' -C3
printf '\n--- maybe_mangle_name ---\n'
rg -n 'fn maybe_mangle_name' crates -g '*.rs' -C3
printf '\n--- current symbol table / mangled_names type ---\n'
rg -n 'mangled_names' crates/codegen/src -g '*.rs' -C3Repository: RustPython/RustPython
Length of output: 250
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Name definition/conversions ---'
rg -n 'pub (struct|type) Name|impl From<.*Name|impl From<.*String|impl From<.*&str' crates -g '*.rs' -C3 || true
echo
echo '--- maybe_mangle_name ---'
rg -n 'fn maybe_mangle_name' crates -g '*.rs' -C3 || true
echo
echo '--- mangled_names usage/type ---'
rg -n 'mangled_names' crates/codegen/src -g '*.rs' -C3 || trueRepository: RustPython/RustPython
Length of output: 12386
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=crates/codegen/src/compile.rs
echo '--- imports around Name ---'
rg -n 'use .*Name|type Name|pub type Name|Name as' "$FILE" -C2
echo
echo '--- top of compile.rs ---'
sed -n '1,120p' "$FILE"
echo
echo '--- any impl From for Name in codegen-related crates ---'
rg -n 'impl From<.*Name|impl From<&.*String.*Name|impl From<String> for .*Name|impl From<&str> for .*Name' crates -g '*.rs' -C3 || trueRepository: RustPython/RustPython
Length of output: 4290
🌐 Web query:
ruff_python_ast::name::Name From<String> From<&str semantics
💡 Result:
In the ruff_python_ast crate, the Name struct implements the From trait for both &str and String, allowing for convenient conversion [1][2]. The implementations are defined as follows: impl<'a> From<&'a str> for Name { #[inline] fn from(s: &'a str) -> Self { Name(s.into) } } impl From for Name { #[inline] fn from(s: String) -> Self { Name(s.into) } } These implementations enable you to create a Name instance directly from string slices or owned strings using the.into method or by calling Name::from [1][2]. The internal representation of Name typically wraps the string data [1]. Additionally, there is often a corresponding implementation of From for String, which allows for the reverse conversion by calling name.as_str.into [1][2].
Citations:
- 1: https://github.com/astral-sh/ruff/blob/99d97bd7/crates/ruff_python_ast/src/name.rs
- 2: https://github.com/astral-sh/ruff/blob/b16565ef/crates/ruff_python_ast/src/name.rs
🌐 Web query:
ruff_python_ast name::Name documentation From<String> From<&str>
💡 Result:
In the ruff_python_ast crate, the Name struct is used to represent identifiers in the Python AST [1][2]. The implementation of the Name struct includes From trait implementations that allow it to be constructed from both &str and String [3]. Specifically, the source code for ruff_python_ast::name::Name includes the following implementations [3]: impl<'a> From<&'a str> for Name { #[inline] fn from(s: &'a str) -> Self { Name(s.into) } } impl From for Name { #[inline] fn from(s: String) -> Self { Name(s.into) } } These implementations enable ergonomic conversion from string types, facilitating the creation of AST nodes (such as ExprName, which uses Name for its id field) from raw string data [2][3]. Note that the internal implementation of Name may vary across versions (e.g., transitioning to specialized string representations for performance), but these From trait bridges are standard for integration [4].
Citations:
- 1: https://docs.rs/ruff_python_ast/latest/ruff_python_ast/name/index.html
- 2: https://docs.rs/ruff_python_ast/latest/ruff_python_ast/struct.ExprName.html
- 3: https://github.com/astral-sh/ruff/blob/b16565ef/crates/ruff_python_ast/src/name.rs
- 4: [ty] Experiment with LeanString-backed AST names astral-sh/ruff#26551
🌐 Web query:
site:docs.rs ruff_python_ast::name::Name From<String> From<&str>
💡 Result:
In the ruff_python_ast crate, the Name struct (located in ruff_python_ast::name) does not implement the standard library's From or From<&str> traits [1][2]. Instead, the Name struct is typically constructed using dedicated methods such as: - Name::new(name: impl AsRef) (or similar variations depending on the version) [1] - Name::new_static(&'static str) [1] Because Name is designed for efficient representation within the AST, it does not provide direct, blanket From implementations for arbitrary strings, which prevents accidental, inefficient allocations or type conversions during AST construction [1]. If you need to convert a String or &str into a Name, you should use the provided constructor methods rather than attempting to use From or Into.
Citations:
- 1: https://docs.rs/rustpython-ruff_python_ast/latest/rustpython_ruff_python_ast/name/struct.Name.html
- 2: https://docs.rs/ruff_python_ast/latest/ruff_python_ast/name/index.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- ruff_python_ast version pins ---'
rg -n 'ruff_python_ast|rustpython_ruff_python_ast' Cargo.lock Cargo.toml crates -g 'Cargo.toml' -g 'Cargo.lock' -C2 || true
echo
echo '--- vendored source presence ---'
fd -a 'name.rs' . | rg 'ruff_python_ast|rustpython_ruff_python_ast|name\.rs$' || true
echo
echo '--- current mangle usage context ---'
sed -n '3060,3075p' crates/codegen/src/compile.rs
sed -n '3430,3455p' crates/codegen/src/symboltable.rsRepository: RustPython/RustPython
Length of output: 5384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
URL='https://raw.githubusercontent.com/RustPython/ruff/3c1ab2dd9987f190f3df94d28452ed5e65e106af/crates/ruff_python_ast/src/name.rs'
curl -L --fail "$URL" | sed -n '1,220p'Repository: RustPython/RustPython
Length of output: 5011
Cache the private name once per scope crates/codegen/src/compile.rs:3065-3073 — Name::from(&String) allocates a new Name on every mangle() call, and this runs on essentially every identifier lookup in class scope. Store ir::CodeInfo.private as Option<Name> (or cache the converted value when the scope is created) so maybe_mangle_name() can borrow it directly.
🤖 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/codegen/src/compile.rs` around lines 3065 - 3073, Cache the converted
private name when each code scope is created, changing CodeInfo.private to
Option<Name> and updating its initialization sites accordingly. Then update
mangle to borrow the cached Name directly instead of calling Name::from on every
invocation, while preserving the existing maybe_mangle_name behavior.
Summary
this should reduce the memory footprint
Summary by CodeRabbit