Skip to content

nitpicks #5886

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
Jul 2, 2025
Merged

nitpicks #5886

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
319065e
Use `Self` instead of struct name
ShaharNaveh Jun 30, 2025
8c8ccdc
General fixes
ShaharNaveh Jun 30, 2025
df03be4
General
ShaharNaveh Jun 29, 2025
bcfe02a
General fixes
ShaharNaveh Jul 2, 2025
91b5666
More nitpicks
ShaharNaveh Jul 2, 2025
9aed7aa
Added newlines
ShaharNaveh Jul 2, 2025
d9aeed7
More nitpicks
ShaharNaveh Jul 2, 2025
3992208
Use Self where possible
ShaharNaveh Jul 2, 2025
7a6e815
Use Self where possible
ShaharNaveh Jul 2, 2025
48b332c
Use Self
ShaharNaveh Jul 2, 2025
75ac4c4
Use Self more
ShaharNaveh Jul 2, 2025
da95cca
Use Self
ShaharNaveh Jul 2, 2025
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
22 changes: 14 additions & 8 deletions derive-impl/src/pyclass.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ impl std::fmt::Display for AttrName {

impl FromStr for AttrName {
type Err = String;

fn from_str(s: &str) -> std::result::Result<Self, Self::Err> {
Ok(match s {
"pymethod" => Self::Method,
Expand Down Expand Up @@ -1216,6 +1217,7 @@ impl ItemMeta for MethodItemMeta {
fn from_inner(inner: ItemMetaInner) -> Self {
Self(inner)
}

fn inner(&self) -> &ItemMetaInner {
&self.0
}
Expand All @@ -1225,6 +1227,7 @@ impl MethodItemMeta {
fn raw(&self) -> Result<bool> {
self.inner()._bool("raw")
}

fn method_name(&self) -> Result<String> {
let inner = self.inner();
let name = inner._optional_str("name")?;
Expand All @@ -1244,6 +1247,7 @@ impl ItemMeta for GetSetItemMeta {
fn from_inner(inner: ItemMetaInner) -> Self {
Self(inner)
}

fn inner(&self) -> &ItemMetaInner {
&self.0
}
Expand Down Expand Up @@ -1274,8 +1278,8 @@ impl GetSetItemMeta {
if name.is_empty() {
Err(err_span!(
inner.meta_ident,
"A #[{}({typ})] fn with a {prefix}* name must \
have something after \"{prefix}\"",
r#"A #[{}({typ})] fn with a {prefix}* name must \
have something after "{prefix}""#,
inner.meta_name(),
typ = item_typ,
prefix = prefix
Expand All @@ -1286,8 +1290,8 @@ impl GetSetItemMeta {
} else {
Err(err_span!(
inner.meta_ident,
"A #[{}(setter)] fn must either have a `name` \
parameter or a fn name along the lines of \"set_*\"",
r#"A #[{}(setter)] fn must either have a `name` \
parameter or a fn name along the lines of "set_*""#,
inner.meta_name()
))
}
Expand Down Expand Up @@ -1337,6 +1341,7 @@ impl ItemMeta for SlotItemMeta {
fn from_inner(inner: ItemMetaInner) -> Self {
Self(inner)
}

fn inner(&self) -> &ItemMetaInner {
&self.0
}
Expand Down Expand Up @@ -1389,6 +1394,7 @@ impl ItemMeta for MemberItemMeta {
fn from_inner(inner: ItemMetaInner) -> Self {
Self(inner)
}

fn inner(&self) -> &ItemMetaInner {
&self.0
}
Expand All @@ -1403,8 +1409,8 @@ impl MemberItemMeta {
if name.is_empty() {
Err(err_span!(
inner.meta_ident,
"A #[{}({typ})] fn with a {prefix}* name must \
have something after \"{prefix}\"",
r#"A #[{}({typ})] fn with a {prefix}* name must \
have something after "{prefix}""#,
inner.meta_name(),
typ = item_typ,
prefix = prefix
Expand All @@ -1415,8 +1421,8 @@ impl MemberItemMeta {
} else {
Err(err_span!(
inner.meta_ident,
"A #[{}(setter)] fn must either have a `name` \
parameter or a fn name along the lines of \"set_*\"",
r#"A #[{}(setter)] fn must either have a `name` \
parameter or a fn name along the lines of "set_*""#,
inner.meta_name()
))
}
Expand Down
6 changes: 3 additions & 3 deletions vm/src/readline.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ mod basic_readline {

impl<H: Helper> Readline<H> {
pub fn new(helper: H) -> Self {
Readline { helper }
Self { helper }
}

pub fn load_history(&mut self, _path: &Path) -> OtherResult<()> {
Expand Down Expand Up @@ -86,7 +86,7 @@ mod rustyline_readline {
)
.expect("failed to initialize line editor");
repl.set_helper(Some(helper));
Readline { repl }
Self { repl }
}

pub fn load_history(&mut self, path: &Path) -> OtherResult<()> {
Expand Down Expand Up @@ -136,7 +136,7 @@ pub struct Readline<H: Helper>(readline_inner::Readline<H>);

impl<H: Helper> Readline<H> {
pub fn new(helper: H) -> Self {
Readline(readline_inner::Readline::new(helper))
Self(readline_inner::Readline::new(helper))
}

pub fn load_history(&mut self, path: &Path) -> OtherResult<()> {
Expand Down
6 changes: 3 additions & 3 deletions vm/src/stdlib/ast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ mod type_parameters;

fn get_node_field(vm: &VirtualMachine, obj: &PyObject, field: &'static str, typ: &str) -> PyResult {
vm.get_attribute_opt(obj.to_owned(), field)?
.ok_or_else(|| vm.new_type_error(format!("required field \"{field}\" missing from {typ}")))
.ok_or_else(|| vm.new_type_error(format!(r#"required field "{field}" missing from {typ}"#)))
}

fn get_node_field_opt(
Expand All @@ -76,7 +76,7 @@ fn get_int_field(
) -> PyResult<PyRefExact<PyInt>> {
get_node_field(vm, obj, field, typ)?
.downcast_exact(vm)
.map_err(|_| vm.new_type_error(format!("field \"{field}\" must have integer type")))
.map_err(|_| vm.new_type_error(format!(r#"field "{field}" must have integer type"#)))
}

struct PySourceRange {
Expand Down Expand Up @@ -107,7 +107,7 @@ impl Row {
self.0.get()
}

fn get_one_indexed(self) -> OneIndexed {
const fn get_one_indexed(self) -> OneIndexed {
self.0
}
}
Expand Down
55 changes: 28 additions & 27 deletions vm/src/stdlib/ast/constant.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,48 +21,49 @@ impl Constant {
}
}

pub(super) fn new_int(value: ruff::Int, range: TextRange) -> Self {
pub(super) const fn new_int(value: ruff::Int, range: TextRange) -> Self {
Self {
range,
value: ConstantLiteral::Int(value),
}
}

pub(super) fn new_float(value: f64, range: TextRange) -> Self {
pub(super) const fn new_float(value: f64, range: TextRange) -> Self {
Self {
range,
value: ConstantLiteral::Float(value),
}
}
pub(super) fn new_complex(real: f64, imag: f64, range: TextRange) -> Self {

pub(super) const fn new_complex(real: f64, imag: f64, range: TextRange) -> Self {
Self {
range,
value: ConstantLiteral::Complex { real, imag },
}
}

pub(super) fn new_bytes(value: Box<[u8]>, range: TextRange) -> Self {
pub(super) const fn new_bytes(value: Box<[u8]>, range: TextRange) -> Self {
Self {
range,
value: ConstantLiteral::Bytes(value),
}
}
Comment on lines +45 to 50
Copy link
Contributor

@coderabbitai coderabbitai bot Jul 2, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Mismatch between new_bytes signature and its call-sites

new_bytes still expects a Box<[u8]>, but bytes_literal_to_object (l. 363-366, unchanged) passes a Vec<u8>.
This doesn’t compile on stable Rust – there’s no implicit conversion.

Either change the constructor to accept impl Into<Box<[u8]>> (cannot be const yet because trait calls aren’t const-stable) or convert at the call-site:

-let c = Constant::new_bytes(bytes.copied().collect(), range);
+let c = Constant::new_bytes(bytes.copied().collect::<Vec<u8>>().into(), range);

Failing to fix this will break the build.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
pub(super) const fn new_bytes(value: Box<[u8]>, range: TextRange) -> Self {
Self {
range,
value: ConstantLiteral::Bytes(value),
}
}
// in bytes_literal_to_object (around line 363)
- let c = Constant::new_bytes(bytes.copied().collect(), range);
+ let c = Constant::new_bytes(bytes.copied().collect::<Vec<u8>>().into(), range);
🤖 Prompt for AI Agents 
In vm/src/stdlib/ast/constant.rs around lines 45 to 50, the new_bytes function
currently accepts a Box<[u8]>, but its call-sites pass a Vec<u8>, causing a
compilation error. To fix this, either change new_bytes to accept a Vec<u8> and
convert it to Box<[u8]> inside the function, or convert the Vec<u8> to Box<[u8]>
at the call-sites before calling new_bytes. Since trait calls are not
const-stable, avoid using impl Into<Box<[u8]>> in the const fn signature.


pub(super) fn new_bool(value: bool, range: TextRange) -> Self {
pub(super) const fn new_bool(value: bool, range: TextRange) -> Self {
Self {
range,
value: ConstantLiteral::Bool(value),
}
}

pub(super) fn new_none(range: TextRange) -> Self {
pub(super) const fn new_none(range: TextRange) -> Self {
Self {
range,
value: ConstantLiteral::None,
}
}

pub(super) fn new_ellipsis(range: TextRange) -> Self {
pub(super) const fn new_ellipsis(range: TextRange) -> Self {
Self {
range,
value: ConstantLiteral::Ellipsis,
Expand Down Expand Up @@ -137,30 +138,30 @@ impl Node for Constant {
impl Node for ConstantLiteral {
fn ast_to_object(self, vm: &VirtualMachine, source_code: &SourceCodeOwned) -> PyObjectRef {
match self {
ConstantLiteral::None => vm.ctx.none(),
ConstantLiteral::Bool(value) => vm.ctx.new_bool(value).to_pyobject(vm),
ConstantLiteral::Str { value, .. } => vm.ctx.new_str(value).to_pyobject(vm),
ConstantLiteral::Bytes(value) => vm.ctx.new_bytes(value.into()).to_pyobject(vm),
ConstantLiteral::Int(value) => value.ast_to_object(vm, source_code),
ConstantLiteral::Tuple(value) => {
Self::None => vm.ctx.none(),
Self::Bool(value) => vm.ctx.new_bool(value).to_pyobject(vm),
Self::Str { value, .. } => vm.ctx.new_str(value).to_pyobject(vm),
Self::Bytes(value) => vm.ctx.new_bytes(value.into()).to_pyobject(vm),
Self::Int(value) => value.ast_to_object(vm, source_code),
Self::Tuple(value) => {
let value = value
.into_iter()
.map(|c| c.ast_to_object(vm, source_code))
.collect();
vm.ctx.new_tuple(value).to_pyobject(vm)
}
ConstantLiteral::FrozenSet(value) => PyFrozenSet::from_iter(
Self::FrozenSet(value) => PyFrozenSet::from_iter(
vm,
value.into_iter().map(|c| c.ast_to_object(vm, source_code)),
)
.unwrap()
.into_pyobject(vm),
ConstantLiteral::Float(value) => vm.ctx.new_float(value).into_pyobject(vm),
ConstantLiteral::Complex { real, imag } => vm
Self::Float(value) => vm.ctx.new_float(value).into_pyobject(vm),
Self::Complex { real, imag } => vm
.ctx
.new_complex(num_complex::Complex::new(real, imag))
.into_pyobject(vm),
ConstantLiteral::Ellipsis => vm.ctx.ellipsis(),
Self::Ellipsis => vm.ctx.ellipsis(),
}
}

Expand All @@ -171,24 +172,24 @@ impl Node for ConstantLiteral {
) -> PyResult<Self> {
let cls = value_object.class();
let value = if cls.is(vm.ctx.types.none_type) {
ConstantLiteral::None
Self::None
} else if cls.is(vm.ctx.types.bool_type) {
ConstantLiteral::Bool(if value_object.is(&vm.ctx.true_value) {
Self::Bool(if value_object.is(&vm.ctx.true_value) {
true
} else if value_object.is(&vm.ctx.false_value) {
false
} else {
value_object.try_to_value(vm)?
})
} else if cls.is(vm.ctx.types.str_type) {
ConstantLiteral::Str {
Self::Str {
value: value_object.try_to_value::<String>(vm)?.into(),
prefix: StringLiteralPrefix::Empty,
}
} else if cls.is(vm.ctx.types.bytes_type) {
ConstantLiteral::Bytes(value_object.try_to_value::<Vec<u8>>(vm)?.into())
Self::Bytes(value_object.try_to_value::<Vec<u8>>(vm)?.into())
} else if cls.is(vm.ctx.types.int_type) {
ConstantLiteral::Int(Node::ast_from_object(vm, source_code, value_object)?)
Self::Int(Node::ast_from_object(vm, source_code, value_object)?)
} else if cls.is(vm.ctx.types.tuple_type) {
let tuple = value_object.downcast::<PyTuple>().map_err(|obj| {
vm.new_type_error(format!(
Expand All @@ -202,18 +203,18 @@ impl Node for ConstantLiteral {
.cloned()
.map(|object| Node::ast_from_object(vm, source_code, object))
.collect::<PyResult<_>>()?;
ConstantLiteral::Tuple(tuple)
Self::Tuple(tuple)
} else if cls.is(vm.ctx.types.frozenset_type) {
let set = value_object.downcast::<PyFrozenSet>().unwrap();
let elements = set
.elements()
.into_iter()
.map(|object| Node::ast_from_object(vm, source_code, object))
.collect::<PyResult<_>>()?;
ConstantLiteral::FrozenSet(elements)
Self::FrozenSet(elements)
} else if cls.is(vm.ctx.types.float_type) {
let float = value_object.try_into_value(vm)?;
ConstantLiteral::Float(float)
Self::Float(float)
} else if cls.is(vm.ctx.types.complex_type) {
let complex = value_object.try_complex(vm)?;
let complex = match complex {
Expand All @@ -226,12 +227,12 @@ impl Node for ConstantLiteral {
}
Some((value, _was_coerced)) => value,
};
ConstantLiteral::Complex {
Self::Complex {
real: complex.re,
imag: complex.im,
}
} else if cls.is(vm.ctx.types.ellipsis_type) {
ConstantLiteral::Ellipsis
Self::Ellipsis
} else {
return Err(vm.new_type_error(format!(
"invalid type in Constant: {}",
Expand Down
3 changes: 2 additions & 1 deletion vm/src/stdlib/ast/exception.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,13 @@ impl Node for ruff::ExceptHandlerExceptHandler {
node_add_location(&dict, _range, _vm, source_code);
node.into()
}

fn ast_from_object(
_vm: &VirtualMachine,
source_code: &SourceCodeOwned,
_object: PyObjectRef,
) -> PyResult<Self> {
Ok(ruff::ExceptHandlerExceptHandler {
Ok(Self {
type_: get_node_field_opt(_vm, &_object, "type")?
.map(|obj| Node::ast_from_object(_vm, source_code, obj))
.transpose()?,
Expand Down
Loading
Loading