bpo-29731: Allow warnings filter to print current stack. #498
Conversation
|
@Carreau, thanks for your PR! By analyzing the history of the files in this pull request, we identified @tiran, @brettcannon, @Haypo, @benjaminp and @loewis to be potential reviewers. |
|
Closing and re-opening to rerun CI. |
Python/_warnings.c
Outdated
| @@ -440,6 +479,7 @@ warn_explicit(PyObject *category, PyObject *message, | |||
| PyObject *key = NULL, *text = NULL, *result = NULL, *lineno_obj = NULL; | |||
| PyObject *item = NULL; | |||
| PyObject *action; | |||
| PyFrameObject *f = PyThreadState_GET()->frame; | |||
There was a problem hiding this comment.
please move it into the print_frames() block, it's only needed there. Or even move it into print_frames() function.
You should use _PyThreadState_GET() to ensure that you get the optimized macro ;-)
There was a problem hiding this comment.
My C is rusty, so apologies for potentially simple questions:
isn't that the same because pycore_pystate aliases it ?
Include/internal/pycore_pystate.h:#define PyThreadState_GET() _PyThreadState_GET()
Also Im' confused about the uppercase vs lowercase PyThreadState(_Get/_GET).
There was a problem hiding this comment.
You only get the alias if you include pycore_pystate. I prefer to always use _PyThreadState_GET() to ensure to always get the optimized macro.
Python/_warnings.c
Outdated
|
|
||
|
|
||
| static void | ||
| print_frames(PyFrameObject *frame) |
There was a problem hiding this comment.
I suggest to rename it to print_stack() to be more consistent with the traceback module.
There was a problem hiding this comment.
Also printing correctly in C and having the exact same behavior in warnigns.py and warning.c seem a hassle, can I go the PyImport route and do the equivalent of
f = sys._getframe().f_back.f_back
traceback.print_stack(f=f)
I know that warning.c is meant to be efficient but here it's an opt-in path, so maybe not that necessary to be completely in C ?
Python/_warnings.c
Outdated
| tb_displayline( | ||
| f_stderr, | ||
| frame->f_code->co_filename, | ||
| PyCode_Addr2Line(frame->f_code, frame->f_lasti), |
There was a problem hiding this comment.
PyFrame_GetLineNumber() must be used here.
nitpick: I would prefer to compute it before calling tb_displayline():
int lineno = PyFrame_GetLineNumber(frame);
tb_displayline(...);
Python/_warnings.c
Outdated
| Py_DECREF(line); | ||
| if (err != 0) | ||
| return err; | ||
| /* ignore errors since we can't report them, can we? */ |
There was a problem hiding this comment.
I suggest to remove the comment.
We could report the error, but I don't think that it's worth it. It would be annoying that logging a warning would emit a second warning/exception.
Python/_warnings.c
Outdated
|
|
||
| if (filename == NULL || name == NULL) | ||
| return -1; | ||
| line = PyUnicode_FromFormat(" File \"%U\", line %d, in %U\n", |
There was a problem hiding this comment.
PyUnicode_FromFormat("%U", arg) doesn't check if arg is really a Unicode type.
I suggest to use %S instead.
There was a problem hiding this comment.
Sorry I completely forgot I had this PR opened, I'll have to get get back into what I was thinking.
It's interesting because I just copy and pasted this from traceback.c, so I guess these comments will apply there as well; and I should likely use the tb_displayline from traceback.c right ?
There was a problem hiding this comment.
Right, you must also fix traceback.c.
It is similar to "always" but show the full stack of where the warning
was triggered.
Currently incomplete, does not respect the `stacklevel=` value, and
non-tested. Code may be shared with the traceback module as well, but
may need a tiny bit of refactoring, stack is printed backward.
~/dev/cpython stack $ cat foo.py
import warnings
warnings.filterwarnings('stack')
def foo():
warnings.warn('this', UserWarning)
def bar():
foo()
bar()
~/dev/cpython stack $ ./python.exe foo.py
File "foo.py", line 5, in foo
warnings.warn('this', UserWarning)
File "foo.py", line 8, in bar
foo()
File "foo.py", line 10, in <module>
bar()
foo.py:5: UserWarning: this
warnings.warn('this', UserWarning)
|
@serhiy-storchaka: This idea is interesting. Would you mind to have a look? |
It is similar to "always" but show the full stack of where the warning
was triggered.
Currently incomplete, does not respect the
stacklevel=value, andnon-tested. Code may be shared with the traceback module as well, but
may need a tiny bit of refactoring, stack is printed backward.
https://bugs.python.org/issue29731