Implement mapping trait for GenericAlias
#3374
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
youknowone
requested changes
Oct 25, 2021
vm/src/builtins/genericalias.rs
Outdated
Comment on lines
198
to
202
| for idx in 0..tuple.len() { | ||
| if tuple.fast_getitem(idx).is(item) { | ||
| return Some(idx); | ||
| } | ||
| } |
There was a problem hiding this comment.
PyTuple is immutable so it doesn't have a lock.
Suggested change
| for idx in 0..tuple.len() { | |
| if tuple.fast_getitem(idx).is(item) { | |
| return Some(idx); | |
| } | |
| } | |
| params.as_slice().position(|p| p.is(arg)) |
There was a problem hiding this comment.
Ah.. I agree with that. Thanks!
vm/src/builtins/genericalias.rs
Outdated
| @@ -1,10 +1,12 @@ | |||
| use crate::{ | |||
| builtins::tuple::IntoPyTuple, | |||
There was a problem hiding this comment.
This is not intended to be used as an API. I'd better to hide this better than now. vm.ctx.new_tuple or PyTuple::new_ref will suggest more consistent way like other types.
vm/src/builtins/genericalias.rs
Outdated
| } | ||
| } | ||
|
|
||
| let newargs = newargs.into_pytuple(vm); |
There was a problem hiding this comment.
Suggested change
| let newargs = newargs.into_pytuple(vm); | |
| let newargs = newargs.into(); |
Into is enough to convert from PyRef to PyObjectRef
0cf8b3a to
9870276
Compare
youknowone
requested changes
Oct 25, 2021
vm/src/builtins/genericalias.rs
Outdated
Comment on lines
210
to
226
| if let Ok(sub_params) = PyTupleRef::try_from_object(vm, sub_params) { | ||
| let sub_args = sub_params | ||
| .as_slice() | ||
| .iter() | ||
| .map(|arg| { | ||
| if let Some(idx) = tuple_index(params, arg) { | ||
| argitems[idx].clone() | ||
| } else { | ||
| arg.clone() | ||
| } | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
| let sub_args: PyObjectRef = PyTuple::new_ref(sub_args, &vm.ctx).into(); | ||
| Some(obj.get_item(sub_args, vm)) | ||
| } else { | ||
| None | ||
| } |
There was a problem hiding this comment.
Suggested change
| if let Ok(sub_params) = PyTupleRef::try_from_object(vm, sub_params) { | |
| let sub_args = sub_params | |
| .as_slice() | |
| .iter() | |
| .map(|arg| { | |
| if let Some(idx) = tuple_index(params, arg) { | |
| argitems[idx].clone() | |
| } else { | |
| arg.clone() | |
| } | |
| }) | |
| .collect::<Vec<_>>(); | |
| let sub_args: PyObjectRef = PyTuple::new_ref(sub_args, &vm.ctx).into(); | |
| Some(obj.get_item(sub_args, vm)) | |
| } else { | |
| None | |
| } | |
| PyTupleRef::try_from_object(vm, sub_params).ok().map(|sub_params| { | |
| let sub_args = sub_params | |
| .as_slice() | |
| .iter() | |
| .map(|arg| { | |
| if let Some(idx) = tuple_index(params, arg) { | |
| argitems[idx].clone() | |
| } else { | |
| arg.clone() | |
| } | |
| }) | |
| .collect::<Vec<_>>(); | |
| let sub_args: PyObjectRef = PyTuple::new_ref(sub_args, &vm.ctx).into(); | |
| Some(obj.get_item(sub_args, vm)) | |
| }) |
vm/src/builtins/genericalias.rs
Outdated
Comment on lines
275
to
283
| let mut new_args: Vec<PyObjectRef> = Vec::with_capacity(zelf.args.len()); | ||
| for arg in zelf.args.as_slice().iter() { | ||
| if is_typevar(arg) { | ||
| let idx = tuple_index(&zelf.parameters, arg).unwrap(); | ||
| new_args.push(arg_items[idx].clone()); | ||
| } else { | ||
| new_args.push(subs_tvars(arg.clone(), &zelf.parameters, arg_items, vm)?); | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggested change
| let mut new_args: Vec<PyObjectRef> = Vec::with_capacity(zelf.args.len()); | |
| for arg in zelf.args.as_slice().iter() { | |
| if is_typevar(arg) { | |
| let idx = tuple_index(&zelf.parameters, arg).unwrap(); | |
| new_args.push(arg_items[idx].clone()); | |
| } else { | |
| new_args.push(subs_tvars(arg.clone(), &zelf.parameters, arg_items, vm)?); | |
| } | |
| } | |
| let new_args = zelf.args.as_slice().iter().map(|arg| { | |
| if is_typevar(arg) { | |
| let idx = tuple_index(&zelf.parameters, arg).unwrap(); | |
| Ok(arg_items[idx].clone()) | |
| } else { | |
| subs_tvars(arg.clone(), &zelf.parameters, arg_items, vm) | |
| } | |
| }).collect::<Result<Vec<_>, _>>()?; |
with_capacity is inferred through iterator
There was a problem hiding this comment.
wow.. collect::<Result<Vec<_>, _>> seems great. I didn't know that Vec<Result> can be converted like that
9870276 to
4705b04
Compare
Signed-off-by: snowapril <sinjihng@gmail.com>
Signed-off-by: snowapril <sinjihng@gmail.com>
Signed-off-by: snowapril <sinjihng@gmail.com>
4705b04 to
795bfeb
Compare
|
I add extra test that didn't caught by unittest from typing import TypeVar
Y = TypeVar('Y')
assert dict[str,Y][int] == dict[str, int] |
youknowone
approved these changes
Oct 26, 2021
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This revision implements
mappingtrait forGenericAliasAlthough this is not included in #3244, actually
GenericAliasprovide mapping protocol interface.(https://github.com/python/cpython/blob/main/Objects/genericaliasobject.c#L376)
Reference