Visitar URL original
Fix genericalias parameters by youknowone · Pull Request #5841 · RustPython/RustPython · GitHub
Skip to content

Fix genericalias parameters #5841

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 10 commits into from
Jun 27, 2025
Merged

Fix genericalias parameters #5841

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
b4f4ffa
genericalias
youknowone Jun 26, 2025
5fd3525
fixgenericalias
youknowone Jun 26, 2025
6f4c9fc
fix test
youknowone Jun 26, 2025
433f24e
Fix attr_exceptions list
youknowone Jun 26, 2025
eb7afdf
unmark
youknowone Jun 26, 2025
6161d13
Fix init
youknowone Jun 26, 2025
af2d36c
impl TypeVarTuple iter
youknowone Jun 26, 2025
d16062f
Update _collections_abc.py to Python 3.13
youknowone Jun 26, 2025
bdee951
genericalias.__unpack__
youknowone Jun 26, 2025
bd133fd
fixup
youknowone Jun 26, 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
2 changes: 0 additions & 2 deletions Lib/test/test_dataclasses.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1906,8 +1906,6 @@ def new_method(self):
c = Alias(10, 1.0)
self.assertEqual(c.new_method(), 1.0)

# TODO: RUSTPYTHON
@unittest.expectedFailure
def test_generic_dynamic(self):
T = TypeVar('T')

Expand Down
4 changes: 0 additions & 4 deletions Lib/test/test_genericalias.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,8 +173,6 @@ def test_exposed_type(self):
self.assertEqual(a.__args__, (int,))
self.assertEqual(a.__parameters__, ())

# TODO: RUSTPYTHON
@unittest.expectedFailure
def test_parameters(self):
from typing import List, Dict, Callable
D0 = dict[str, int]
Expand Down Expand Up @@ -214,8 +212,6 @@ def test_parameters(self):
self.assertEqual(L5.__args__, (Callable[[K, V], K],))
self.assertEqual(L5.__parameters__, (K, V))

# TODO: RUSTPYTHON
@unittest.expectedFailure
def test_parameter_chaining(self):
from typing import List, Dict, Union, Callable
self.assertEqual(list[T][int], list[int])
Expand Down
2 changes: 0 additions & 2 deletions Lib/test/test_types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -825,8 +825,6 @@ def check(arg, expected):
check(x | None, (x, type(None)))
check(None | x, (type(None), x))

# TODO: RUSTPYTHON
@unittest.expectedFailure
def test_union_parameter_chaining(self):
T = typing.TypeVar("T")
S = typing.TypeVar("S")
Expand Down
157 changes: 115 additions & 42 deletions vm/src/builtins/genericalias.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,34 +212,49 @@ impl PyGenericAlias {
}
}

pub(crate) fn is_typevar(obj: &PyObjectRef, vm: &VirtualMachine) -> bool {
let class = obj.class();
"TypeVar" == &*class.slot_name()
&& class
.get_attr(identifier!(vm, __module__))
.and_then(|o| o.downcast_ref::<PyStr>().map(|s| s.as_str() == "typing"))
.unwrap_or(false)
}

