GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-96348: Deprecate the 3-arg signature of coroutine.throw, generator.throw and agen.athrow by ofey404 · Pull Request #96428 · python/cpython

Conversation

@ofey404

@ofey404 ofey404 commented Aug 30, 2022 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Add an deprecation warning, when nargs > 1 in Objects/genobject.c:gen_throw.

  • Also change corresponding docstrings.
  • Issue: Deprecate gen.throw(typ, val, tb) #96348

Sorry, something went wrong.

Uh oh!

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

All reactions
@ofey404
feat: Deprecate gen.throw(typ, val, tb)
b388f9a
@ofey404
ofey404 requested a review from markshannon as a code owner August 30, 2022 13:49
@bedevere-bot

bedevere-bot commented Aug 30, 2022

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot added the awaiting review label Aug 30, 2022
@ofey404

ofey404 commented Aug 30, 2022

Copy link
Copy Markdown
Contributor Author

There is a problem: raise DeprecationWarning will break many lib tests, since they use the (type, val, tb) exception representation.

Eg:

cpython/Lib/contextlib.py

Lines 154 to 155 in 22ed523

try:
self.gen.throw(typ, value, traceback)

All reactions

Sorry, something went wrong.

Uh oh!

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

@ofey404 ofey404 mentioned this pull request Aug 30, 2022
Deprecate gen.throw(typ, val, tb) #96348
Closed
@gvanrossum
gvanrossum self-requested a review August 30, 2022 17:43
gvanrossum
gvanrossum reviewed Aug 30, 2022
View reviewed changes

@gvanrossum 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.

One nit. Too bad about the failing tests, let's brainstorm what to do about them:

  • We could just fix them, replacing .throw(A, B, C) with .throw(B).
  • We could add filterwarnings() calls to the offending tests (I don't recall how to do that but there are probable plenty of examples).
  • We could report a SilentDeprecationWarning, then gradually fix the tests, and then later change it into a DeprecationWarning.
  • We could first fix the tests, in batches, and then land this PR.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Objects/genobject.c Resolved

Uh oh!

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

@gvanrossum

gvanrossum commented Aug 30, 2022

Copy link
Copy Markdown
Member

We could report a SilentDeprecationWarning, then gradually fix the tests, and then later change it into a DeprecationWarning.

Whoops, there's no such thing. Maybe it once existed, but it doesn't now, so ignore this option. :-)

All reactions

Sorry, something went wrong.

Uh oh!

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

@ofey404

ofey404 commented Aug 31, 2022

Copy link
Copy Markdown
Contributor Author

One nit. Too bad about the failing tests, let's brainstorm what to do about them:

Let's estimate the problem size, by finding with regex throw\(.*,.*,.*\)

There are 51 results in 12 files.

  • 3 file is under Lib
  • 1 is in Lib/lib2to3/fixes
  • The rest are tests.
  • 51 results in 12 files.txt

  • We could just fix them, replacing .throw(A, B, C) with .throw(B).
  • We could first fix the tests, in batches, and then land this PR.

I prefer fixing them in this PR, for the fix need to be verified by raising an error. Otherwise the batch fix might miss something.

  • However, the batch plan would be clearer, in Issue and PR history.

