Skip to content

/ cpython

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 17 commits into from Jul 22, 2021
Merged

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

merged 17 commits into from Jul 22, 2021
+137 −16

Conversation

@uriyyo
Copy link
Contributor

@uriyyo uriyyo commented Jul 20, 2021 •
edited by bedevere-bot

https://bugs.python.org/issue44353
@uriyyo
bpo-44353: Implement typing.NewType __call__ method in C
24f5ccb
@uriyyo uriyyo requested review from gvanrossum and rhettinger as code owners Jul 20, 2021
@uriyyo
Copy link
Contributor Author

@uriyyo uriyyo commented Jul 20, 2021

@Fidget-Spinner Could you please review this PR?
@uriyyo
Rename _idfunc to functools_idfunc
cdbd36e
@JelleZijlstra
JelleZijlstra reviewed Jul 20, 2021
View changes
Modules/_functoolsmodule.c Outdated Show resolved Hide resolved
@uriyyo
Add missed Py_INCREF
7b94a7d
@uriyyo uriyyo requested a review from JelleZijlstra Jul 20, 2021
@Fidget-Spinner
Fidget-Spinner reviewed Jul 20, 2021
View changes
Copy link
Contributor

@Fidget-Spinner Fidget-Spinner left a comment

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.
Modules/_functoolsmodule.c Outdated Show resolved Hide resolved
Misc/NEWS.d/next/Library/2021-07-20-18-34-16.bpo-44353.ATuYq4.rst Outdated Show resolved Hide resolved
uriyyo and others added 3 commits Jul 20, 2021
@uriyyo
Make _idfunc static
dd2becd
@uriyyo @Fidget-Spinner
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>
@uriyyo
Align functools_idfunc
e2ac170
@uriyyo uriyyo requested a review from Fidget-Spinner Jul 20, 2021
@JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented 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.
@JelleZijlstra
JelleZijlstra reviewed Jul 20, 2021
View changes
Misc/NEWS.d/next/Library/2021-07-20-18-34-16.bpo-44353.ATuYq4.rst Outdated Show resolved Hide resolved
@uriyyo
Move _idfunc to operator module
d5fcf87
@uriyyo
Copy link
Contributor Author

@uriyyo uriyyo commented Jul 20, 2021

@JelleZijlstra Yup, I agree - operator is better place for such helper
@uriyyo uriyyo requested a review from JelleZijlstra Jul 20, 2021
@uriyyo
Remove _idfunc from functools
3d7b70c
@Fidget-Spinner Fidget-Spinner removed the request for review from rhettinger Jul 21, 2021
@Fidget-Spinner
Fidget-Spinner approved these changes Jul 21, 2021
View changes
Copy link
Contributor

@Fidget-Spinner Fidget-Spinner left a comment

LG. I removed Raymond from the reviewer list since this doesn't touch functools anymore.
@dlax
dlax reviewed Jul 21, 2021
View changes
Modules/_operator.c Outdated Show resolved Hide resolved
@uriyyo
Rename functools_idfunc to operator_idfunc
5293943
@dlax
dlax reviewed Jul 21, 2021
View changes
Modules/_operator.c Outdated Show resolved Hide resolved
Modules/_operator.c Outdated Show resolved Hide resolved
@uriyyo @dlax
Update Modules/_operator.c
ceea1bc 
Co-authored-by: Denis Laxalde <denis@laxalde.org>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jul 22, 2021

Thanks for making the requested changes!

@corona10: please review the changes made to this pull request.
@bedevere-bot bedevere-bot requested a review from corona10 Jul 22, 2021
@uriyyo
Fix typos
1cad8c6
@Fidget-Spinner
Copy link
Contributor

@Fidget-Spinner Fidget-Spinner commented Jul 22, 2021 •
edited

@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
corona10 requested changes Jul 22, 2021
View changes
Copy link
Member

@corona10 corona10 left a comment

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
Copy link

@bedevere-bot bedevere-bot commented 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
Add tests to cover _typing and typing modules
2c51ef7
@uriyyo
Copy link
Contributor Author

@uriyyo uriyyo commented Jul 22, 2021

I have made the requested changes; please review again
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jul 22, 2021

Thanks for making the requested changes!

@corona10: please review the changes made to this pull request.
@bedevere-bot bedevere-bot requested a review from corona10 Jul 22, 2021
uriyyo added 2 commits Jul 22, 2021
@uriyyo
Merge remote-tracking branch 'upstream/main' into fix-issue-44353-fas…
6e4f684 
…ter-call

# Conflicts:
#	Lib/test/test_typing.py
@uriyyo
Update test_or
6f990c0
@corona10
corona10 approved these changes Jul 22, 2021
View changes
Copy link
Member

@corona10 corona10 left a comment

LGTM about module architecture.
Now I am turning over reviews to typing module experts
@ambv ambv merged commit 96c4cbd into python:main Jul 22, 2021
13 checks passed
13 checks passed
@github-actions
Docs
Details
@github-actions
Check for source changes
Details
@github-actions
Check if generated files are up to date
Details
@github-actions
Windows (x86)
Details
@github-actions
Windows (x64)
Details
@github-actions
macOS
Details
@github-actions
Ubuntu
Details
@github-actions
Ubuntu SSL tests with OpenSSL
Details
@github-actions
Address sanitizer Address sanitizer
Details
Azure Pipelines PR #20210722.16 succeeded
Details
@travis-ci
Travis CI - Pull Request Build Passed
Details
@bedevere-bot
bedevere/issue-number Issue number 44353 found
Details
@bedevere-bot
bedevere/news News entry found in Misc/NEWS.d
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jul 22, 2021

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

@ambv ambv commented Jul 22, 2021

Thanks, @uriyyo!
@pablogsal
Copy link
Member

@pablogsal pablogsal commented 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
Copy link
Contributor Author

@uriyyo uriyyo commented Jul 23, 2021 •
edited

@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)
@uriyyo uriyyo mentioned this pull request Jul 23, 2021
Merged
ambv pushed a commit that referenced this pull request Jul 23, 2021
@uriyyo
bpo-44353: Fix memory leak introduced by GH-27262 (GH-27305)
8f42106
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dlax dlax

@corona10 corona10

@Fidget-Spinner Fidget-Spinner

@gvanrossum gvanrossum

@JelleZijlstra JelleZijlstra

@python/windows-team windows-team

Assignees
No one assigned
Labels
CLA signed type-enhancement
Projects
None yet
Milestone
No milestone
9 participants
@uriyyo @JelleZijlstra @bedevere-bot @Fidget-Spinner @ambv @pablogsal @dlax @corona10 @the-knights-who-say-ni