GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-44654: Refactor and clean up the union type implementation by serhiy-storchaka · Pull Request #27196 · python/cpython

Conversation

serhiy-storchaka commented Jul 16, 2021 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

https://bugs.python.org/issue44654

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          bpo-44654: Refactor and clean up the union type implementation

97992aa

serhiy-storchaka added skip news needs backport to 3.10 labels

Jul 16, 2021

serhiy-storchaka requested review from gvanrossum, ilevkivskyi and markshannon as code owners

July 16, 2021 17:19

bedevere-bot added the awaiting core review label

Jul 16, 2021

the-knights-who-say-ni added the CLA signed label

Jul 16, 2021

Fidget-Spinner self-requested a review

July 16, 2021 17:34

serhiy-storchaka commented

Jul 16, 2021

View reviewed changes

Comment thread

Objects/unionobject.c

		{
		_Py_IDENTIFIER(__module__);
		PyObject *module;
		if (_PyObject_LookupAttrId(obj, &PyId___module__, &module) < 0) {

serhiy-storchaka Jul 16, 2021

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main effect of this change is that it no longer raises AttributeError for types without __module__.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

corona10 reacted with thumbs up emoji

All reactions

👍 1 reaction

Fidget-Spinner commented Jul 17, 2021

Copy link

Copy Markdown

Member

Serhiy, please see #27203 where I backported another PR's refactoring changes to 3.10. Without that PR this one won't be able to land in 3.10.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Fidget-Spinner mentioned this pull request

Jul 17, 2021

[3.10] bpo-44490: Partially backport GH-26980's refactoring for easier bugfix backports #27203

Closed

Fidget-Spinner approved these changes

Jul 17, 2021

View reviewed changes

Fidget-Spinner left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but someone else should probably take a look too in case I miss anything. I can see it's mostly renaming and moving things around, though some parts are simplifying logic too and those require more scrutiny.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Include/internal/pycore_unionobject.h

		PyAPI_DATA(PyTypeObject) _Py_UnionType;
		PyAPI_FUNC(PyObject ) _Py_union_type_or(PyObject self, PyObject* param);
		PyAPI_DATA(PyTypeObject) _PyUnion_Type;
		#define _PyUnion_Check(op) Py_IS_TYPE(op, &_PyUnion_Type)

Fidget-Spinner Jul 17, 2021

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about naming this _PyUnion_CheckExact instead? I know we don't have subtypes now but that could change in the future (it changed with GenericAlias).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Jul 17, 2021

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we change it in future, the code will still use _PyUnion_Check instead of _PyUnion_CheckExact. So it is better to name it _PyUnion_Check now.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Objects/unionobject.c

		PyObject* b_set = PySet_New(NULL);
		PyObject b_set = PySet_New(((unionobject)b)->args);
		if (b_set == NULL) {
		goto exit;

Fidget-Spinner Jul 17, 2021

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm shocked at how much simpler this is and how it still passes the types.Union[] == typing.Union[] tests. This work because after your Py_RETURN_NOTIMPLEMENTED, Python then looks into typing.Union's __eq__ right?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

gvanrossum Jul 17, 2021

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

gvanrossum approved these changes

Jul 17, 2021

View reviewed changes

gvanrossum left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Serhiy, you are the new Benjamin! Making things better by throwing away code.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot added awaiting merge and removed awaiting core review labels

Jul 17, 2021

serhiy-storchaka merged commit 0fd2737 into python:main

Jul 17, 2021

miss-islington commented Jul 17, 2021

Copy link

Copy Markdown

Contributor

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the awaiting merge label

Jul 17, 2021

serhiy-storchaka deleted the union-cleanup branch

July 17, 2021 19:44

bedevere-bot commented Jul 17, 2021

Copy link

Copy Markdown

GH-27219 is a backport of this pull request to the 3.10 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the needs backport to 3.10 label

Jul 17, 2021

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Jul 17, 2021


          bpo-44654: Refactor and clean up the union type implementation (pytho…

d412101

…nGH-27196)

(cherry picked from commit 0fd2737)

Co-authored-by: Serhiy Storchaka <[email protected]>

serhiy-storchaka pushed a commit that referenced this pull request

Jul 17, 2021


          [3.10] bpo-44654: Refactor and clean up the union type implementation (…

03aad30

…GH-27196) (GH-27219)

(cherry picked from commit 0fd2737)

shihai1991 added a commit to shihai1991/cpython that referenced this pull request

Jul 20, 2021


          Merge remote-tracking branch 'origin/main' into bpo_42035

56e8889

* origin/main: (1146 commits)
  bpo-42064: Finalise establishing sqlite3 global state (pythonGH-27155)
  bpo-44678: Separate error message for discontinuous padding in binascii.a2b_base64 strict mode (pythonGH-27249)
  correct spelling (pythonGH-27076)
  bpo-44524: Add missed __name__ and __qualname__ to typing module objects (python#27237)
  bpo-27513: email.utils.getaddresses() now handles Header objects (python#13797)
  Clean up comma usage in Doc/library/functions.rst (python#27083)
  bpo-42238: Fix small rst issue in NEWS.d/. (python#27238)
  bpo-41972: Tweak fastsearch.h string search algorithms (pythonGH-27091)
  bpo-44340: Add support for building with clang full/thin lto (pythonGH-27231)
  bpo-44661: Update property_descr_set to use vectorcall if possible. (pythonGH-27206)
  bpo-44645: Check for interrupts on any potentially backwards edge (pythonGH-27216)
  bpo-41546: make pprint (like print) not write to stdout when it is None (pythonGH-26810)
  bpo-44554: refactor pdb targets (and internal tweaks) (pythonGH-26992)
  bpo-43086: Add handling for out-of-spec data in a2b_base64 (pythonGH-24402)
  bpo-44561: Update hyperlinks in Doc/distributing/index.rst (python#27032)
  bpo-42355: symtable.get_namespace() now checks whether there are multiple or any namespaces found (pythonGH-23278)
  bpo-44654: Do not export the union type related symbols (pythonGH-27223)
  bpo-44633: Fix parameter substitution of the union type with wrong types. (pythonGH-27218)
  bpo-44654: Refactor and clean up the union type implementation (pythonGH-27196)
  bpo-20291: Fix MSVC warnings in getargs.c (pythonGH-27211)
  ...

Reviewers

gvanrossum

gvanrossum approved these changes

Fidget-Spinner

Fidget-Spinner approved these changes

ilevkivskyi

Awaiting requested review from ilevkivskyi

markshannon

Awaiting requested review from markshannon

markshannon is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

serhiy-storchaka commented Jul 16, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka Jul 16, 2021

Choose a reason for hiding this comment

Uh oh!

Fidget-Spinner commented Jul 17, 2021

Uh oh!

Fidget-Spinner left a comment

Choose a reason for hiding this comment

Uh oh!

Fidget-Spinner Jul 17, 2021

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Jul 17, 2021

Choose a reason for hiding this comment

Uh oh!

Fidget-Spinner Jul 17, 2021

Choose a reason for hiding this comment

Uh oh!

gvanrossum Jul 17, 2021

Choose a reason for hiding this comment

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Jul 17, 2021

Uh oh!

bedevere-bot commented Jul 17, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

serhiy-storchaka commented Jul 16, 2021 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.