bpo-28556: Don't simplify unions at runtime by ilevkivskyi · Pull Request #6841 · python/cpython

ilevkivskyi · 2018-05-14T22:37:20Z

https://bugs.python.org/issue28556

grimreaper

The change itself looks find to me, but its not clear that changing this is the right thing to do. As a general note we're removing old explicit logic and tests rather than just fixing a bug. I didn't notice any discussion of the issue on the mailing list or otherwise. Feel free to direct me if I missed it.

ilevkivskyi · 2018-05-15T11:58:50Z

The change itself looks find to me, but its not clear that changing this is the right thing to do. As a general note we're removing old explicit logic and tests rather than just fixing a bug. I didn't notice any discussion of the issue on the mailing list or otherwise. Feel free to direct me if I missed it.

Some people see this as a bug, see example in the issue linked above. Also this code was a bug magnet because it needs to use issubclass() which is prohibited for various special types.

ilevkivskyi · 2018-05-17T00:30:08Z

@gvanrossum Sorry for being annoying, but if we want to get this in 3.7 we only have time until Monday. Could you please briefly take a look at this PR?

gvanrossum · 2018-05-17T00:40:31Z

No worries. I'll try to review later today.

gvanrossum

A few nits about tests; the substance of the code LGTM. (Nice that it was such a clear separate bit of code. :-)

gvanrossum · 2018-05-17T00:53:50Z

Lib/test/test_typing.py

-        self.assertEqual(u, object)
+        u1 = Union[int, object]
+        u2 = Union[object, int]
+        self.assertEqual(u1, u2)


Maybe also assert that it isn't object?

gvanrossum · 2018-05-17T00:55:35Z

Lib/test/test_typing.py

+    def test_base_class_kept(self):
        u = Union[Employee, Manager]
-        self.assertIs(u, Employee)
+        self.assertIsNot(u, Employee)


Can you check that Employee and Manager are both members of the union?

gvanrossum · 2018-05-17T00:56:53Z

Lib/test/test_typing.py

    def test_union_generalization(self):
        self.assertFalse(Union[str, typing.Iterable[int]] == str)
        self.assertFalse(Union[str, typing.Iterable[int]] == typing.Iterable[int])
-        self.assertTrue(Union[str, typing.Iterable] == typing.Iterable)


How about a more positive test of the expected members?

gvanrossum · 2018-05-17T00:59:34Z

Lib/typing.py

