Introduce `PyUnionType` by moreal · Pull Request #3497 · RustPython/RustPython

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

+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

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


Suggested change

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

+
+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

+    #[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.

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

+
+    #[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

+
+    #[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.

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()

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

moreal commented Dec 5, 2021 •

edited

Loading

youknowone Feb 6, 2022 •

edited

Loading