gh-131927: Prevent emitting optimizer warnings twice in the REPL

[tomasr8]

Fix based on the idea from @iritkatriel: #131927 (comment)

Yet another option is to leave ast_opt alone, and handle this in the repl. So the repl would save the warnings from the ast construction and then filter out duplicated warnings from the compilation call.

PEP 765 is new in 3.14 so I think we can skip a news entry?

• Issue: Compilation emits multiple warnings in the finally block #131927

[iritkatriel]

Can we use "default" instead of "always", and just one context manager that spans both compiler calls? I think that will do the de-duplication, if I understand the doc correctly.

https://docs.python.org/3/library/warnings.html#the-warnings-filter

[tomasr8]

From what I can tell it does not work in this case. The syntax warning uses _PyErr_EmitSyntaxWarning which in turn calls PyErr_WarnExplicitObject and sets the registry (last argument) to NULL. The registry is used to dedup the warnings so when it's not used, all warnings are emitted regardless of the filter setting.

Here's an example that shows that:

import warnings

def _compile():
    compile('1 is 1', 'input', 'eval')

with warnings.catch_warnings():
    warnings.simplefilter("default")
    _compile()
    _compile()

This is probably not something we want to change, so doing a manual deduplication in pyrepl seems like the way to go. What do you think?

[iritkatriel]

Maybe we can pass a registry?

[tomasr8]

Maybe, though the registry would have to persist between separate invocations of compile. I'll check if there's a nice way to do that.

[tomasr8]

Ok so I spent some time going over the warning code and this is my understanding so far. The warning registries are dictionaries that are inserted as __warningregistry__ into the globals of a Python frame. Which frame this is, is controlled by the stack level. The registries are created on demand in setup_context:

cpython/Python/_warnings.c

Lines 911 to 916 in 37bc386

	static int
	setup_context(Py_ssize_t stack_level,
	PyTupleObject *skip_file_prefixes,
	PyObject **filename, int *lineno,
	PyObject **module, PyObject **registry)
	{

This function is pretty complex and depends on other pieces in the warnings module so we cannot construct a registry manually in _PyErr_EmitSyntaxWarning (which is the one responsible for emitting warnings from ast_opt and currently sets the registry to NULL):

cpython/Python/errors.c

Lines 1909 to 1910 in 37bc386

	if (PyErr_WarnExplicitObject(PyExc_SyntaxWarning, msg, filename,
	lineno, NULL, NULL) < 0)

One thing we could do, though I don't know how desirable it is, is to add a new private function akin to PyErr_WarnExplicitObject which sets up the registry for us. Basically something like this:

int
- PyErr_WarnExplicitObject(PyObject *category, PyObject *message,
-                          PyObject *filename, int lineno,
-                          PyObject *module, PyObject *registry)
+ _PyErr_WarnExplicitObjectRegistry(PyObject *category, PyObject *message,
+                                  PyObject *filename, int lineno)
{
    PyObject *res;
    if (category == NULL)
        category = PyExc_RuntimeWarning;
    PyThreadState *tstate = get_current_tstate();
    if (tstate == NULL) {
        return -1;
    }

+    PyObject *unused_filename, *module, *registry;
+    int unused_lineno;
+
+    if (!setup_context(1, NULL,
+                       &unused_filename, &unused_lineno, &module, &registry))
+        return -1;

    warnings_lock(tstate->interp);
    res = warn_explicit(tstate, category, message, filename, lineno,
                        module, registry, NULL, NULL);
    warnings_unlock(tstate->interp);
    if (res == NULL)
        return -1;
    Py_DECREF(res);
    return 0;
}

This seems to eliminate the duplicated syntax warnings in the REPL. What do you think about this approach?

[iritkatriel]

I like this. Can _PyErr_WarnExplicitObjectRegistry just create a registry and call PyErr_WarnExplicitObject with it?

[tomasr8]

I think so, basically something like this:

int
_PyErr_WarnExplicitObjectRegistry(PyObject *category, PyObject *message,
                         PyObject *filename, int lineno)
{
    PyObject *unused_filename, *module, *registry;
    int unused_lineno;
    int stack_level = 1;

    if (!setup_context(stack_level, NULL, &unused_filename, &unused_lineno,
                       &module, &registry))
        return -1;

    int rc = PyErr_WarnExplicitObject(category, message, filename, lineno,
                                      module, registry);
    Py_DECREF(module);
    return rc;
}

I'll update the PR to see if everything still works with this change.

[iritkatriel]

tests are failing.

[tomasr8]

tests are failing.

Yep, I suspect it's because setup_context fails which then causes _PyErr_EmitSyntaxWarning to raise instead of emit a warning (I should probably change that). Though I'm not able to reproduce this locally yet.

[python-cla-bot]

All commit authors signed the Contributor License Agreement.

[iritkatriel]

LGTM. @serhiy-storchaka what do you think?

[tomasr8]

Ok it seems some other test modified the warning filters so I just needed to add warnings.simplefilter("default") to the test.

I have another question though, regarding the setup_context function. It can currently fail but I'm not sure what is the best way to handle that? Should we raise a SystemError or maybe just fall back to emitting a warning without constructing the registry (and silently ignore the failure)? Or just do what we do now and let it raise a SyntaxError (instead of emitting a warning)?

[serhiy-storchaka]

You can use .count() here.

"'return' in a 'finally' block" may be an error in future versions. Maybe use some more long living warnings, like assert(1, 'message') or 1 is 1? BTW, they are currently emitted 3 times, not 2.

[tomasr8]

You can use .count() here.

Hmm I don't think I can, unless I want to match the warning message exactly?

"'return' in a 'finally' block" may be an error in future versions. Maybe use some more long living warnings, like assert(1, 'message') or 1 is 1?

I can't if I want to test the new repl. The PEP 765 warning is the only one emitted from the ast optimizer which is needed to trigger this behaviour in the repl. However this does change the behaviour of all warnings that use _PyErr_EmitSyntaxWarning so I also added tests to test_compile for it.

[serhiy-storchaka]

What happens if set it to "always"?

[tomasr8]

It emits twice, I added a test to test_compile for it

[serhiy-storchaka]

LGTM. 👍

[serhiy-storchaka]

Please add also a compile() test for a warning emitted in the finally block. Currently it is emitted twice because the code in the finally block is complied twice (for normal execution and for exception handling).

[iritkatriel]

Let's create a new issue for that.

[iritkatriel]

Let's create a new issue for that.

[tomasr8]

Please add also a compile() test for a warning emitted in the finally block. Currently it is emitted twice because the code in the finally block is complied twice (for normal execution and for exception handling).

PR is here: #132436

[miss-islington-app]

Thanks @tomasr8 for the PR, and @iritkatriel for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

[miss-islington-app]

Sorry, @tomasr8 and @iritkatriel, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 3d08c8ad20dfabd4864be139cd9c2eb5602ccdfe 3.13

[tomasr8]

I'll take care of the backport

[bedevere-app]

GH-132463 is a backport of this pull request to the 3.13 branch.

[bedevere-app]

GH-132463 is a backport of this pull request to the 3.13 branch.

[bedevere-app]

GH-132463 is a backport of this pull request to the 3.13 branch.