bpo-44353: Implement typing.NewType __call__ method in C by uriyyo · Pull Request #27262 · python/cpython

uriyyo · Jul 20, 2021

https://bugs.python.org/issue44353

uriyyo · Jul 20, 2021

@Fidget-Spinner Could you please review this PR?

Fidget-Spinner

LGTM, but maybe we should wait for the functools maintainers in case they don't want it in that module.

Also, thanks a bunch for reviewing this too Jelle :). It's nice to have more reviewers around for typing.

Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>

JelleZijlstra · Jul 20, 2021

@Fidget-Spinner feel free to request my review on other typing PRs!

I don't have a strong opinion on where to put the helper function. I feel like operator actually makes the most sense.

uriyyo · Jul 20, 2021

@JelleZijlstra Yup, I agree - operator is better place for such helper

Fidget-Spinner

LG. I removed Raymond from the reviewer list since this doesn't touch functools anymore.

Co-authored-by: Denis Laxalde <denis@laxalde.org>

uriyyo · Jul 22, 2021

I have made the requested changes; please review again

bedevere-bot · Jul 22, 2021

Thanks for making the requested changes!

@corona10: please review the changes made to this pull request.

Fidget-Spinner · Jul 22, 2021

@Fidget-Spinner do we need backport it to python 3.10?

Probably not. Usually only doc/bugfixes can be backported. Performance improvements are usually not backported, unless the old version has performance that was so bad it caused a security vulnerability/bug/crash (e.g. regex DOS). We are too late into the 3.10 dev cycle to backport an improvement.

