implement warnings module in C

[nnorwitz]

BPO	1631171
Nosy	@brettcannon, @tiran
Files	c_warnings.diff: Complete patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/brettcannon'
closed_at = <Date 2008-04-12.23:44:28.708>
created_at = <Date 2007-01-09.06:29:21.000>
labels = ['interpreter-core']
title = 'implement warnings module in C'
updated_at = <Date 2008-04-12.23:44:28.707>
user = 'https://bugs.python.org/nnorwitz'

bugs.python.org fields:

activity = <Date 2008-04-12.23:44:28.707>
actor = 'brett.cannon'
assignee = 'brett.cannon'
closed = True
closed_date = <Date 2008-04-12.23:44:28.708>
closer = 'brett.cannon'
components = ['Interpreter Core']
creation = <Date 2007-01-09.06:29:21.000>
creator = 'nnorwitz'
dependencies = []
files = ['9920']
hgrepos = []
issue_num = 1631171
keywords = ['patch']
message_count = 25.0
messages = ['51714', '51715', '56283', '56288', '56291', '56409', '56509', '56517', '56545', '56983', '57627', '57670', '57767', '57968', '58110', '62708', '63510', '64040', '64780', '64801', '64803', '64808', '64820', '64831', '65427']
nosy_count = 3.0
nosy_names = ['nnorwitz', 'brett.cannon', 'christian.heimes']
pr_nums = []
priority = 'high'
resolution = 'accepted'
stage = None
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue1631171'
versions = ['Python 2.6']

[nnorwitz]

Re-implement the warnings module in C for speed and to reduce start up time.

I don't remember the exact state of this patch. I'm sure it needs cleanup. IIRC the only thing missing feature-wise was processing command line arguments. Though I'm not entirely sure. It's been a while since I did it.

I think I may have not used as many goto's in the code. I'm also thinking I didn't like it as the error handling was too complex. This definitely needs review. If anyone wants to finish this off, go for it. I'll probably return to it, but it won't be for a few weeks at the earliest. It would probably be good to make comments to remind me of what needs to be done.

The new file should be Python/_warnings.c. I couldn't decide whether to put it under Python/ or Modules/. It seems some builtin modules are in both places. Maybe we should determine where the appropriate place is and move them all there.

I couldn't figure out how to get svn to do a diff of a file that wasn't checked in. I think I filtered out all the unrelated changes.

[nnorwitz]

File Added: _warnings.c

[brettcannon]

I have uploaded my touched up version of _warnings.c . I also have a
new diff since the old one did not apply cleanly (test_warnings didn't
get patched nor did PCbuild8). It also lacked the changes needed to
Modules/config.c .

It is still not complete, though. Some things are missing from
_warnings.c and test_warnings is completely failing. But I figured I
should get something uploaded so that other people can help if they want.

[brettcannon]

I just tried Neal's version of _warnings.c and it has the same failures,
so they are not my fault. =)

[brettcannon]

I figured out why the tests are all failing; the C code does not call
back into the Python 'warnings' wrapper.

For instance, warn_explicit in the C code does not ever try to use a
user-provided showwarning() function. This causes the tests to fail as
they rely on this functionality.

There is also the issue of the filters and once_registry also only be
referenced in the C code and not in the Python code. So while the list
and dict, respectively, are assigned in warnings.py from _warnings.c,
the C code never checks to see if the attributes in the Python code were
changed. This is an issue as a lot of times code does::

  warnings.filter = []
  ... code ...
  warnings.filter = original_filter

That will not work with how it is implemented now as the C code only
works off of the object it created and never one that a user may have
provided.

Could a descriptor be written so that when the Python code has the
filter, once_registry, or showwarnings set it actually gets set in the C
code? That way the C code can continue to be fully independent of the
Python code and not have to import it unless a specific change was
attempted upon 'warnings'?

[brettcannon]

So the descriptor idea didn't work.

Another idea is to have the C code that relies on attributes on warnings
that are allowed to change have an initial check for warnings, and if
that fails to fall back on C code. That way the module can still be
completely self-sufficient while still allowing users to change values
from Python code.

For instance, take warnings.filters. Initially it can be set to
_warnings.filters. But in the C code, where access to the filters list
is needed, a check is first done to see if the warnings module has been
imported. If so, check for a filters attribute. If it exists, replace
the internal list with that one and continue on. If the module is not
there, however, use the list stored statically in the module without
change. Same idea of the once registry and a similar idea for
showwarnings().

[brettcannon]

Attached is a new version of _warnings.c that checks to see if
'warnings' has been imported, and if so, uses the attributes from that
module for onceregistry and 'filters'. I did it in such a way so that
'warnings' is in no way required nor imported through checking.

If Neal does a code review and OKs the approach then it can also be
implemented for showwarning() which should make testing test_warnings
possible. =) Then we can start testing that changes to the module
attributes actually affect things properly.

[nnorwitz]

I think this is good enough for now. The approach will probably stand
even if the details change. Go for it!

[brettcannon]

Regression test suite now passes. =) Had to add the support for when
warnings.showwarning() is set and a bug in PyErr_WarnExplicit() where a
NULL value for the module name was not being allowed.

Couple things still left to implement. One is the second output line in
show_warning(). Don't notice this in unit tests as it imports warnings
and thus uses the Python implementation that works properly. Also need
to implement warn_explicit(). Lastly, -W arguments need to be handled.