@@ -203,7 +203,7 @@ def _check_generic(cls, parameters):

 def _remove_dups_flatten(parameters):
    """An internal helper for Union creation and substitution: flatten Union's


Union's -> Unions

ilevkivskyi · 2018-05-17T01:55:58Z

@gvanrossum Thanks for review! I addressed all comments, plus I think there is one important point to add: this change breaks semantics of __eq__, now Union[Manager, Employee] != Employee. I can tweak __eq__, but the problem is that generics are cashed, so after once writing List[Employee], then writing List[Union[Manager, Employee]] will return List[Employee] thus again effectively simplifying a union.

gvanrossum · 2018-05-18T04:15:24Z

this change breaks semantics of eq, now Union[Manager, Employee] != Employee. I can tweak eq, but the problem is that generics are cashed, so after once writing List[Employee], then writing List[Union[Manager, Employee]] will return List[Employee] thus again effectively simplifying a union.

Sorry, the tenses are confusing. Is the problem with List[Union[Manager, Employee]] returning List[Employee] happening with this version of the PR or would that happen if you "tweaked" __eq__?

Honestly given that we don't simplify unions I think it's right that Union[Manager, Employee] != Employee.

gvanrossum · 2018-05-18T04:15:46Z

Also I think this requires a news item.

ilevkivskyi · 2018-05-18T04:19:58Z

Honestly given that we don't simplify unions I think it's right that Union[Manager, Employee] != Employee.

OK, This is what happens with this PR, the problem I described would will appear only if I will tweak __eq__.

Also I think this requires a news item.

OK, I will add it.

ilevkivskyi · 2018-05-18T06:21:08Z

asyncio failures are unrelated/flakes.

ilevkivskyi · 2018-05-18T14:14:50Z

I will close and re-open to trigger AppVeyor.

ilevkivskyi · 2018-05-18T22:25:49Z

Restarting AppVeyor again...

gvanrossum

You have to merge this manually because the VSTS tests always fail (they are non-blocking, but Miss Islington doesn't know).

ilevkivskyi · 2018-05-18T23:00:12Z

You have to merge this manually because the VSTS tests always fail (they are non-blocking, but Miss Islington doesn't know).

OK, I see. Will do this now.

miss-islington · 2018-05-18T23:00:41Z

Thanks @ilevkivskyi for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

(cherry picked from commit f65e31f) Co-authored-by: Ivan Levkivskyi <levkivskyi@gmail.com>

bedevere-bot · 2018-05-18T23:01:50Z

GH-6979 is a backport of this pull request to the 3.7 branch.

(cherry picked from commit f65e31f) Co-authored-by: Ivan Levkivskyi <levkivskyi@gmail.com>

Ivan Levkivskyi added 2 commits May 14, 2018 18:28

Don't simplify unions at runtime

624120b

Update docs

3cb8f01

ilevkivskyi added skip issue needs backport to 3.7 labels May 14, 2018

ilevkivskyi requested a review from gvanrossum as a code owner May 14, 2018 22:37

bedevere-bot added the awaiting merge label May 14, 2018

the-knights-who-say-ni added the CLA signed label May 14, 2018

ilevkivskyi added the skip news label May 14, 2018

grimreaper reviewed May 15, 2018

View reviewed changes

gvanrossum reviewed May 17, 2018

View reviewed changes

Address review comments and minor simplification in code

a78dfc1

Add NEWS item

e5eb873

ilevkivskyi removed skip issue skip news labels May 18, 2018

ilevkivskyi changed the title ~~Don't simplify unions at runtime~~ bpo-28556: Don't simplify unions at runtime May 18, 2018

ilevkivskyi closed this May 18, 2018

ilevkivskyi reopened this May 18, 2018

ilevkivskyi closed this May 18, 2018

ilevkivskyi reopened this May 18, 2018

gvanrossum approved these changes May 18, 2018

View reviewed changes

ilevkivskyi merged commit f65e31f into python:master May 18, 2018

bedevere-bot removed the awaiting merge label May 18, 2018

ilevkivskyi deleted the no-simplify-union branch May 18, 2018 23:00

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 18, 2018

bpo-28556: Don't simplify unions at runtime (pythonGH-6841)

f851877

(cherry picked from commit f65e31f) Co-authored-by: Ivan Levkivskyi <levkivskyi@gmail.com>

bedevere-bot removed the needs backport to 3.7 label May 18, 2018

ilevkivskyi added a commit that referenced this pull request May 18, 2018

bpo-28556: Don't simplify unions at runtime (GH-6841) (GH-6979)

09ca590

(cherry picked from commit f65e31f) Co-authored-by: Ivan Levkivskyi <levkivskyi@gmail.com>

		@@ -203,7 +203,7 @@ def _check_generic(cls, parameters):

		def _remove_dups_flatten(parameters):
		"""An internal helper for Union creation and substitution: flatten Union's

Uh oh!

Conversation

ilevkivskyi commented May 14, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grimreaper left a comment

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi commented May 15, 2018

Uh oh!

ilevkivskyi commented May 17, 2018

Uh oh!

gvanrossum commented May 17, 2018

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

gvanrossum May 17, 2018

Choose a reason for hiding this comment

Uh oh!

gvanrossum May 17, 2018

Choose a reason for hiding this comment

Uh oh!

gvanrossum May 17, 2018

Choose a reason for hiding this comment

Uh oh!

gvanrossum May 17, 2018

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi commented May 17, 2018

Uh oh!

gvanrossum commented May 18, 2018

Uh oh!

gvanrossum commented May 18, 2018

Uh oh!

ilevkivskyi commented May 18, 2018

Uh oh!

ilevkivskyi commented May 18, 2018

Uh oh!

ilevkivskyi commented May 18, 2018

Uh oh!

ilevkivskyi commented May 18, 2018

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

ilevkivskyi commented May 18, 2018

Uh oh!

miss-islington commented May 18, 2018

Uh oh!

bedevere-bot commented May 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ilevkivskyi commented May 14, 2018 •

edited by bedevere-bot

Loading