Skip to content
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

Deprecate gen.throw(typ, val, tb) #96348

Closed
gvanrossum opened this issue Aug 28, 2022 · 23 comments
Closed

Deprecate gen.throw(typ, val, tb) #96348

gvanrossum opened this issue Aug 28, 2022 · 23 comments
Assignees
Labels
3.12 bugs and security fixes easy interpreter-core (Objects, Python, Grammar, and Parser dirs) release-blocker type-feature A feature request or enhancement

Comments

@gvanrossum
Copy link
Member

CC: @iritkatriel

Issue GH-89874 gets rid of exception triples where it makes sense. I don't know if it was considered, but another place where such triples are supported is the gen.throw(). It allows passing either an exception, or a (type, value, traceback) triple where the value and traceback are optional. I think it makes sense to keep allowing passing an exception type, but I don't see the use case for passing separate type and value. We could deprecate this easily by adding a warning to gen_throw() when more than one argument is present.

Thoughts?

@gvanrossum gvanrossum added the type-feature A feature request or enhancement label Aug 28, 2022
@rhettinger
Copy link
Contributor

rhettinger commented Aug 28, 2022

This would be a nice simplification. AFAICT, the three argument form is rarely used, so deprecating that variant won't be disruptive. Also, when the signature becomes METH_O, calls to gen.throw will get faster.

@iritkatriel iritkatriel changed the title Deprecate gen.throw(typ, val, exc) Deprecate gen.throw(typ, val, tb) Aug 28, 2022
@iritkatriel
Copy link
Member

Yes, this would be good. The triplet is not just wasteful, it's also ambiguous when val.__traceback__ != tb.

@iritkatriel iritkatriel self-assigned this Aug 28, 2022
@gvanrossum
Copy link
Member Author

I wonder if this could be marked as an "easy" issue for someone with moderate C skills to tackle? All you have to do (for now) is issue a deprecation warning if the arg count is larger than 1. (And update some docs.)

@iritkatriel iritkatriel removed their assignment Aug 28, 2022
@iritkatriel iritkatriel added interpreter-core (Objects, Python, Grammar, and Parser dirs) easy 3.12 bugs and security fixes release-blocker labels Aug 28, 2022
@iritkatriel
Copy link
Member

Marked as easy, but also as release-blocker for 3.12 so we won't forget.

@ofey404
Copy link
Contributor

ofey404 commented Aug 29, 2022

I wonder if this could be marked as an "easy" issue for someone with moderate C skills to tackle? All you have to do (for now) is issue a deprecation warning if the arg count is larger than 1. (And update some docs.)

Does it locate in Objects/iterobject.c? I'm trying to become a core developer and willing to help.

static PyObject *
anextawaitable_throw(anextawaitableobject *obj, PyObject *arg) {
return anextawaitable_proxy(obj, "throw", arg);
}

And the (type, value, traceback) triple documentation:

help(a.throw)
Help on built-in function throw:

throw(...) method of builtins.generator instance
    throw(typ[,val[,tb]]) -> raise exception in generator,
    return next yielded value or raise StopIteration.

@gvanrossum
Copy link
Member Author

No, I'm looking at gen_throw in genobject.c. Looks like you found another instance of the same problem!

@ofey404
Copy link
Contributor

ofey404 commented Aug 29, 2022

I got it, here is the genobject.c:gen_throw doc:

cpython/Objects/genobject.c

Lines 1145 to 1150 in af368a7

PyDoc_STRVAR(coro_throw_doc,
"throw(value)\n\
throw(type[,value[,traceback]])\n\
\n\
Raise exception in coroutine, return next iterated value or raise\n\
StopIteration.");

Besides, there is another occurrence in genobject.c, the async_gen_athrow:

cpython/Objects/genobject.c

Lines 1539 to 1540 in af368a7

PyDoc_STRVAR(async_athrow_doc,
"athrow(typ[,val[,tb]]) -> raise exception in generator.");

And how to throw deprecation warning from cpython c code? I searched the web but didn't find much useful material.

@arhadthedev
Copy link
Member

The keywords here are PyExc_DeprecationWarning and PyErr_WarnEx (I've briefly seen these names before but didn't use them in practice). Here are relevant examples, though I don't know how to determine the last parameter of the function (7 in two examples, 2 in the third):

PyMODINIT_FUNC
PyInit_spwd(void)
{
if (PyErr_WarnEx(PyExc_DeprecationWarning,
"'spwd' is deprecated and slated for removal in "
"Python 3.13",
7)) {
return NULL;
}
return PyModuleDef_Init(&spwdmodule);
}

cpython/Modules/nismodule.c

Lines 524 to 534 in 804f252

PyMODINIT_FUNC
PyInit_nis(void)
{
if (PyErr_WarnEx(PyExc_DeprecationWarning,
"'nis' is deprecated and slated for removal in "
"Python 3.13",
7)) {
return NULL;
}
return PyModuleDef_Init(&nismodule);
}

