Introduce `PyUnionType` #3497

moreal · 2021-12-05T18:05:57Z

This pull request is related with #3419, #3475.

This pull request does:

Introduce PyUnionType (a.k.a. types.UnionType, unionobject in CPython)
Bump modules
- _py_abc 3.10.0
- abc 3.10.0
- types 3.10.0
- typing 3.10.0
- _collections_abc 3.10.2

youknowone · 2022-01-22T07:48:58Z

@moreal I am sorry to keep this unreviewed for long time. Could you please rebase this PR?

youknowone · 2022-01-25T15:46:52Z

vm/src/builtins/mod.rs

+pub(crate) mod unionobject;
+pub use unionobject::PyUnionObject;


typically we don't name ~object for RustPython modules and types. pyunion for module name (to avoid keyword union) and PyUnion for the type name would fit to convention.

youknowone · 2022-01-25T15:47:11Z

vm/src/builtins/unionobject.rs

+    PyRef, PyResult, PyValue, TryFromObject, TypeProtocol, VirtualMachine,
+};
+use std::fmt;
+


Suggested change

youknowone · 2022-01-25T15:48:21Z

vm/src/builtins/unionobject.rs

+
+use super::genericalias;
+
+static CLS_ATTRS: [&str; 1] = ["__module__"];


if there is no specific needs for array, slice also works. (and also const, not to access memory)

Suggested change

static CLS_ATTRS: [&str; 1] = ["__module__"];

const CLS_ATTRS: &[&str] = &["__module__"];

youknowone · 2022-01-25T15:52:09Z

vm/src/builtins/unionobject.rs

