PEP 553 built-in debug() function (bpo-31353) #3355

warsaw · 2017-09-05T20:39:45Z

https://bugs.python.org/issue31353

Found by Kirit Sankar Gupta.

* debug() calls sys.debughook() * sys.debughook() by default calls pdb.set_trace() * Add sys.__debughook__ to preserve the original.

Use Argument Clinic for the debug() docstring.

warsaw · 2017-09-05T21:20:34Z

https://www.python.org/dev/peps/pep-0553/

freddrake · 2017-09-07T19:33:17Z

Python/sysmodule.c

@@ -1951,6 +1978,9 @@ _PySys_BeginInit(void)
                               PyDict_GetItemString(sysdict, "displayhook"));
    SET_SYS_FROM_STRING_BORROW("__excepthook__",
                               PyDict_GetItemString(sysdict, "excepthook"));
+    SET_SYS_FROM_STRING_BORROW(


Your indentation here is different from what's already present here. While I personally prefer how you've done it, local inconsistency is ugly.

True, but unfortunately that's the only way to not break 80 columns (or maybe "the least horrible way").

Changing the indentation of the other entries should not be discarded as an option if that allows PEP 7 conformance.

Not ideal, perhaps, but allows local consistency.

I'm not sure I want to touch lines outside those related to this PEP.

In the interest of local consistency, that could be done after PEP code is landed (assuming acceptance, which sounds likely), as a separate commit. Otherwise, some way to conform would be valuable.

vstinner

You requested my review. Here you have, enjoy! :-)

My main question is if PYTHONBREAKPOINT should be evaluated at each sys.breakpoint() call, or if it could be read only once at Python startup?

The PEP explicitly says "PYTHONBREAKPOINT is re-interpreted every time sys.breakpointhook() is reached.". That's surprising to me, but I don't really care and the PEP is already accepted. So please ignore my comments related to this ;-)

vstinner · 2017-10-03T21:57:42Z

Doc/library/sys.rst

+   resulting module must have a callable named ``function()``.  This is run,
+   passing in ``*args`` and ``**kws``, and whatever ``function()`` returns,
+   ``sys.breakpointhook()`` returns to the built-in :func:`breakpoint`
+   function.


PYTHONBREAKPOINT should be written:

:envvar:`PYTHONBREAKPOINT`

(as you did in the What's New in Python 3.7) and you should document the variable in Doc/using/cmdline.rst.

Thanks, fixed!

vstinner · 2017-10-03T22:01:28Z

Lib/test/test_builtin.py

+
+class TestBreakpoint(unittest.TestCase):
+    def test_breakpoint(self):
+        with patch('pdb.set_trace') as mock:


Hum, would it be possible to modify the unit test so it doesn't rely on breakpoint() calling pdb.set_trace()? The test may be run with PYTHONBREAKPOINT set to something else.. to debug the test (or another test while running the test suite). Or skip the test if sys.breakpointhook is no sys.breakpointhook?

See above about the clean slate, but in this case we're testing the default behavior, which is exactly that the clean slate calls pdb.set_trace().

vstinner · 2017-10-03T22:02:14Z

Lib/test/test_builtin.py

+        call_status = 'Not called'
+        def my_breakpointhook():
+            nonlocal call_status
+            call_status = 'Called'


You might use a mock to get arguments: assert_called_once_with(...).

I'm not quite sure what you're suggesting here. There are other tests to ensure that the right positional and keyword arguments are passed through. This test just ensures that a custom sys.breakpointhook gets called at all. (I like focused unit tests.)

I think haypo is suggesting code like:

myhook = mock.MagicMock() sys.breakpointhook = myhook breakpoint('hello') myhook.assert_called_once_with('hello')

instead of manual tracking of the call status.

@merwok ah, thanks! I like that.

vstinner · 2017-10-03T22:03:14Z

Lib/test/test_builtin.py

+            sys.breakpointhook = my_breakpointhook
+            breakpoint()
+        finally:
+            sys.breakpointhook = sys.__breakpointhook__


I suggest to use support.swap_attr(sys, "breakpointhook", ...): it's shorter and restore the variable to its previous value. Again, the test runner might have replaced sys.breakpointhook for good reasons. Don't reset it to sys.breakpointhook.

Excellent suggestion. I'm going to use this as mentioned above.

vstinner · 2017-10-03T22:04:57Z

Lib/test/test_builtin.py

+        self.assertEqual(call_status, 'Called')
+        with patch('pdb.set_trace') as mock:
+            breakpoint()
+        mock.assert_called_once()


Would it be possible to restore sys.breakpointhook to its original value at the end of the test?

Yep, see above.

vstinner · 2017-10-03T22:27:02Z

Python/sysmodule.c

@@ -96,6 +96,80 @@ PySys_SetObject(const char *name, PyObject *v)
        return PyDict_SetItemString(sd, name, v);
 }

