Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Apr 12, 2025
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
2bd0582
Prevent emitting optimizer warnings twice in the REPL
tomasr8 Apr 1, 2025
08eab52
Improve test
tomasr8 Apr 2, 2025
2049da3
Fix tests
tomasr8 Apr 2, 2025
a922e16
Lint fix
tomasr8 Apr 2, 2025
742a4b1
Revert REPL changes
tomasr8 Apr 5, 2025
bbefbf8
Set up a warning registry for syntax warnings
tomasr8 Apr 5, 2025
d047dd3
newline
tomasr8 Apr 5, 2025
c4a27bc
Fix test
tomasr8 Apr 7, 2025
7f8d1af
PEP7 + missing decref
tomasr8 Apr 7, 2025
c15635b
Add missing decref
tomasr8 Apr 12, 2025
97d028e
Add more tests
tomasr8 Apr 12, 2025
a8fa07f
Merge remote-tracking branch 'upstream/main' into repl-warnings
tomasr8 Apr 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 35 additions & 8 deletions Lib/_pyrepl/console.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
from dataclasses import dataclass, field
import os.path
import sys
import warnings


TYPE_CHECKING = False
Expand Down Expand Up @@ -188,24 +189,29 @@ def runcode(self, code):

def runsource(self, source, filename="<input>", symbol="single"):
try:
tree = self.compile.compiler(
source,
filename,
"exec",
ast.PyCF_ONLY_AST,
incomplete_input=False,
)
with warnings.catch_warnings(record=True) as all_warnings:
warnings.simplefilter("always")
Copy link
Member

@iritkatriel iritkatriel Apr 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can pass a registry?

Copy link
Member Author

@tomasr8 tomasr8 Apr 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

tree = self.compile.compiler(
source,
filename,
"exec",
ast.PyCF_ONLY_AST,
incomplete_input=False,
)
except (SyntaxError, OverflowError, ValueError):
self.showsyntaxerror(filename, source=source)
return False
_replay_warnings(all_warnings)
if tree.body:
*_, last_stmt = tree.body
for stmt in tree.body:
wrapper = ast.Interactive if stmt is last_stmt else ast.Module
the_symbol = symbol if stmt is last_stmt else "exec"
item = wrapper([stmt])
try:
code = self.compile.compiler(item, filename, the_symbol)
with warnings.catch_warnings(record=True) as stmt_warnings:
warnings.simplefilter("always")
code = self.compile.compiler(item, filename, the_symbol)
linecache._register_code(code, source, filename)
except SyntaxError as e:
if e.args[0] == "'await' outside function":
Expand All @@ -220,10 +226,31 @@ def runsource(self, source, filename="<input>", symbol="single"):
self.showsyntaxerror(filename, source=source)
return False

# Only emit new warnings and throw away those
# which were already emitted when the source
# was compiled to AST.
# This prevents emitting duplicate warnings which are
# raised in the AST optimizer.
all_warnings = {str(w.message) for w in all_warnings}
new_warnings = [w for w in stmt_warnings
if str(w.message) not in all_warnings]
_replay_warnings(new_warnings)

if code is None:
return True

result = self.runcode(code)
if result is self.STATEMENT_FAILED:
break
return False


def _replay_warnings(ws):
for w in ws:
warnings.warn_explicit(
message=w.message,
category=w.category,
filename=w.filename,
lineno=w.lineno,
source=w.source,
)
25 changes: 25 additions & 0 deletions Lib/test/test_pyrepl/test_interact.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,3 +273,28 @@ def test_incomplete_statement(self):
code = "if foo:"
console = InteractiveColoredConsole(namespace, filename="<stdin>")
self.assertTrue(_more_lines(console, code))


class TestWarnings(unittest.TestCase):
def test_pep_765_warning(self):
"""
Test that a SyntaxWarning emitted from the
AST optimizer is only shown once in the REPL.
"""
# gh-131927
console = InteractiveColoredConsole()
code = dedent("""\
def f():
try:
return 1
finally:
return 2
""")

f = io.StringIO()
with contextlib.redirect_stderr(f):
result = console.runsource(code)
self.assertFalse(result)
self.assertEqual(f.getvalue(),
"<input>:5: SyntaxWarning: "
"'return' in a 'finally' block\n")
Loading