classification
Title: Add PendingDeprecationWarning for % formatting
Type: Stage:
Components: Interpreter Core Versions: Python 3.0, Python 2.6
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: amaury.forgeotdarc, benjamin.peterson, eric.smith, nnorwitz, rhettinger
Priority: normal Keywords: patch

Created on 2008-05-06 14:14 by eric.smith, last changed 2009-01-06 10:00 by rhettinger. This issue is now closed.

Files
File name Uploaded Description Edit
percent_formatting_pending_deprecation.diff eric.smith, 2008-05-06 14:14
percent_formatting_pending_deprecation_0.diff eric.smith, 2008-05-09 12:57
Messages (16)
msg66313 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2008-05-06 14:14
Per http://mail.python.org/pipermail/python-3000/2008-April/013094.html,
add a PendingDeprecationWarning to 3.0 for % formatting.

The attached patch implements this for 3.0.  For 2.6, it should only be
a PendingDeprecationWarning if -3 warnings are turned on.  I'll work on
that after the 3.0 patch is accepted.

I'm not wild about using a global variable to disallow recursion, but
it's probably the way to go.  Question:  Do I need to acquire the GIL here?

Another issue is that the interpreter should probably at least start up
without triggering these warnings.  I'll add an issue for that at a
later date.
msg66314 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2008-05-06 14:48
> Do I need to acquire the GIL here?
No, the GIL has already been acquired.

The problem with the static variable is that while the main thread is
PyErr_WarnEx(), another thread may gain focus. And it will not trigger
the warning.
msg66316 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2008-05-06 14:55
Right.  But is it worth adding per-thread machinery to this?  I was
going to do that, but it seemed like overkill.  Upon further reflection,
however, I think you may be right.

I'll remove the "easy" keyword!
msg66333 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-05-06 21:16
Would it be practical to add a _PyErr_InErrorProcessing function to
prevent recursion?
msg66337 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2008-05-06 21:50
Since we're just trying to prevent this function from recursing
(indirectly) into itself, I think all of the logic can go here.

What would you suggest the function _PyErr_InErrorProcessing do differently?

I think the real issue is:  Does the additional logic and execution time
involved in adding per-thread state justify being "more correct", or can
we occasionally lose a warning message?
msg66338 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-05-06 21:58
On Tue, May 6, 2008 at 4:50 PM, Eric Smith <report@bugs.python.org> wrote:
>
>  Eric Smith <eric@trueblade.com> added the comment:
>
>  Since we're just trying to prevent this function from recursing
>  (indirectly) into itself, I think all of the logic can go here.
>
>  What would you suggest the function _PyErr_InErrorProcessing do differently?

I was just thinking that this problem would probably come up again.

>  I think the real issue is:  Does the additional logic and execution time
>  involved in adding per-thread state justify being "more correct", or can
>  we occasionally lose a warning message?

Well, the first thing to check for is Py_Py3kWarning. Then do the
extra logic and execution speed.
msg66346 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2008-05-06 23:36
> Well, the first thing to check for is Py_Py3kWarning. Then do the
> extra logic and execution speed.

In 3.0, it's always a PendingDeprecationWarning, so that won't work. 
The test needs to be:

if not recursing and warning_is_not_suppressed:
    warn()

The recursion test is expensive if using thread local storage; the
warning suppressed test looks expensive, too.  So there's no quick short
circuit test that I see.  Of course all of this is just hot air until
coded and benchmarked.  I'll cook up a patch, but it will probably not
be ready before the next alpha releases.
msg66350 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2008-05-07 04:03
Why not use the normal recursion check mechanism?  Specifically,

       if (Py_EnterRecursiveCall("unicode % "))
                return NULL;
       // err = Warn();
       Py_LeaveRecursiveCall();