In terms of differing semantics, file paths are different. Old way did
absolute paths. It also handled what the file path should be when
executing a warning from the interpreter differently.

In terms of testing, some _warnings-specific tests are needed. As
mentioned above, the differences between _warnings.show_warning() and
warnings.showwarning() are not being picked up. This shows how tests
for setting of 'filters', 'onceregistry', and 'showwarning' needs to be
tested.

Assigning to Neal to see if there is anything I missed in terms of todos
and see if he wants to take any of them on. =)

[nnorwitz]

I think Brett summarized the issues well. I can't think of anything
else that seems to need doing.

[brettcannon]

This version of test_warnings has tests for _warnings.c. It currently
is failing as the second line of output for warnings has not been
implemented yet for _warnings.c.

But all the specified tests in my last comment have now been implemented.

[brettcannon]

Attached is a diff against the trunk (including _warnings.c) that adds
the second line of output.

That leaves warn_explicit() and handling -W arguments on the TODO list.
I really don't want to do the -W arguments, so anyone who wants to take
that on, feel free. =)

[brettcannon]

Implementing warn_explicit() is going to be troublesome with the new
module_globals argument. It requires not only to go through the hassle
of looking for a loader and calling get_source(), but it will most
likely require reworking the traceback function introduced to print out
the second line of output (it doesn't look like it directly supports
loaders, but I have not tried it personally).

[brettcannon]

I see two ways of implementing the fetching of a source code line from
__loader__.get_source().

One is to do it in Python. We have a function provided that can
suppress the second line of output from a warning and just handle it in
Python code. That has the requirement that Python code be available to
import, but if you are using Lib/warnings.py instead of
Python/_warnings.c that is pretty much guaranteed.

The other option is to rely on the fact that get_source() is supposed to
use universal newlines. Then we can find the index of the x and x-1
newlines and print the substring between the two. That can be done in C
code by checking for the loader, checking for get_source(), calling it,
getting the char buffer, and then just looping through looking for the
needed newlines to find the needed indexes. Otherwise we can use the
Python API on strings to do what would have been done in pure Python,
but that is a lot of PyObject_Call() usage and seems overly inefficient
if one bothers with coding it in C. =)

[brettcannon]

The attached patch implements warn_explicit(), all in C. Tests are
still needed, though (including for showwarning() and formatwarning() to
deal with their new 'line' argument).

And -W still needs to be dealt with.

[brettcannon]

Patch that has been brought up-to-date with r60968. No new work, though.

[brettcannon]

Add tests for the 'line' argument to formatwarning() and showwarning().

[brettcannon]

Attached is what I think is a completely working version of warnings
implemented in a mixture of C and Python. I am not worrying about
documenting the new C APIs I had to add since they are pretty specific
to internal stuff. Probably should add a leading '_', but I'm tired. =)

I think the fleshed out tests do a pretty good job of testing new code.
Even if this patch gets held up the tests should definitely get
backported as is reasonable.

Assigning to Neal to review (hopefully soon).

[brettcannon]

On another note, the warnings module should be made to work with or
without _warnings. that way IronPython, Jython, and PyPy won't have to re-
implement stuff. This also means that test cases need to be changed to
test this.

[nnorwitz]

I didn't realize this was waiting for me. You should have just checked
it in, that would have gotten me to review faster. :-)

pythonrun.c:

• Should PyModule_GetWarningsModule() return a valid pointer?
• The code below crashes. Need to XDECREF, not DECREF (or similar).
  + PyObject *warnings_module = PyImport_ImportModule("warnings");
  + if (!warnings_module)
  + PyErr_Clear();
  + Py_DECREF(warnings_module);

Python/_warnings.c:

• Remove // XXX(nnorwitz): need to parse -W cmd line flags

Include/pythonrun.h

• init_warnings has the wrong name (not prefixed with _Py). I'm not
  sure it should be exported at all.

test_support/frozen: did you want the captured_std{out,err} change in
this patch?

Changes to Makefile.pre.in other than adding _warnings.o?

I think this is good enough if it's working. How about checking it in
after 1) the alpha is released Wed and 2) fixing up the nits?

[brettcannon]

After all the threats about checking in code that break stuff, I am not
about to check this in. =)

I will get to the changes when I can and then commit after the alpha.

[brettcannon]

Neal's issues are addressed in this patch. I also finally filled out
warnings.h. The only thing that I didn't deal with is Neal's worry of
exposing _PyWarnings_Init(). It is not explicitly exported anywhere as
part of the API so I am not sure what he is worrying about.

The attached patch is finished for CPython. I do want to go back and put
back in the pure Python code that _warnings.c replaces so that
alternative VMs don't have to implement any part of warnings if they
don't want to.

[brettcannon]

Attached should have everything, including a pure Python fallback. As soon
as the next alpha is out I will apply.

[nnorwitz]

On Tue, Apr 1, 2008 at 6:14 AM, Brett Cannon <report@bugs.python.org> wrote:

Brett Cannon <brett@python.org> added the comment:

Neal's issues are addressed in this patch. I also finally filled out
warnings.h. The only thing that I didn't deal with is Neal's worry of
exposing _PyWarnings_Init(). It is not explicitly exported anywhere as
part of the API so I am not sure what he is worrying about.

I wasn't so worried about exposing it, I didn't like the name
pollution (we're talking about init_warnings, right?). I know that we
have other modules that use init*, but those are broken too. I'm not
sure we should fix those in 2.6, but 3.0 we should.

[brettcannon]

Committed in revision 62303.