vm/src/builtins/genericalias.rs (3)

217-257: Consider optimizing vector resizing for better performance.

The current implementation resizes the vector multiple times during execution, which could lead to unnecessary allocations. Consider collecting parameters into a temporary structure first or using a more efficient growth strategy.

You could optimize by first collecting all parameters and then creating the final vector:

 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;
+    let mut seen = std::collections::HashSet::new();
+    let mut parameters = Vec::new();

     for arg in args {
         // 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;
+            if seen.insert(arg.as_object().id()) {
+                parameters.push(arg.clone());
             }
         } 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 {
-                    // 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;
+                    if seen.insert(sub_param.as_object().id()) {
+                        parameters.push(sub_param.clone());
                     }
                 }
             }
         }
     }

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

---

261-269: Refactor to avoid code duplication between tuple_index and tuple_index_vec.

Both functions perform the same operation of finding an item by identity. Consider creating a generic helper function.

 #[inline]
-fn tuple_index(tuple: &PyTupleRef, item: &PyObjectRef) -> Option<usize> {
-    tuple.iter().position(|element| element.is(item))
+fn find_by_identity<'a>(iter: impl Iterator<Item = &'a PyObjectRef>, item: &PyObjectRef) -> Option<usize> {
+    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 tuple_index(tuple: &PyTupleRef, item: &PyObjectRef) -> Option<usize> {
+    find_by_identity(tuple.iter(), item)
+}
+
+#[inline]
+fn tuple_index_vec(vec: &[PyObjectRef], item: &PyObjectRef) -> Option<usize> {
+    find_by_identity(vec.iter(), item)
 }

---

375-392: Clarify error message for unsupported default values.

The comment indicates that CPython doesn't support default values in this context, but the error message doesn't make this clear to users.

             } 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
+                    "No argument provided for parameter '{}' at index {} (default values not supported with __typing_subst__)",
+                    arg.repr(vm).unwrap_or_else(|_| "?".to_string()),
+                    idx
                 )));
             }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.109Z
Learning: Use the macro system (such as 'pyclass', 'pymodule', 'pyfunction') when implementing Python functionality in Rust for RustPython.

Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.109Z
Learning: When implementing a Python module in Rust for RustPython, use the #[pymodule] attribute and expose functions and classes using #[pyfunction], #[pyclass], and #[pymethod] as appropriate.

vm/src/builtins/genericalias.rs (1)

217-234: Consider simplifying the vector management logic.

The index-based insertion with resize calls could be simplified. The current approach works but may be harder to maintain.

Consider this cleaner approach:

-    let mut parameters: Vec<PyObjectRef> = Vec::with_capacity(args.len());
-    let mut iparam = 0;
-
     for arg in args {
         // 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;
+            if !parameters.contains(arg) {
+                parameters.push(arg.clone());
             }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.109Z
Learning: Use the macro system (such as 'pyclass', 'pymodule', 'pyfunction') when implementing Python functionality in Rust for RustPython.

Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.109Z
Learning: When implementing a Python module in Rust for RustPython, use the #[pymodule] attribute and expose functions and classes using #[pyfunction], #[pyclass], and #[pymethod] as appropriate.

vm/src/builtins/genericalias.rs (1)

374-392: This still needs verification against CPython behavior.

The error handling for missing parameters when using __typing_subst__ may still be too strict compared to CPython's actual behavior, as noted in the previous review.

The previous review comment about this exact issue remains valid. Please verify CPython's behavior for this scenario:

#!/bin/bash
# Verify CPython's behavior with __typing_subst__ and missing parameters
python3 -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)
"

vm/src/builtins/genericalias.rs (2)

220-264: Consider simplifying the complex parameter handling logic.

The rewritten make_parameters function introduces significant complexity with vector resizing, index tracking, and deduplication logic. While the approach appears to align with CPython's behavior, the implementation could be error-prone.

Consider these potential improvements:

1. Simplify vector management: Instead of frequent resizing, use a Vec that grows naturally and truncate at the end.
2. Optimize deduplication: Consider using a HashSet for O(1) duplicate detection instead of linear search via tuple_index.
3. Add comprehensive comments: The complex logic would benefit from detailed comments explaining the CPython behavior being mimicked.

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;
+    let mut parameters: Vec<PyObjectRef> = Vec::new();
+    let mut seen: std::collections::HashSet<*const PyObject> = std::collections::HashSet::new();

    for arg in args {
        // 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(&parameters, arg).is_none() {
-                if iparam >= parameters.len() {
-                    parameters.resize(iparam + 1, vm.ctx.none());
-                }
-                parameters[iparam] = arg.clone();
-                iparam += 1;
-            }
+            if seen.insert(arg.as_ptr()) {
+                parameters.push(arg.clone());
+            }
        } else if let Ok(subparams) = arg.get_attr(identifier!(vm, __parameters__), vm) {
            // ... similar simplification for sub_params handling
        }
    }

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

---

284-308: Good TypeVarTuple handling, but add clarifying comments.

The logic for handling TypeVarTuple expansion is correct, but the implementation would benefit from clearer comments explaining the behavior.

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

-                            // Check if this is a TypeVarTuple (has tp_iter)
+                            // TypeVarTuple handling: Check if this parameter has iteration support
+                            // and the substituted argument is a tuple that should be expanded
                            if param.class().slots.iter.load().is_some()
                                && substituted_arg.try_to_ref::<PyTuple>(vm).is_ok()
                            {
-                                // TypeVarTuple case - extend with tuple elements
+                                // Expand tuple elements for TypeVarTuple substitution
                                if let Ok(tuple) = substituted_arg.try_to_ref::<PyTuple>(vm) {
                                    for elem in tuple.iter() {
                                        sub_args.push(elem.clone());
                                    }
                                    continue;
                                }
                            }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.109Z
Learning: Use the macro system (such as 'pyclass', 'pymodule', 'pyfunction') when implementing Python functionality in Rust for RustPython.

vm/src/stdlib/typing.rs (1)

937-959: Consider caching the typing module import for performance.

The helper function is well-structured and follows good practices for calling typing module functions. However, it imports the typing module on every call.

Consider caching the typing module reference if this function is called frequently:

fn call_typing_args_kwargs(
    name: &'static str,
    cls: PyTypeRef,
    args: FuncArgs,
    vm: &VirtualMachine,
) -> PyResult {
-   let typing = vm.import("typing", 0)?;
+   // Consider caching if performance becomes an issue
+   let typing = vm.import("typing", 0)?;
    let func = typing.get_attr(name, vm)?;
    
    // Prepare arguments: (cls, *args)
    let mut call_args = vec![cls.into()];
    call_args.extend(args.args);
    
    // Call with prepared args and original kwargs
    let func_args = FuncArgs {
        args: call_args,
        kwargs: args.kwargs,
    };
    
    func.call(func_args, vm)
}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.109Z
Learning: Use the macro system (such as 'pyclass', 'pymodule', 'pyfunction') when implementing Python functionality in Rust for RustPython.

Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-06-26T12:52:11.109Z
Learning: When implementing a Python module in Rust for RustPython, use the #[pymodule] attribute and expose functions and classes using #[pyfunction], #[pyclass], and #[pymethod] as appropriate.