We could add filterwarnings() calls to the offending tests (I don't recall how to do that but there are probable plenty of examples).

filterwarnings() call might be a safety valve, if I am not quite confident about how some tests work.

All reactions

Sorry, something went wrong.

Uh oh!

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

@gvanrossum

gvanrossum commented Aug 31, 2022

Copy link
Copy Markdown
Member

Okay, make sure you consider this.

Also, some of the hits may be tests for this very feature, those should not be "fixed" but you may have to add a with self.warns() on those.

All reactions

Sorry, something went wrong.

Uh oh!

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

@ofey404 @gvanrossum
Update Objects/genobject.c
713d77a
Co-authored-by: Guido van Rossum <[email protected]>
@bedevere-bot

bedevere-bot commented Aug 31, 2022

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

All reactions

Sorry, something went wrong.

Uh oh!

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

@ofey404
fix clear cases
fcfb65e
@ofey404
ofey404 requested a review from 1st1 as a code owner September 1, 2022 05:26
@ofey404

ofey404 commented Sep 1, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

I run ./python -m test,and fixed clear cases, changed them to .throw(val) form.

However, there are still some DeprecationWarnings need to be handle. As @iritkatriel said in comment of 96348, some of the tests need to stay.

1. Generator's doc test

I think we should filter the DeprecationWarning here, for it's testing exactly the deprecated API.

cpython/Lib/test/test_generators.py

Lines 2116 to 2130 in 0cd33e1

>>> g.throw(ValueError, ValueError(1)) # value+matching type
caught ValueError (1)
>>> g.throw(ValueError, TypeError(1)) # mismatched type, rewrapped
caught ValueError (1)
>>> g.throw(ValueError, ValueError(1), None) # explicit None traceback
caught ValueError (1)
>>> g.throw(ValueError(1), "foo") # bad args
Traceback (most recent call last):
...
TypeError: instance exception may not have a separate value
>>> g.throw(ValueError, "foo", 23) # bad args

2. Lib unittest.case

It happens here:

cpython/Lib/unittest/case.py

Lines 236 to 237 in 0cd33e1

with self:
callable_obj(*args, **kwargs)

The output is like:

/../cpython/Lib/unittest/case.py:237: DeprecationWarning: the (type, val, tb) exception representationis deprecated, and may be removed in a future version of Python.
  callable_obj(*args, **kwargs)

I am not sure what callable_obj does here, and maybe filtering this warning requires a change in standard library of python?

All reactions

Sorry, something went wrong.

Uh oh!

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

iritkatriel
iritkatriel reviewed Sep 1, 2022
View reviewed changes
Comment thread Objects/genobject.c Resolved

Uh oh!

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

iritkatriel
iritkatriel reviewed Sep 1, 2022
View reviewed changes
Comment thread Objects/genobject.c Resolved

Uh oh!

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

iritkatriel
iritkatriel reviewed Sep 1, 2022
View reviewed changes
Comment thread Misc/NEWS.d/next/Core and Builtins/2022-08-31-18-46-13.gh-issue-96348.xzCoTP.rst Resolved

Uh oh!

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

@iritkatriel

iritkatriel commented Sep 1, 2022

Copy link
Copy Markdown
Member
/../cpython/Lib/unittest/case.py:237: DeprecationWarning: the (type, val, tb) exception representationis deprecated, and may be removed in a future version of Python.
  callable_obj(*args, **kwargs)

I am not sure what callable_obj does here, and maybe filtering this warning requires a change in standard library of python?

Look at the code in Lib/unittest/case.py. This is part of the assertRaises mechanism. The callable_obj and the args passed to it are defined by the test, so this should be dealt with in the particular test and not here in the test framework.

All reactions

Sorry, something went wrong.

Uh oh!

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

@iritkatriel

iritkatriel commented Sep 1, 2022

Copy link
Copy Markdown
Member

I am not sure what callable_obj does here, and maybe filtering this warning requires a change in standard library of python?

Look at the code in Lib/unittest/case.py. This is part of the assertRaises mechanism. The callable_obj and the args passed to it are defined by the test, so this should be dealt with in the particular test and not here in the test framework.

The test is this one. It's testing the types of the triplet, so we need to keep it and suppress the deprecation warning.

    def test_future_iter_throw(self):
        fut = self._new_future(loop=self.loop)
        fi = iter(fut)
        self.assertRaises(TypeError, fi.throw, 
                          Exception, Exception("elephant"), 32)
        self.assertRaises(TypeError, fi.throw,
                          Exception("elephant"), Exception("elephant"))
        self.assertRaises(TypeError, fi.throw, list)
All reactions

Sorry, something went wrong.

Uh oh!

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

ofey404 and others added 2 commits September 3, 2022 13:46
@ofey404 @iritkatriel
Update Misc/NEWS.d/next/Core and Builtins/2022-08-31-18-46-13.gh-issu…
073aa62
…e-96348.xzCoTP.rst

Co-authored-by: Irit Katriel <[email protected]>
@ofey404
Filter remaining warnings.
1c6ed30
@ofey404
ofey404 requested a review from asvetlov as a code owner September 3, 2022 09:02
@ofey404

ofey404 commented Sep 3, 2022

Copy link
Copy Markdown
Contributor Author

Okay, handled the remaining warnings.

Tested with those scripts:

./python -m test -j16 > test.log; cat test.log | grep "DeprecationWarning: the (type, val, tb) exception representation is deprecated"
# Come out with nothing

./python -Werror -m unittest -v test.test_asyncio.test_futures.PyFutureTests.test_future_iter_throw test.test_generators
# OK

Besides, should I squash all commits into one, when all the reviews are done?

All reactions

Sorry, something went wrong.

Uh oh!

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

@gvanrossum

gvanrossum commented Sep 3, 2022

Copy link
Copy Markdown
Member

Don’t squash please! It makes review harder. We will squash upon merge.

All reactions

Sorry, something went wrong.

Uh oh!

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

kumaraditya303
kumaraditya303 requested changes Sep 6, 2022
View reviewed changes

@kumaraditya303 kumaraditya303 left a comment

Copy link
Copy Markdown
Contributor

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.

This is a significant change, it should be properly documented in 3.12 What's New.

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-bot bedevere-bot added awaiting core review and removed awaiting review labels Sep 6, 2022
@ofey404
Add deprecated info in documentation.
4881855
@ofey404
Merge branch 'main' into ofey404/deprecate-get-throw
53c74db
iritkatriel
iritkatriel reviewed Sep 25, 2022
View reviewed changes
Comment thread Misc/NEWS.d/next/Core and Builtins/2022-08-31-18-46-13.gh-issue-96348.xzCoTP.rst Resolved

Uh oh!

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

@ofey404 @iritkatriel
Update Misc/NEWS.d/next/Core and Builtins/2022-08-31-18-46-13.gh-issu…
93db966
…e-96348.xzCoTP.rst

Co-authored-by: Irit Katriel <[email protected]>
kumaraditya303
kumaraditya303 requested changes Sep 25, 2022
View reviewed changes
Comment thread Lib/test/test_asyncgen.py Resolved

Uh oh!

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

iritkatriel and others added 4 commits September 25, 2022 20:44
@iritkatriel
assert that deprecation warning is emitted
0e56eb9
@ofey404
stage, exploring how to capture deprecation warning rather than supre…
8738273
…ss them.
@ofey404
Revert "stage, exploring how to capture deprecation warning rather th…
31b2e9f
…an supress them."

This reverts commit 8738273.
@ofey404
Merge pull request #6 from iritkatriel/pr96428
8b701d6
assert that deprecation warning is emitted
@iritkatriel iritkatriel added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 28, 2022
@bedevere-bot

bedevere-bot commented Sep 28, 2022

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit 8b701d6 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Sep 28, 2022
@iritkatriel
Merge branch 'main' into ofey404/deprecate-get-throw
70e1f28
iritkatriel
iritkatriel reviewed Sep 28, 2022
View reviewed changes
Comment thread Lib/test/test_asyncio/test_futures.py Resolved

Uh oh!

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

@iritkatriel
fix test
ed2e83a
@iritkatriel

iritkatriel commented Sep 28, 2022

Copy link
Copy Markdown
Member

It seems that my tests exposed some problem - there are 3 versions of futures, and only one of them is emitting the deprecation warning?

All reactions

Sorry, something went wrong.

Uh oh!

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

@iritkatriel

iritkatriel commented Sep 28, 2022

Copy link
Copy Markdown
Member

Looking at the code, if futures._CFuture is fixed (made to raise the deprecation warning) then that will fix CSubFuture as well.

All reactions

Sorry, something went wrong.

Uh oh!

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

@iritkatriel

iritkatriel commented Sep 28, 2022

Copy link
Copy Markdown
Member

So I think we need one more deprecation warning from FutureIter_throw in
Modules/_asynciomodule.c.

All reactions

Sorry, something went wrong.

Uh oh!

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

@ofey404
Add a warning in FutureIter_throw
03fc6e0
@ofey404

ofey404 commented Sep 29, 2022

Copy link
Copy Markdown
Contributor Author

It seems that my tests exposed some problem - there are 3 versions of futures, and only one of them is emitting the deprecation warning?

The warning is added.


But I still feel a little dizzy about it - How do you know that we should add a test in test_futures.py? Is python future implemented by coroutine object or async generator?

  • My guess: According to the doc, so test_future.py is testing the high-level interface of coroutine object, from that we know that we should add a test on iter(future).throw.

The concurrent.futures module provides a high-level interface for asynchronously executing callables.
concurrent.futures — Launching parallel tasks

All reactions

Sorry, something went wrong.

Uh oh!

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

@iritkatriel

iritkatriel commented Sep 29, 2022

Copy link
Copy Markdown
Member

@kumaraditya303 How does it look now? (I'm a little dizzy by now as well).

All reactions

Sorry, something went wrong.

Uh oh!

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

kumaraditya303
kumaraditya303 approved these changes Sep 30, 2022
View reviewed changes

@kumaraditya303 kumaraditya303 left a comment

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

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

All reactions
@kumaraditya303

kumaraditya303 commented Sep 30, 2022

Copy link
Copy Markdown
Contributor

You only need to add deprecation warning in _CFuture since it is implemented in C. The pure Python versions uses regular generators so they will automatically emit warning since warning is added to generator.throw.

cpython/Lib/asyncio/futures.py

Lines 290 to 298 in 9a11ed8

def __await__(self):
if not self.done():
self._asyncio_future_blocking = True
yield self # This tells Task to wait for completion.
if not self.done():
raise RuntimeError("await wasn't used with future")
return self.result() # May raise too.
__iter__ = __await__ # make compatible with 'yield from'.

All reactions

Sorry, something went wrong.

Uh oh!

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

@iritkatriel iritkatriel added 3.12 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Sep 30, 2022
@iritkatriel iritkatriel changed the title gh-96348: Deprecate the 3-arg signature of coroutine.throw and generator.throw gh-96348: Deprecate the 3-arg signature of coroutine.throw, generator.throw and agen.athrow Sep 30, 2022
@iritkatriel
iritkatriel merged commit 83a3de4 into python:main Sep 30, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Sep 30, 2022
merwok
merwok reviewed Sep 30, 2022
View reviewed changes
Comment thread Doc/reference/datamodel.rst

.. versionchanged:: 3.12

The second signature \(type\[, value\[, traceback\]\]\) is deprecated and

@merwok merwok Sep 30, 2022 •
edited
Loading

Uh oh!

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

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.

For future PRs, not that code should generally be marked up like

      The second signature `(type[, value[, traceback]])` is deprecated and

Sorry, something went wrong.

Uh oh!

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

All reactions
serhiy-storchaka pushed a commit to serhiy-storchaka/cpython that referenced this pull request Oct 2, 2022
@ofey404 @serhiy-storchaka
pythongh-96348: Deprecate the 3-arg signature of coroutine.throw and …
3c84af2
…generator.throw (pythonGH-96428)
@romanz romanz mentioned this pull request May 21, 2025
chore(core): fix deprecation warning for input_flow.throw(...) trezor/trezor-firmware#5067
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@merwok merwok merwok left review comments
@iritkatriel iritkatriel iritkatriel approved these changes
@gvanrossum gvanrossum gvanrossum left review comments
@kumaraditya303 kumaraditya303 kumaraditya303 approved these changes
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner
@1st1 1st1 Awaiting requested review from 1st1 1st1 is a code owner
@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

3.12 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs)

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

@ofey404 @bedevere-bot @gvanrossum @iritkatriel @kumaraditya303 @merwok
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.