I don't see where the problem with threads comes in.  The GIL is held
and shouldn't be released during this call.  That may not be quite true
(it's conceivable the GIL is released when warning).  I'm not sure what
happens with the I/O system at this point, it's possible that releases
the GIL.  However, if GIL is released and re-acquired in PyWarn_WarnEx()
there are probably bigger issues than this patch that will need to be
addressed.  Note that since the warnings module is now implemented in C,
this should be easier to deal with.

Using the macros above in essence uses TLS, but through Python's
PyThreadState.
msg66361 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2008-05-07 15:33
I'm not sure Py_EnterRecursiveCall is what I want, because that allows
the recursion to happen, but just aborts it when it gets too deep.  What
I want to achieve is to have the warning not called if it's the warning
that's being formatted.  I coded this up and couldn't get it to do the
right thing.

I think a better approach will be to remove % formatting from
warnings.py.  I think that will remove the need for any checks at all. 
I'll investigate that.
msg66365 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-05-07 18:49
On Wed, May 7, 2008 at 10:33 AM, Eric Smith <report@bugs.python.org> wrote:
>  I think a better approach will be to remove % formatting from
>  warnings.py.  I think that will remove the need for any checks at all.
>  I'll investigate that.

That would make user code warning that uses '%"' brittle. However, if
we warn about it, I think it's ok.
msg66384 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2008-05-08 01:26
> That would make user code warning that uses '%"' brittle. However, if
> we warn about it, I think it's ok.

True enough.  Then I think we should go with:
1. Use .format() in the warnings module.
2. Tell the users not to use % formatting in user code for warnings.
3. Add my original, simple, global check for recursion.  It will work
incorrectly in an insignificant number of cases, and will typically
result in at least one warning about % formatting, anyway.

I'll have a patch ready soon.
msg66466 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2008-05-09 12:57
The attached patch (percent_formatting_pending_deprecation_0.diff) adds
both the simple global lock to unicode_mod() and changes warnings.py to
use .format() instead of % formatting.

I think this is good enough.  We should add a warning to the docs saying
user code used for warnings should also avoid % formatting.  The only
problem they'll see, however, is dropping an occasional warning in a
multi-threaded app.  But I'm not sure yet exactly where this mention
would go.

I also don't have any tests for this.  I haven't found a test of a
PendingDeprecationWarning.

The biggest problem is that this patch breaks test_doctest with
"RuntimeError: dictionary changed size during iteration".  I've tried to
find out why, but so far it escapes me.  If someone else could look at
the issue, I'd appreciate it.
msg70003 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2008-07-19 03:59
I think this is premature until be know for sure that % formatting will 
in-fact be deprecated in Py3.1.  Time will tell how well the new format 
options get accepted.  Likewise, we'll learn more about how readily 
legacy code can get converted.

Guido, can you pronouce?
msg70007 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2008-07-19 08:21
I agree.  That's one of the reasons I un-assigned it to me.

Well, that and I couldn't get it to pass all tests.
msg79230 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2009-01-06 05:04
Not it.
msg79243 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2009-01-06 10:00
Am closing this one.  It is pointless and counter-productive unless
there is a firm decision that % formatting is definitely going away (and
migration tools have been developed).  Right now, it looks like there is
some chance that it might pull through for a few versions.
History
Date User Action Args
2009-01-06 10:00:13rhettingersetstatus: open -> closed
resolution: rejected
messages: + msg79243
keywords: patch, patch
2009-01-06 05:04:58gvanrossumsetkeywords: patch, patch
nosy: - gvanrossum
2009-01-06 05:04:48gvanrossumsetkeywords: patch, patch
assignee: gvanrossum ->
messages: + msg79230
nosy: gvanrossum, nnorwitz, rhettinger, amaury.forgeotdarc, eric.smith, benjamin.peterson
2008-07-19 08:21:39eric.smithsetkeywords: patch, patch
messages: + msg70007
2008-07-19 03:59:28rhettingersetkeywords: patch, patch
assignee: gvanrossum
messages: + msg70003
nosy: + rhettinger, gvanrossum
2008-07-19 00:56:12eric.smithsetkeywords: patch, patch
assignee: eric.smith -> (no value)
2008-05-09 12:58:02eric.smithsetkeywords: patch, patch
files: + percent_formatting_pending_deprecation_0.diff
messages: + msg66466
2008-05-08 01:26:49eric.smithsetkeywords: patch, patch
messages: + msg66384
2008-05-07 18:49:31benjamin.petersonsetmessages: + msg66365
2008-05-07 15:33:30eric.smithsetkeywords: patch, patch
messages: + msg66361
2008-05-07 04:03:31nnorwitzsetkeywords: patch, patch
nosy: + nnorwitz
messages: + msg66350
2008-05-06 23:36:57eric.smithsetkeywords: patch, patch
messages: + msg66346
2008-05-06 21:58:51benjamin.petersonsetmessages: + msg66338
2008-05-06 21:50:10eric.smithsetkeywords: patch, patch
messages: + msg66337
2008-05-06 21:16:22benjamin.petersonsetkeywords: patch, patch
nosy: + benjamin.peterson
messages: + msg66333
2008-05-06 14:55:34eric.smithsetkeywords: - easy
messages: + msg66316
2008-05-06 14:48:30amaury.forgeotdarcsetkeywords: patch, patch, easy
nosy: + amaury.forgeotdarc
messages: + msg66314
2008-05-06 14:14:09eric.smithcreate