pub(crate) fn make_parameters(args: &Py<PyTuple>, vm: &VirtualMachine) -> PyTupleRef {
let mut parameters: Vec<PyObjectRef> = Vec::with_capacity(args.len());
let mut iparam = 0;

for arg in args {
if is_typevar(arg, vm) {
if !parameters.iter().any(|param| param.is(arg)) {
parameters.push(arg.clone());
// We don't want __parameters__ descriptor of a bare Python class.
if arg.class().is(vm.ctx.types.type_type) {
continue;
}

// Check for __typing_subst__ attribute (like CPython)
if arg.get_attr(identifier!(vm, __typing_subst__), vm).is_ok() {
// Use tuple_add equivalent logic
if tuple_index_vec(&parameters, arg).is_none() {
if iparam >= parameters.len() {
parameters.resize(iparam + 1, vm.ctx.none());
}
parameters[iparam] = arg.clone();
iparam += 1;
}
} else if let Ok(obj) = arg.get_attr(identifier!(vm, __parameters__), vm) {
if let Ok(sub_params) = obj.try_to_ref::<PyTuple>(vm) {
} else if let Ok(subparams) = arg.get_attr(identifier!(vm, __parameters__), vm) {
if let Ok(sub_params) = subparams.try_to_ref::<PyTuple>(vm) {
let len2 = sub_params.len();
// Resize if needed
if iparam + len2 > parameters.len() {
parameters.resize(iparam + len2, vm.ctx.none());
}
for sub_param in sub_params {
if !parameters.iter().any(|param| param.is(sub_param)) {
parameters.push(sub_param.clone());
// Use tuple_add equivalent logic
if tuple_index_vec(&parameters[..iparam], sub_param).is_none() {
if iparam >= parameters.len() {
parameters.resize(iparam + 1, vm.ctx.none());
}
parameters[iparam] = sub_param.clone();
iparam += 1;
}
}
}
}
}
parameters.shrink_to_fit();

// Resize to actual size
parameters.truncate(iparam);
PyTuple::new_ref(parameters, &vm.ctx)
}

Expand All @@ -248,6 +263,11 @@ fn tuple_index(tuple: &PyTupleRef, item: &PyObjectRef) -> Option<usize> {
tuple.iter().position(|element| element.is(item))
}

#[inline]
fn tuple_index_vec(vec: &[PyObjectRef], item: &PyObjectRef) -> Option<usize> {
vec.iter().position(|element| element.is(item))
}

fn subs_tvars(
obj: PyObjectRef,
params: &PyTupleRef,
Expand All @@ -261,23 +281,40 @@ fn subs_tvars(
.ok()
.filter(|sub_params| !sub_params.is_empty())
.map(|sub_params| {
let sub_args = sub_params
.iter()
.map(|arg| {
if let Some(idx) = tuple_index(params, arg) {
arg_items[idx].clone()
} else {
arg.clone()
let mut sub_args = Vec::new();

for arg in sub_params.iter() {
if let Some(idx) = tuple_index(params, arg) {
let param = &params[idx];
let substituted_arg = &arg_items[idx];

// Check if this is a TypeVarTuple (has tp_iter)
if param.class().slots.iter.load().is_some()
&& substituted_arg.try_to_ref::<PyTuple>(vm).is_ok()
{
// TypeVarTuple case - extend with tuple elements
if let Ok(tuple) = substituted_arg.try_to_ref::<PyTuple>(vm) {
for elem in tuple.iter() {
sub_args.push(elem.clone());
}
continue;
}
}
})
.collect::<Vec<_>>();

sub_args.push(substituted_arg.clone());
} else {
sub_args.push(arg.clone());
}
}

let sub_args: PyObjectRef = PyTuple::new_ref(sub_args, &vm.ctx).into();
obj.get_item(&*sub_args, vm)
})
})
.unwrap_or(Ok(obj))
}

// _Py_subs_parameters
pub fn subs_parameters<F: Fn(&VirtualMachine) -> PyResult<String>>(
repr: F,
args: PyTupleRef,
Expand All @@ -297,26 +334,62 @@ pub fn subs_parameters<F: Fn(&VirtualMachine) -> PyResult<String>>(
};

let num_items = arg_items.len();
if num_params != num_items {
let plural = if num_items > num_params {
"many"
} else {
"few"
};
return Err(vm.new_type_error(format!("Too {} arguments for {}", plural, repr(vm)?)));

// Check if we need to apply default values
if num_items < num_params {
// Count how many parameters have defaults
let mut params_with_defaults = 0;
for param in parameters.iter().rev() {
if let Ok(has_default) = vm.call_method(param, "has_default", ()) {
if has_default.try_to_bool(vm)? {
params_with_defaults += 1;
} else {
break; // No more defaults from this point backwards
}
} else {
break;
}
}

let min_required = num_params - params_with_defaults;
if num_items < min_required {
return Err(vm.new_type_error(format!(
"Too few arguments for {}; actual {}, expected at least {}",
repr(vm)?,
num_items,
min_required
)));
}
} else if num_items > num_params {
return Err(vm.new_type_error(format!(
"Too many arguments for {}; actual {}, expected {}",
repr(vm)?,
num_items,
num_params
)));
}

let new_args = args
.iter()
.map(|arg| {
if is_typevar(arg, vm) {
let idx = tuple_index(&parameters, arg).unwrap();
Ok(arg_items[idx].clone())
let mut new_args = Vec::new();

for arg in args.iter() {
// Check for __typing_subst__ attribute directly (like CPython)
if let Ok(subst) = arg.get_attr(identifier!(vm, __typing_subst__), vm) {
let idx = tuple_index(&parameters, arg).unwrap();
if idx < num_items {
// Call __typing_subst__ with the argument
let substituted = subst.call((arg_items[idx].clone(),), vm)?;
new_args.push(substituted);
} else {
subs_tvars(arg.clone(), &parameters, arg_items, vm)
// CPython doesn't support default values in this context
return Err(vm.new_type_error(format!(
"No argument provided for parameter at index {}",
idx
)));
}
})
.collect::<PyResult<Vec<_>>>()?;
} else {
new_args.push(subs_tvars(arg.clone(), &parameters, arg_items, vm)?);
}
}
Comment on lines +374 to +399

@coderabbitai coderabbitai Bot Jun 26, 2025 •
edited
Loading

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

Potential issue with missing parameter handling.

The error handling for missing parameters when using __typing_subst__ may be too strict. CPython might handle this case differently, particularly with default values.

         } else {
-            // CPython doesn't support default values in this context
-            return Err(vm.new_type_error(format!(
-                "No argument provided for parameter at index {}",
-                idx
-            )));
+            // Use the parameter itself as default (CPython behavior)
+            new_args.push(arg.clone());
         }

This needs verification against CPython's actual behavior in similar scenarios.

📝 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
for arg in args.iter() {
// Check for __typing_subst__ attribute directly (like CPython)
if let Ok(subst) = arg.get_attr(identifier!(vm, __typing_subst__), vm) {
let idx = tuple_index(&parameters, arg).unwrap();
if idx < num_items {
// Call __typing_subst__ with the argument
let substituted = subst.call((arg_items[idx].clone(),), vm)?;
new_args.push(substituted);
} else {
subs_tvars(arg.clone(), &parameters, arg_items, vm)
// CPython doesn't support default values in this context
return Err(vm.new_type_error(format!(
"No argument provided for parameter at index {}",
idx
)));
}
})
.collect::<PyResult<Vec<_>>>()?;
} else {
new_args.push(subs_tvars(arg.clone(), &parameters, arg_items, vm)?);
}
}
for arg in args.iter() {
// Check for __typing_subst__ attribute directly (like CPython)
if let Ok(subst) = arg.get_attr(identifier!(vm, __typing_subst__), vm) {
let idx = tuple_index(&parameters, arg).unwrap();
if idx < num_items {
// Call __typing_subst__ with the argument
let substituted = subst.call((arg_items[idx].clone(),), vm)?;
new_args.push(substituted);
} else {
// Use the parameter itself as default (CPython behavior)
new_args.push(arg.clone());
}
} else {
new_args.push(subs_tvars(arg.clone(), &parameters, arg_items, vm)?);
}
}
🤖 Prompt for AI Agents 
In vm/src/builtins/genericalias.rs between lines 374 and 392, the current code
returns a type error if an argument index exceeds the number of items, assuming
no default values are supported like CPython. To address the potential issue,
verify CPython's behavior regarding default parameters with __typing_subst__ and
adjust the error handling accordingly. If CPython allows default values or
handles missing parameters differently, modify the code to support default
values or skip substitution for those parameters instead of returning an error.

@youknowone youknowone Jun 26, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

python3.13 -c "
import typing
from typing import TypeVar, Generic

# Create a test case that would trigger the missing parameter scenario
T = TypeVar('T')
U = TypeVar('U', bound=str)

class MyGeneric(Generic[T, U]):
    pass

# Test various substitution scenarios
try:
    # This should help us understand how CPython handles missing parameters
    result = MyGeneric[int]  # Missing second parameter
    print('CPython allows partial substitution:', result)
except Exception as e:
    print('CPython error:', e)
"

CPython error: Too few arguments for <class '__main__.MyGeneric'>; actual 1, expected at least 2

@coderabbitai coderabbitai Bot Jun 26, 2025

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.

@youknowone Thank you for providing that test! Your output clearly shows that CPython raises an error with the format "Too few arguments for...; actual X, expected at least Y" when there are insufficient arguments for a generic type.

Looking at the current implementation in the diff, I can see that the RustPython code actually handles this correctly:

if num_items < min_required {
    return Err(vm.new_type_error(format!(
        "Too few arguments for {}; actual {}, expected at least {}",
        repr(vm)?,
        num_items,
        min_required
    )));
}

This matches CPython's behavior exactly. My original suggestion was incorrect - I was concerned that the error handling for the __typing_subst__ case might be too strict, but your test demonstrates that CPython does indeed raise errors for insufficient arguments.

The current implementation correctly:

  1. Validates argument counts upfront, considering parameters with defaults
  2. Uses error messages that match CPython's format
  3. Only reaches the "No argument provided for parameter at index" error in cases that should be unreachable if the earlier validation works properly

I retract my original suggestion. The current implementation appears to align properly with CPython's behavior.

🐰💭


Ok(PyTuple::new_ref(new_args, &vm.ctx))
}
Expand Down
1 change: 1 addition & 0 deletions vm/src/vm/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ declare_const_name! {
__sizeof__,
__truediv__,
__trunc__,
__typing_subst__,
__xor__,

// common names
Expand Down