static PyObject *
_asyncio_Future_cancel_impl(FutureObj *self, PyObject *msg)
/*[clinic end generated code: output=3edebbc668e5aba3 input=925eb545251f2c5a]*/
{
if (msg != Py_None) {
if (PyErr_WarnEx(PyExc_DeprecationWarning,
"Passing 'msg' argument to Future.cancel() "
"is deprecated since Python 3.11, and "
"scheduled for removal in Python 3.14.",
2))
{
return NULL;
}
}
ENSURE_FUTURE_ALIVE(self)
return future_cancel(self, msg);
}

@ofey404
Copy link
Contributor

ofey404 commented Aug 29, 2022

The keywords here are PyExc_DeprecationWarning and PyErr_WarnEx (I've briefly seen these names before but didn't use them in practice).

Besides, I wonder are there any unit test catching depreciated warning? I randomly delete one warning in cpython source code, but all test suite still passed.

@CharlieZhao95
Copy link
Contributor

Besides, I wonder are there any unit test catching depreciated warning? I randomly delete one warning in cpython source code, but all test suite still passed.

You might refer to a historical commit(beea26b) from vstinner, which uses assertWarns to check if the DeprecationWarning is thrown.

with self.assertWarns(DeprecationWarning):
    do_something()

Also, you can check the git log of the deprecated module to see if the unit test was also committed :)

@gvanrossum
Copy link
Member Author

I don't know how to determine the last parameter of the function (7 in two examples, 2 in the third)

This is the number of (Python) stack levels to skip when computing the file name and line number to appear in the message. I'd start setting it to 1 and see what warning message actually gets printed. Increase it until the filename is reasonable.

@kumaraditya303
Copy link
Contributor

While we are at it, the same should be done for async generators too.

@ofey404
Copy link
Contributor

ofey404 commented Aug 30, 2022

I opened a pull request: #96428

One problem: raise DeprecationWarning will break so 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)

@gvanrossum
Copy link
Member Author

Would it be reasonable to fix the failing tests in the same PR? Or are there too many? They should all just switch to throwing the middle value of the triple (the exception instance).

@iritkatriel
Copy link
Member

You'll get pretty far with the tests if you fix this one:

--- a/Lib/contextlib.py
+++ b/Lib/contextlib.py
@@ -152,7 +152,7 @@ def __exit__(self, typ, value, traceback):
                 # tell if we get the same exception back
                 value = typ()
             try:
-                self.gen.throw(typ, value, traceback)
+                self.gen.throw(value)
             except StopIteration as exc:
                 # Suppress StopIteration *unless* it's the same exception that
                 # was passed to throw().  This prevents a StopIteration

@iritkatriel iritkatriel added 🔨 test-with-buildbots Test PR w/ buildbots; report in status section and removed 🔨 test-with-buildbots Test PR w/ buildbots; report in status section labels Aug 30, 2022
@ofey404
Copy link
Contributor

ofey404 commented Aug 31, 2022

The problem size is not so large. I'll try to submit a version fixing them.

With regex throw\(.*,.*,.*\), there are 51 results in 12 files.

@iritkatriel
Copy link
Member

Not all of them need to be ‘fixed’ though.

Some of the tests are testing the deprecated API and need to stay (with the deprecation warning suppressed).

lib2to3 maps the deprecated form to the single arg form. It should not change.

Also, I would run the tests and see what fails rather than rely on a regex search.

@iritkatriel
Copy link
Member

Note also that just emitting a deprecation warning doesn’t cause tests to fail (we don’t run the tests with warnings-as-errors in CI).

This is why I suggested the change in #96348 (comment), because that actually causes most (if not all) of the test failures.

@ofey404
Copy link
Contributor

ofey404 commented Aug 31, 2022

This is why I suggested the change in #96348 (comment), because that actually causes most (if not all) of the test failures.

You're right.

@ofey404
Copy link
Contributor

ofey404 commented Sep 30, 2022

Should we close this since the PR is merged?

Repository owner moved this from Todo to Done in Release and Deferred blockers 🚫 Sep 30, 2022
serhiy-storchaka pushed a commit to serhiy-storchaka/cpython that referenced this issue Oct 2, 2022
@gvanrossum
Copy link
Member Author

gvanrossum commented Oct 11, 2022 via email

@AdamWill
Copy link
Contributor

Wasn't this deprecation rather aggressive?

For generators at least, the single-argument form was only added in Python 3.9, at least according to the docs. It's not mentioned in the 3.8 docs, it only appears in the 3.9 docs.

3.8 is still in support for another whole year. So if I'm trying to fix something - like twisted - to not print deprecation warnings constantly on Python 3.12, I either have to break it for a still-supported version of Python, or use some kind of rather ugly conditional to figure out what to pass it, right?

@AdamWill
Copy link
Contributor

...or is this similar to #105269 and the single-argument form was actually added earlier, but not documented?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 bugs and security fixes easy interpreter-core (Objects, Python, Grammar, and Parser dirs) release-blocker type-feature A feature request or enhancement
Projects
Development

No branches or pull requests

8 participants