Actionable comments posted: 0

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

Reviewing files that changed from the base of the PR and between 3031d5b and 7bc7887.

• Lib/test/test_genericalias.py (0 hunks)
• vm/src/builtins/genericalias.rs (4 hunks)
• vm/src/vm/context.rs (1 hunks)

• Lib/test/test_genericalias.py

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.

• GitHub Check: Run tests under miri
• GitHub Check: Check Rust code with rustfmt and clippy
• GitHub Check: Check the WASM package and demo
• GitHub Check: Run snippets and cpython tests (macos-latest)
• GitHub Check: Run snippets and cpython tests (windows-latest)
• GitHub Check: Run snippets and cpython tests (ubuntu-latest)
• GitHub Check: Run snippets and cpython tests on wasm-wasi
• GitHub Check: Run rust tests (ubuntu-latest)
• GitHub Check: Run rust tests (windows-latest)
• GitHub Check: Run rust tests (macos-latest)
• GitHub Check: Ensure compilation on various targets

vm/src/vm/context.rs (1)

226-226: LGTM!

The addition of __typing_subst__ constant follows the established pattern and is necessary for the enhanced generic alias parameter handling.

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

284-309: TypeVarTuple handling looks correct!

The implementation properly handles TypeVarTuple expansion by checking for the iterator slot and expanding tuple elements when appropriate. This aligns with CPython's behavior for type parameter substitution.

Actionable comments posted: 1

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

Reviewing files that changed from the base of the PR and between 7bc7887 and b4f4ffa.

• Lib/test/test_dataclasses.py (0 hunks)
• Lib/test/test_genericalias.py (0 hunks)
• Lib/test/test_types.py (0 hunks)
• vm/src/builtins/genericalias.rs (4 hunks)
• vm/src/vm/context.rs (1 hunks)

• Lib/test/test_genericalias.py
• Lib/test/test_types.py
• Lib/test/test_dataclasses.py

• vm/src/vm/context.rs

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.

• GitHub Check: Run rust tests (ubuntu-latest)
• GitHub Check: Run tests under miri
• GitHub Check: Run snippets and cpython tests on wasm-wasi
• GitHub Check: Run snippets and cpython tests (macos-latest)
• GitHub Check: Run rust tests (macos-latest)
• GitHub Check: Check the WASM package and demo
• GitHub Check: Check Rust code with rustfmt and clippy
• GitHub Check: Run snippets and cpython tests (ubuntu-latest)
• GitHub Check: Run rust tests (windows-latest)
• GitHub Check: Ensure compilation on various targets
• GitHub Check: Run snippets and cpython tests (windows-latest)

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

266-269: LGTM! Clean helper function implementation.

The tuple_index_vec function correctly uses identity comparison with is() which is appropriate for Python object references.

---

256-258: Good optimization with truncate.

Using truncate instead of shrink_to_fit is more appropriate here since we know the exact final size needed.

---

235-254: Verify the nested parameter expansion logic.

The recursive parameter expansion with index management is complex and could be error-prone. The logic appears correct but warrants careful testing.

#!/bin/bash
# Search for tests that cover nested parameter expansion scenarios
rg -A 10 -B 5 "__parameters__.*__parameters__" --type py

---

284-314: Verify TypeVarTuple detection mechanism.

The TypeVarTuple detection using param.class().slots.iter.load().is_some() is interesting but may not be the most reliable way to identify TypeVarTuple objects.

#!/bin/bash
# Search for TypeVarTuple usage patterns and tests
rg -A 5 -B 5 "TypeVarTuple" --type py
ast-grep --pattern 'class TypeVarTuple($_) { $$$ }'

---

338-370: ```shell
#!/bin/bash
sed -n '330,370p' vm/src/builtins/genericalias.rs

</details>

</blockquote></details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

Actionable comments posted: 0

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

Reviewing files that changed from the base of the PR and between 5fd3525 and 433f24e.

• Lib/test/test_typing.py (1 hunks)
• vm/src/builtins/genericalias.rs (4 hunks)

• Lib/test/test_typing.py

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.

• GitHub Check: Run rust tests (windows-latest)
• GitHub Check: Run rust tests (macos-latest)
• GitHub Check: Run rust tests (ubuntu-latest)
• GitHub Check: Run snippets and cpython tests on wasm-wasi
• GitHub Check: Run snippets and cpython tests (macos-latest)
• GitHub Check: Run snippets and cpython tests (ubuntu-latest)
• GitHub Check: Run snippets and cpython tests (windows-latest)
• GitHub Check: Ensure compilation on various targets
• GitHub Check: Check the WASM package and demo
• GitHub Check: Check Rust code with rustfmt and clippy
• GitHub Check: Run tests under miri

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

20-34: LGTM: Appropriate expansion of special attributes.

The addition of __class__, __bases__, __unpacked__, and __typing_unpacked_tuple_args__ to the attribute exceptions list is appropriate for comprehensive generic alias support.

---

266-269: LGTM: Clean utility function.

The tuple_index function is a straightforward utility that correctly uses identity comparison for Python objects.

---

338-370: Excellent default parameter handling implementation.

The logic for handling default parameters is well-implemented with proper validation and informative error messages. The approach of counting defaults from the end backwards aligns with Python's parameter handling conventions.

Actionable comments posted: 0

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

Reviewing files that changed from the base of the PR and between bdd7519 and af2d36c.

• Lib/test/test_functools.py (0 hunks)
• Lib/test/test_genericalias.py (0 hunks)
• Lib/test/test_typing.py (2 hunks)
• vm/src/protocol/object.rs (1 hunks)
• vm/src/stdlib/typing.rs (5 hunks)

• Lib/test/test_functools.py
• Lib/test/test_genericalias.py

• vm/src/protocol/object.rs
• Lib/test/test_typing.py

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.

• GitHub Check: Run rust tests (windows-latest)
• GitHub Check: Check the WASM package and demo
• GitHub Check: Run snippets and cpython tests (windows-latest)
• GitHub Check: Run rust tests (macos-latest)
• GitHub Check: Run rust tests (ubuntu-latest)
• GitHub Check: Run snippets and cpython tests on wasm-wasi
• GitHub Check: Run tests under miri
• GitHub Check: Check Rust code with rustfmt and clippy
• GitHub Check: Run snippets and cpython tests (ubuntu-latest)
• GitHub Check: Ensure compilation on various targets
• GitHub Check: Run snippets and cpython tests (macos-latest)

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

16-17: LGTM! Clean import additions for new functionality.

The added imports (PyObjectRef, PyRef, PyTupleRef, and Iterable) are necessary for the new Iterable trait implementation and properly follow the existing import organization.

Also applies to: 20-20

---

610-610: Good addition of Iterable trait to TypeVarTuple.

Adding Iterable to the trait list enables iteration support for TypeVarTuple, which aligns with CPython's behavior for handling variadic generics.

---

670-682: Well-implemented Iterable trait for TypeVarTuple.

The implementation correctly handles the unpacking behavior by:

1. Importing the typing module to access Unpack
2. Creating Unpack[self] to represent the unpacked tuple
3. Returning a list iterator containing the unpacked element

This aligns with CPython's behavior for Generic[*Ts] as mentioned in the comment.

---

971-978: Excellent refactor to delegate to Python typing module.

The change from directly creating PyGenericAlias to calling _generic_class_getitem via the helper function aligns with CPython's behavior and centralizes the logic in the Python typing module.

---

980-983: Good delegation to Python typing module for subclass initialization.

The refactor to use call_typing_args_kwargs for _generic_init_subclass is consistent with the class_getitem changes and properly delegates complex logic to the Python side.