+static PyObject *
+sys_breakpointhook(PyObject *self, PyObject *args, PyObject *keywords)
+{


Add "assert(!PyErr_Occurred());" at the entry point since this function is called when something goes wrong, and the implementation don't expect that an exception is currently set.

... Maybe even raise an exception at runtime if it's called with an exception set? See _Py_CheckFunctionResult() which implements a similar check at exit point.

Is that true, i.e. that it's possible to call this with an exception set? I don't mind adding an assertion for that, but I'm not sure I see how it's possible. builtin_breakpoint() won't call it with an exception set AFAICT. What scenario are you thinking about?

Is that true, i.e. that it's possible to call this with an exception set?

In my experience, it's very common that a function returning a result and set an exception is not catched immediately, but somewhere "later". I had such issues when I ran my pyfailmalloc tool which injects MemoryError anywhere.

But I made many changes in CPython internals to add assert(!PyErr_Occurred()) at many places. The main change is that all functions calling functions should now check that no exception is raised if the called function returned a result. This is _Py_CheckFunctionResult().

To come back to your breakpoint() implementation, "assert(!PyErr_Occurred());" should be enough.

Just to be clear, you're suggesting assert(!PyErr_Occurred()); at the beginning of sys_breakpointhook() right? I'm cool with that.

vstinner · 2017-10-03T22:27:41Z

Python/sysmodule.c

+    PyErr_Clear();
+    int status = PyErr_WarnFormat(
+        PyExc_RuntimeWarning, 0,
+        "Ignoring unimportable $PYTHONBREAKPOINT: \"%s\"", envar);


I suggest to not dump the variable value to avoid encoding/escaping issues (%s doesn't escape quotes). It's up to you.

For now I think it's useful to include the value. Let's see if it's a problem in practice.

vstinner · 2017-10-03T22:30:45Z

Python/sysmodule.c

+    int status = PyErr_WarnFormat(
+        PyExc_RuntimeWarning, 0,
+        "Ignoring unimportable $PYTHONBREAKPOINT: \"%s\"", envar);
+    if (status == 0) {


I would prefer to handle the error first "if (status < 0) { return NULL; }", and use "Py_RETURN_NONE" for the regular case. (Except true and false branches.)

vstinner · 2017-10-03T22:31:53Z

Python/sysmodule.c

+}
+
+PyDoc_STRVAR(breakpointhook_doc,
+"breakpointhook()\n"


It's unclear to me if the prototype is breakpointhook() or breakpointhook(*args, **kw)?

vstinner · 2017-10-03T22:32:48Z

Python/sysmodule.c

+PyDoc_STRVAR(breakpointhook_doc,
+"breakpointhook()\n"
+"\n"
+"Called when the built-in breakpoint() function is called.\n"


I suggest to repeat the first sentence of its doc:

This hook function is called by built-in breakpoint().

bedevere-bot · 2017-10-03T22:34:58Z

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 didn't expect the Spanish Inquisition!. 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.

The biggest change is to use a setUp() that ensures a clean slate for the tests, regardless of whether sys.breakpointhook or PYTHONBREAKPOINT is set.

* Improve the readability of the docstring. * Add an assertion. * Use Py_GETENV to obey -E

warsaw · 2017-10-05T03:28:41Z

@Haypo @merwok Thanks for the great comments. I think all issues are address, save the behavior of PYTHONBREAKPOINT under -E. Let's see how the python-dev discussion shakes out.

vstinner · 2017-10-05T13:39:19Z

There is still a bug somewhere in tests:

haypo@selma$ PYTHONBREAKPOINT=0 ./python -E -m test test_builtin
Run tests sequentially
0:00:00 load avg: 1.46 [1/1] test_builtin
> /home/haypo/prog/python/master/Lib/test/test_builtin.py(1581)test_envar_good_path_other()
-> mock.assert_called_once_with()
(Pdb)

vstinner · 2017-10-05T13:42:55Z

I ran "./python -m test -R 3:3 test_builtins" and there is no reference leak.

Note: I had to disable two unrelated unit tests because of https://bugs.python.org/issue31703

vstinner

LGTM, but there is a minor issue in tests (see my previous comment) and a missing "verisonadded" in the doc. Moreover, tests are failing, so I cannot approve the change yet.

vstinner · 2017-10-05T13:37:21Z

Doc/using/cmdline.rst

+   called by built-in :func:`breakpoint`.  If not set, or set to the empty
+   string, it is equivalent to the value "pdb.set_trace".  Setting this to the
+   string "0" causes the default implementation of :func:`sys.breakpointhook`
+   to do nothing but return immediately.


".. versionadded:: 3.7" is missing here.

warsaw · 2017-10-05T14:39:57Z

@Haypo Yeah, a bunch of tests have to be skipped when -E is given! I don't think there's any way to run them in that case, since Py_IgnoreEnvironmentFlag is not settable or mockable from Python.

ncoghlan · 2017-10-05T14:46:02Z

@warsaw There are some other tests that are run in subprocesses specifically to cope with the main test runner being executed with -E set. It's less convenient for coverage collection, but avoids the need to skip the tests when -E is set.

warsaw · 2017-10-05T15:58:48Z

I ran "./python -m test -R 3:3 test_builtins" and there is no reference leak.

I couldn't get that to do anything other than just (seemingly) hang on macOS. :(

% ./python.exe -m test -R 3:3 test_builtin
./python.exe -m test -R 3:3 test_builtin
Run tests sequentially
0:00:00 load avg: 2.25 [1/1] test_builtin
beginning 6 repetitions
123456
  C-c C-c^C
Test suite interrupted by signal SIGINT.
1 test omitted:
    test_builtin

Total duration: 49 min 14 sec
Tests result: INTERRUPTED

Maybe that's caused by the bug you referenced?

warsaw · 2017-10-05T16:00:34Z

@ncoghlan

There are some other tests that are run in subprocesses specifically to cope with the main test runner being executed with -E set. It's less convenient for coverage collection, but avoids the need to skip the tests when -E is set.

I think you're talking about test_cmd_line.py's use of interpreter_requires_environment()? I'm not sure this is a big enough win for the complexity. I'm fine skipping the tests since they really make no sense under -E.

warsaw · 2017-10-05T16:01:30Z

@Haypo I think we're good to go. Do you agree?

vstinner · 2017-10-05T16:03:05Z

Yeah, LGTM. We can still enhance tests later if needed (run tests which depends on -E in subprocesses).

warsaw added 12 commits September 4, 2017 09:26

Watch out for NULL return.

9dc577a

Found by Kirit Sankar Gupta.

Add a blurb entry for the issue.

bc4634d

Typo in blurb entry.

f7e3c40

Merge branch 'master' into compile-null-ptr

6696c23

Set an appropriate exception.

4600bcf

Add a debug() builtin.

0f46af7

* debug() calls sys.debughook() * sys.debughook() by default calls pdb.set_trace() * Add sys.__debughook__ to preserve the original.

Document the new stuff.

a141f63

Add tests for the debug() builtin.

79d7ac8

Use Argument Clinic for the debug() docstring.

Undo a branch leak.

1666925

Add versionadded tags and cleanup.

8437033

Merge branch 'master' into debughook

6534542

Merge branch 'master' into debughook

2d75d7a

the-knights-who-say-ni added the CLA signed label Sep 5, 2017

bedevere-bot added the awaiting merge label Sep 5, 2017

warsaw changed the title ~~PEP 553 built-in debug() function~~ PEP 553 built-in debug() function (bpo-31353) Sep 5, 2017

Add a blurb for PEP 553.

ed649fa

warsaw added 8 commits September 6, 2017 13:35

Merge branch 'master' into debughook

0bea882

Merge branch 'master' into debughook

eee151d

debug() -> breakpoint()

05b7cae

Add a test for breakpointhook reset.

6b9a59c

Merge branch 'master' into debughook

5e0b93a

Signature changed to breakpoint(*args, **kws).

c57d0dc

Merge branch 'master' into debughook

eb17d0f

Update documentation.

244bff5

freddrake reviewed Sep 7, 2017

View reviewed changes

warsaw added 4 commits September 7, 2017 14:17

Merge branch 'master' into debughook

c4bed66

Make patchheck happy.

6eb3470

Merge branch 'master' into debughook

af81484

Support $PYTHONBREAKPOINT

f3200c7

vstinner requested changes Oct 3, 2017

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting merge labels Oct 3, 2017

warsaw added 6 commits October 3, 2017 19:47

Merge branch 'master' into debughook

6b23d5b

Several fixes addressing review comments.

0d2fae5

The biggest change is to use a setUp() that ensures a clean slate for the tests, regardless of whether sys.breakpointhook or PYTHONBREAKPOINT is set.

Respond to reviewer comments

1cf3e06

* Improve the readability of the docstring. * Add an assertion. * Use Py_GETENV to obey -E

Use METH_FAST and _PyObject_FastCallKeywords()

6217e90

Merge branch 'master' into debughook

28bd8e4

Use a MagicMock.

8808367

Merge branch 'master' into debughook

bd68b76

vstinner reviewed Oct 5, 2017

View reviewed changes

warsaw added 3 commits October 5, 2017 10:14

Merge branch 'master' into debughook

fea5d3d

Some tests must be skipped when -E is given

599108d

Add a missing versionadded

a74f23e

vstinner approved these changes Oct 5, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Oct 5, 2017

warsaw merged commit 36c1d1f into python:master Oct 5, 2017

bedevere-bot removed the awaiting merge label Oct 5, 2017

warsaw deleted the debughook branch October 5, 2017 16:11

PEP 553 built-in debug() function (bpo-31353) #3355

PEP 553 built-in debug() function (bpo-31353) #3355

Conversation

warsaw commented Sep 5, 2017 • edited by bedevere-bot

warsaw commented Sep 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merwok Oct 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bedevere-bot commented Oct 3, 2017

warsaw commented Oct 5, 2017

vstinner commented Oct 5, 2017

vstinner commented Oct 5, 2017

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

warsaw commented Oct 5, 2017

ncoghlan commented Oct 5, 2017

warsaw commented Oct 5, 2017

warsaw commented Oct 5, 2017

warsaw commented Oct 5, 2017

vstinner commented Oct 5, 2017

warsaw commented Sep 5, 2017 •

edited by bedevere-bot

merwok Oct 4, 2017 •

edited