Also, thinking abit more, @corona10 is right about adding a separate module for this. I know I've said before that we should be conservative about adding more typing stuff in C, but if it's a small simple optimization like the current PR (which doesn't break compatibility with things like PyPy), I'm +1. In case we need to add more things in the future, it's better to create a new module now and not pollute the other modules.

corona10

Once you added the accelerated module, you have to carefully manage both versions.
To manage the identical implementation within different languages, you should update unit tests to run both versions.

Please refer how other modules did.

cpython/Lib/test/test_statistics.py

Line 182 in c05a790

py_statistics = import_helper.import_fresh_module('statistics',

bedevere-bot · Jul 22, 2021

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

uriyyo · Jul 22, 2021

I have made the requested changes; please review again

bedevere-bot · Jul 22, 2021

Thanks for making the requested changes!

@corona10: please review the changes made to this pull request.

…ter-call # Conflicts: # Lib/test/test_typing.py

corona10

LGTM about module architecture.
Now I am turning over reviews to typing module experts

bedevere-bot · Jul 22, 2021

@ambv: Please replace # with GH- in the commit message next time. Thanks!

ambv · Jul 22, 2021

Thanks, @uriyyo!

pablogsal · Jul 23, 2021

This PR was introduced reference leaks and currently all refleak buildbots are red:

Example:

https://buildbot.python.org/all/#/builders/205/builds/102/steps/5/logs/stdio

Ran 367 tests in 0.198s
OK (skipped=1)
.
test_typing leaked [220, 220, 220] references, sum=660
test_typing leaked [69, 68, 68] memory blocks, sum=205
1 test failed again:
    test_typing
== Tests result: FAILURE then FAILURE ==

Per our buildbot policy, if a fix is not performed in 24 h we will need to revert

uriyyo · Jul 23, 2021

@pablogsal Looks like this because of this line

cpython/Modules/_typingmodule.c

Line 26 in 17575f7

Py_INCREF(x);

I will open PR to fix it.

UPD: that no problem of a memory leak

UPD: Root cause of memory leak - #27305 (comment)

bpo-44353: Implement typing.NewType __call__ method in C

24f5ccb

uriyyo requested review from gvanrossum and rhettinger as code owners July 20, 2021 15:45

bedevere-bot added the awaiting review label Jul 20, 2021

the-knights-who-say-ni added the CLA signed label Jul 20, 2021

Rename _idfunc to functools_idfunc

cdbd36e

JelleZijlstra reviewed Jul 20, 2021

View reviewed changes

Comment thread Modules/_functoolsmodule.c Outdated

Add missed Py_INCREF

7b94a7d

uriyyo requested a review from JelleZijlstra July 20, 2021 15:59

Fidget-Spinner reviewed Jul 20, 2021

View reviewed changes

Comment thread Modules/_functoolsmodule.c Outdated

Comment thread Misc/NEWS.d/next/Library/2021-07-20-18-34-16.bpo-44353.ATuYq4.rst Outdated

uriyyo and others added 3 commits July 20, 2021 19:01

Make _idfunc static

dd2becd

Update Misc/NEWS.d/next/Library/2021-07-20-18-34-16.bpo-44353.ATuYq4.rst

084e77c

Co-authored-by: Ken Jin <28750310+Fidget-Spinner@users.noreply.github.com>

Align functools_idfunc

e2ac170

uriyyo requested a review from Fidget-Spinner July 20, 2021 16:05

Fidget-Spinner added the type-feature A feature request or enhancement label Jul 20, 2021

JelleZijlstra reviewed Jul 20, 2021

View reviewed changes

Comment thread Misc/NEWS.d/next/Library/2021-07-20-18-34-16.bpo-44353.ATuYq4.rst Outdated

Move _idfunc to operator module

d5fcf87

uriyyo requested a review from JelleZijlstra July 20, 2021 17:42

Remove _idfunc from functools

3d7b70c

Fidget-Spinner removed the request for review from rhettinger July 21, 2021 10:33

Fidget-Spinner approved these changes Jul 21, 2021

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Jul 21, 2021

dlax reviewed Jul 21, 2021

View reviewed changes

Comment thread Modules/_operator.c Outdated

Rename functools_idfunc to operator_idfunc

5293943

dlax reviewed Jul 21, 2021

View reviewed changes

Comment thread Modules/_operator.c Outdated

Comment thread Modules/_operator.c Outdated

Update Modules/_operator.c

ceea1bc

Co-authored-by: Denis Laxalde <denis@laxalde.org>

bedevere-bot added awaiting change review and removed awaiting changes labels Jul 22, 2021

bedevere-bot requested a review from corona10 July 22, 2021 08:37

Fix typos

1cad8c6

corona10 requested changes Jul 22, 2021

View reviewed changes

bedevere-bot removed the awaiting change review label Jul 22, 2021

bedevere-bot added the awaiting changes label Jul 22, 2021

Add tests to cover _typing and typing modules

2c51ef7

bedevere-bot added awaiting change review and removed awaiting changes labels Jul 22, 2021

bedevere-bot requested a review from corona10 July 22, 2021 16:26

uriyyo added 2 commits July 22, 2021 19:28

Merge remote-tracking branch 'upstream/main' into fix-issue-44353-fas…

6e4f684

…ter-call # Conflicts: # Lib/test/test_typing.py

Update test_or

6f990c0

corona10 approved these changes Jul 22, 2021

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jul 22, 2021

ambv merged commit 96c4cbd into python:main Jul 22, 2021

bedevere-bot removed the awaiting merge label Jul 22, 2021

uriyyo mentioned this pull request Jul 23, 2021

bpo-44353: Fix memory leak introduced by #27262 #27305

Merged

ambv pushed a commit that referenced this pull request Jul 23, 2021

bpo-44353: Fix memory leak introduced by GH-27262 (GH-27305)

8f42106

Search code, repositories, users, issues, pull requests...

Uh oh!

Conversation

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

Uh oh!

Uh oh!

uriyyo commented Jul 20, 2021

Uh oh!

Uh oh!

Fidget-Spinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JelleZijlstra commented Jul 20, 2021

Uh oh!

Uh oh!

uriyyo commented Jul 20, 2021

Uh oh!

Fidget-Spinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

uriyyo commented Jul 22, 2021

Uh oh!

bedevere-bot commented Jul 22, 2021

Uh oh!

Fidget-Spinner commented Jul 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corona10 left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jul 22, 2021

Uh oh!

uriyyo commented Jul 22, 2021

Uh oh!

bedevere-bot commented Jul 22, 2021

Uh oh!

corona10 left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jul 22, 2021

Uh oh!

ambv commented Jul 22, 2021

Uh oh!

pablogsal commented Jul 23, 2021

Uh oh!

uriyyo commented Jul 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

uriyyo commented Jul 20, 2021 •

edited by bedevere-bot

Loading

Fidget-Spinner commented Jul 22, 2021 •

edited

Loading

uriyyo commented Jul 23, 2021 •

edited

Loading