+    #[pymethod(magic)]
+    fn repr(&self, vm: &VirtualMachine) -> PyResult<String> {
+        fn repr_item(obj: PyObjectRef, vm: &VirtualMachine) -> PyResult<String> {
+            if obj.is(&vm.ctx.types.none_type) {


Suggested change

if obj.is(&vm.ctx.types.none_type) {

if vm.is_none(&obj) {

https://github.com/RustPython/RustPython/pull/3497\#discussion_r791852425

https://github.com/RustPython/RustPython/pull/3497\#discussion_r791853976

This commits make some tests skip with `unittest.skip`: - `test.test_typing.BaseCallableTests.test_pickle()` - `test.test_typing.BaseCallableTests.test_paramspec()` Because `CollectionsCallableTests` and `TypingCallableTests` inherit tests in `BaseCallableTests` but the tests are failed only in `CollectionsCallableTests`. I tried to use `unittest.expectedFailure` but I couldn't find the way to apply it conditionally.

moreal · 2022-01-29T18:57:37Z

@moreal I am sorry to keep this unreviewed for long time. Could you please rebase this PR?

Hello @youknowone, I have worked for making this pull request pass the check at this time and it finished.
I have a question about the 9fd840d commit. As I wrote on the commit message, there were two failed tests but the tests are in the BaseCallableTests and I couldn't apply unittest.expectedFailure conditionally. So I applied unittest.skip with # TODO: RUSTPYTHON. What do you think about it? That seems not good at the side to skip also two passing tests, not two failed tests.

fanninpm · 2022-01-29T20:37:49Z

I've faced this many times before, and I have a solution. What you can do is go down to CollectionsCallableTests and do something like this:

    # TODO: RUSTPYTHON, WeirdError: for some reason, this test failed in this class
    @unittest.expectedFailure
    def test_whatever(self): # TODO: RUSTPYTHON, remove when this passes
        super().test_whatever() # TODO: RUSTPYTHON, remove when this passes

If you want the answer for your specific case, it's under the cut:

Answer for your specific case

diff --git a/Lib/test/test_typing.py b/Lib/test/test_typing.py
index 330aa1a18..ac97fd781 100644
--- a/Lib/test/test_typing.py
+++ b/Lib/test/test_typing.py
@@ -532,8 +532,6 @@ class BaseCallableTests:
         alias = Callable[[int, str], float]
         self.assertEqual(weakref.ref(alias)(), alias)
 
-    # TODO: RUSTPYTHON
-    @unittest.skip("Failed at CollectionsCallableTests")
     def test_pickle(self):
         Callable = self.Callable
         alias = Callable[[int, str], float]
@@ -575,8 +573,6 @@ class BaseCallableTests:
         self.assertIs(a().__class__, C1)
         self.assertEqual(a().__orig_class__, C1[[int], T])
 
-    # TODO: RUSTPYTHON
-    @unittest.skip("Failed at CollectionsCallableTests")
     def test_paramspec(self):
         Callable = self.Callable
         fullname = f"{Callable.__module__}.Callable"
@@ -646,6 +642,15 @@ class TypingCallableTests(BaseCallableTests, BaseTestCase):
 class CollectionsCallableTests(BaseCallableTests, BaseTestCase):
     Callable = collections.abc.Callable
 
+    # TODO: RUSTPYTHON
+    @unittest.expectedFailure
+    def test_paramspec(self): # TODO: RUSTPYTHON, remove when this passes
+        super().test_paramspec() # TODO: RUSTPYTHON, remove when this passes
+
+    # TODO: RUSTPYTHON, pickle.PicklingError: Can't pickle <class 'collections.abc._CallableGenericAlias'>: it's not found as collections.abc._CallableGenericAlias
+    @unittest.expectedFailure
+    def test_pickle(self): # TODO: RUSTPYTHON, remove when this passes
+        super().test_pickle() # TODO: RUSTPYTHON, remove when this passes
 
 class LiteralTests(BaseTestCase):
     def test_basics(self):

moreal · 2022-01-30T00:50:55Z

@fanninpm I didn't think it works like that 😓 Thank you for your tip and I applied it in a90d18e commit! 🙏🏻

youknowone

Looks great! I left a few subtle comments.

vm/src/builtins/genericalias.rs

vm/src/builtins/pytype.rs

youknowone · 2022-01-31T02:55:19Z

vm/src/builtins/pyunion.rs

+
+    #[pyproperty(magic)]
+    fn parameters(&self) -> PyObjectRef {
+        self.parameters.as_object().to_owned()


Suggested change

self.parameters.as_object().to_owned()

self.parameters.into()

youknowone · 2022-01-31T02:55:34Z

vm/src/builtins/pyunion.rs

+
+    #[pyproperty(magic)]
+    fn args(&self) -> PyObjectRef {
+        self.args.as_object().to_owned()


Suggested change

self.args.as_object().to_owned()

self.args.into()

I tried to apply this suggestion but it failed with the below logs 😿 :

error[E0507]: cannot move out of `self.parameters` which is behind a shared reference --> vm/src/builtins/pyunion.rs:79:9 | 79 | self.parameters.into() | ^^^^^^^^^^^^^^^ move occurs because `self.parameters` has type `pyobjectrc::PyRef<PyTuple>`, which does not implement the `Copy` trait error[E0507]: cannot move out of `self.args` which is behind a shared reference --> vm/src/builtins/pyunion.rs:84:9 | 84 | self.args.into() | ^^^^^^^^^ move occurs because `self.args` has type `pyobjectrc::PyRef<PyTuple>`, which does not implement the `Copy` trait

Suggested change

self.args.as_object().to_owned()

self.args.clone().into()

Adding a clone will work.

vm/src/builtins/pyunion.rs

Co-authored-by: Jeong YunWon <69878+youknowone@users.noreply.github.com>

youknowone · 2022-02-06T09:58:36Z

Thank you for long time work!

moreal · 2022-02-06T10:32:10Z

Thank you for long time work!

I also appreciate your help and passion! 🙇🏻‍♂️

moreal added 9 commits December 5, 2021 17:04

Update _py_abc module to CPython 3.10.0

c060f99

Update abc module to CPython 3.10.0

1b227f8

Update types module to CPython 3.10.0

7da94d8

Introduce PyUnionType

e4d569b

Add test._typed_dict_helper

6f3bcb9

Unmark types.UnionType

72ba40e

Mark & Unmark failed & resolved tests

1163b54

Update typing module to CPython 3.10.0

fad636c

Mark failed tests in test_abc

0317090

moreal mentioned this pull request Dec 11, 2021

Expose types.GenericAlias.__getitem__ #3499

Merged

Merge branch 'main' into update-types-310

28b07ba

moreal marked this pull request as ready for review January 23, 2022 11:40

moreal changed the title ~~Introduce PyUnionType~~ WIP: Introduce PyUnionType Jan 24, 2022

moreal added 3 commits January 25, 2022 23:31

Update _collections_abc module to CPython 3.10.2

0b5ceb8

Unmark tests not failed

0468d0c

Mark failed tests

2f14245

moreal force-pushed the update-types-310 branch from 93d40c7 to 2f14245 Compare January 25, 2022 14:33

youknowone reviewed Jan 25, 2022

View reviewed changes

moreal added 10 commits January 26, 2022 01:48

Rename unionobject to fit to convention

ce91afe

https://github.com/RustPython/RustPython/pull/3497\#discussion_r791852425

Use CLS_ATTRS as constant

426d95f

https://github.com/RustPython/RustPython/pull/3497\#discussion_r791853976

Prefer VirtualMachine.is_none method

ebb9114

Remove useless newline between use statements

6c20e1b

Apply cargo fmt

8bea665

Merge remote-tracking branch 'upstream/main' into update-types-310

9c86e44

Merge branch 'main' into update-types-310

beae3d8

Mark failed test

b3641ab

Unmark passing tests

3ba3bb1

moreal changed the title ~~WIP: Introduce PyUnionType~~ Introduce PyUnionType Jan 29, 2022

Mark expectedFailure specific tests instead to skip

a90d18e

moreal requested a review from youknowone January 30, 2022 13:53

youknowone requested changes Jan 31, 2022

View reviewed changes

moreal and others added 4 commits January 31, 2022 21:55

Remove useless function call

aeb91b8

Co-authored-by: Jeong YunWon <69878+youknowone@users.noreply.github.com>

Use Vec<T>.extend() method

03850d6

Co-authored-by: Jeong YunWon <69878+youknowone@users.noreply.github.com>

Inline a function called once

4af4a5c

Apply cargo fmt

dc968c2

moreal force-pushed the update-types-310 branch from 95b06ee to dc968c2 Compare January 31, 2022 15:13

Apply cargo clippy

078bcd7

moreal requested a review from youknowone January 31, 2022 15:45

moreal added 2 commits February 6, 2022 14:48

Merge remote-tracking branch 'upstream/main' into update-types-310

a2c318a

Use Into trait

d2e3bff

moreal force-pushed the update-types-310 branch from ddcc922 to d2e3bff Compare February 6, 2022 05:51

youknowone approved these changes Feb 6, 2022

View reviewed changes

youknowone merged commit dded3a6 into RustPython:main Feb 6, 2022

		pub(crate) mod unionobject;
		pub use unionobject::PyUnionObject;


		use super::genericalias;

		static CLS_ATTRS: [&str; 1] = ["__module__"];

	static CLS_ATTRS: [&str; 1] = ["__module__"];
	const CLS_ATTRS: &[&str] = &["__module__"];

	self.parameters.as_object().to_owned()
	self.parameters.into()

Introduce PyUnionType #3497

Introduce PyUnionType #3497

Uh oh!

Conversation

moreal commented Dec 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

youknowone commented Jan 22, 2022

Uh oh!

youknowone Jan 25, 2022

Choose a reason for hiding this comment

Uh oh!

youknowone Jan 25, 2022

Choose a reason for hiding this comment

Uh oh!

youknowone Jan 25, 2022

Choose a reason for hiding this comment

Uh oh!

youknowone Jan 25, 2022

Choose a reason for hiding this comment

Uh oh!

moreal commented Jan 29, 2022

Uh oh!

fanninpm commented Jan 29, 2022

Uh oh!

moreal commented Jan 30, 2022

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

youknowone Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

youknowone Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

moreal Jan 31, 2022

Choose a reason for hiding this comment

Uh oh!

youknowone Feb 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

youknowone commented Feb 6, 2022

Uh oh!

moreal commented Feb 6, 2022

Uh oh!

Uh oh!

Introduce `PyUnionType` #3497

Introduce `PyUnionType` #3497

moreal commented Dec 5, 2021 •

edited

Loading

youknowone Feb 6, 2022 •

edited

Loading