GeezerVison

tomasr8 requested review from ambv, lysnikolaou and pablogsal as code owners

April 1, 2025 22:00

bedevere-app Bot added the awaiting review label

bedevere-app Bot mentioned this pull request

Compilation emits multiple warnings in the finally block #131927

Closed

iritkatriel reviewed

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

View reviewed changes

Comment thread

Lib/test/test_pyrepl/test_interact.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Lib/_pyrepl/console.py Outdated

		incomplete_input=False,
		)
		with warnings.catch_warnings(record=True) as all_warnings:
		warnings.simplefilter("always")

iritkatriel Apr 1, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

tomasr8 Apr 2, 2025

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

iritkatriel Apr 2, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

tomasr8 Apr 2, 2025 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

tomasr8 Apr 5, 2025

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

iritkatriel Apr 5, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

tomasr8 Apr 5, 2025

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

tomasr8 added 6 commits

April 2, 2025 19:58


          Improve test

08eab52


          Fix tests

2049da3


          Lint fix

a922e16


          Revert REPL changes

742a4b1


          Set up a warning registry for syntax warnings

bbefbf8


          newline

d047dd3

iritkatriel reviewed

Apr 5, 2025

View reviewed changes

Comment thread

Python/_warnings.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel commented Apr 5, 2025

Copy link

Copy Markdown

Member

tests are failing.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tomasr8 commented Apr 5, 2025

Copy link

Copy Markdown

Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

python-cla-bot Bot commented Apr 6, 2025

Copy link

Copy Markdown

All commit authors signed the Contributor License Agreement.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tomasr8 added 2 commits

April 7, 2025 22:54


          Fix test

c4a27bc


          PEP7 + missing decref

7f8d1af

iritkatriel approved these changes

Apr 7, 2025

View reviewed changes

bedevere-app Bot added awaiting merge and removed awaiting review labels

Apr 7, 2025

iritkatriel commented Apr 7, 2025

Copy link

Copy Markdown

Member

LGTM. @serhiy-storchaka what do you think?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tomasr8 commented Apr 7, 2025

Copy link

Copy Markdown

Member Author

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)?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka reviewed

Apr 8, 2025

View reviewed changes

Comment thread

Python/_warnings.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Lib/test/test_pyrepl/test_interact.py

		warnings.simplefilter("default")
		console.runsource(code)

		count = sum("'return' in a 'finally' block" in str(w.message)

serhiy-storchaka Apr 8, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

tomasr8 Apr 12, 2025

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/test/test_pyrepl/test_interact.py

		""")

		with warnings.catch_warnings(record=True) as caught:
		warnings.simplefilter("default")

serhiy-storchaka Apr 8, 2025

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

What happens if set it to "always"?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

tomasr8 Apr 12, 2025

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

tomasr8 added 3 commits

April 12, 2025 11:26


          Add missing decref

c15635b


          Add more tests

97d028e


          Merge remote-tracking branch 'upstream/main' into repl-warnings

a8fa07f

serhiy-storchaka approved these changes

Apr 12, 2025

View reviewed changes

serhiy-storchaka left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM. 👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Brett2148 reacted with laugh emoji

tomasr8 reacted with rocket emoji

All reactions

😄 1 reaction
🚀 1 reaction

tomasr8 commented Apr 12, 2025

Copy link

Copy Markdown

Member Author

I'll take care of the backport

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tomasr8 mentioned this pull request

Apr 12, 2025

gh-132435: Test syntax warnings in a finally block #132436

Merged

tomasr8 added a commit to tomasr8/cpython that referenced this pull request


          [3.13] pythongh-131927: Prevent emitting optimizer warnings twice in …

5c30078

…the REPL (pythonGH-131993)

(cherry picked from commit 3d08c8a)

Co-authored-by: Tomas R. <[email protected]>

tomasr8 added a commit to tomasr8/cpython that referenced this pull request


          [3.13] pythongh-131927: Prevent emitting optimizer warnings twice in …

a0685cb

…the REPL (pythonGH-131993)

(cherry picked from commit 3d08c8a)

Co-authored-by: Tomas R. <[email protected]>

tomasr8 added a commit to tomasr8/cpython that referenced this pull request


          [3.13] pythongh-131927: Prevent emitting optimizer warnings twice in …

f0d8917

…the REPL (pythonGH-131993)

(cherry picked from commit 3d08c8a)

Co-authored-by: Tomas R. <[email protected]>

tomasr8 mentioned this pull request

[3.13] gh-131927: Prevent emitting optimizer warnings twice in the REPL (GH-131993) #132463

Merged

bedevere-app Bot commented Apr 13, 2025

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-app Bot removed the needs backport to 3.13 only security fixes label

bedevere-app Bot commented Apr 13, 2025

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

1 similar comment

bedevere-app Bot commented Apr 13, 2025

Copy link

Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka pushed a commit that referenced this pull request


          [3.13] gh-131927: Prevent emitting compiler warnings twice (GH-131993) (

4ff5d88

GH-132463)

(cherry picked from commit 3d08c8a)

serhiy-storchaka mentioned this pull request

Syntax warnings can be swallowed in import #139640

Closed

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request


          pythongh-139640: Fix swallowing syntax warnings in different modules

2fa4266

Revert pythonGH-131993.

Fix swallowing some syntax warnings in different modules if they accidentally
have the same message and are emitted from the same line.

ast.parse() no longer emits syntax warnings for
return/break/continue in finally (see PEP-765) -- they are only
emitted during compilation.

serhiy-storchaka mentioned this pull request

gh-131927: Do not emit PEP 765 warnings in ast.parse() #139642

Merged

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request


          pythongh-139640: Fix swallowing syntax warnings in different modules

c04d874

Revert pythonGH-131993.

Fix swallowing some syntax warnings in different modules if they accidentally
have the same message and are emitted from the same line.

ast.parse() no longer emits syntax warnings for
return/break/continue in finally (see PEP-765) -- they are only
emitted during compilation.

serhiy-storchaka mentioned this pull request

Improve/fix filtering SyntaxWarnings by module #135801

Closed

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request

Oct 7, 2025


          pythongh-139640: Fix swallowing syntax warnings in different modules

Revert pythonGH-131993. Explicitly silence the duplicated PEP 765 syntax warnings
in the REPL.

Fix swallowing some syntax warnings in different modules if they accidentally
have the same message and are emitted from the same line.

Co-authored-by: Tomas R. <[email protected]>

serhiy-storchaka mentioned this pull request

Oct 7, 2025

gh-131927: Silence PEP 765 warnings in PyREPL #139719

Closed

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request

Oct 8, 2025


          pythongh-139640: Fix swallowing syntax warnings in different modules

6cb99f0

Revert pythonGH-131993.

Fix swallowing some syntax warnings in different modules if they accidentally
have the same message and are emitted from the same line.

serhiy-storchaka mentioned this pull request

Oct 8, 2025

gh-139640: Fix swallowing syntax warnings in different modules (restore duplicated PEP 765 warnings) #139755

Merged

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request

Oct 8, 2025


          pythongh-139640: Fix swallowing syntax warnings in different modules

c12e7ec

Revert pythonGH-131993.

Fix swallowing some syntax warnings in different modules if they accidentally
have the same message and are emitted from the same line.

serhiy-storchaka added a commit that referenced this pull request


          gh-139640: Fix swallowing syntax warnings in different modules (GH-13…

279db6b

…9755)

Revert GH-131993.

Fix swallowing some syntax warnings in different modules if they accidentally
have the same message and are emitted from the same line.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request


          pythongh-139640: Fix swallowing syntax warnings in different modules (p…

cd94065

…ythonGH-139755)

Revert pythonGH-131993.

Fix swallowing some syntax warnings in different modules if they accidentally
have the same message and are emitted from the same line.
(cherry picked from commit 279db6b)

Co-authored-by: Serhiy Storchaka <[email protected]>

miss-islington mentioned this pull request

[3.14] gh-139640: Fix swallowing syntax warnings in different modules (GH-139755) #140117

Merged

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request


          [3.13] pythongh-139640: Fix swallowing syntax warnings in different m…

f84e1bd

…odules (pythonGH-139755)

Revert pythonGH-131993.

Fix swallowing some syntax warnings in different modules if they accidentally
have the same message and are emitted from the same line.
(cherry picked from commit 279db6b)

Co-authored-by: Serhiy Storchaka <[email protected]>

serhiy-storchaka mentioned this pull request

[3.13] gh-139640: Fix swallowing syntax warnings in different modules (GH-139755) #140119

Merged

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this pull request


          [3.13] pythongh-139640: Fix swallowing syntax warnings in different m…

72c4058

…odules (pythonGH-139755)

Revert pythonGH-131993.

Fix swallowing some syntax warnings in different modules if they accidentally
have the same message and are emitted from the same line.

Fix duplicated warnings in the "finally" block.

(cherry picked from commit 279db6b)

Co-authored-by: Serhiy Storchaka <[email protected]>

serhiy-storchaka added a commit that referenced this pull request


          [3.13] gh-139640: Fix swallowing syntax warnings in different modules (…

92f7965

…GH-139755) (GH-140119)

Revert GH-131993.

Fix swallowing some syntax warnings in different modules if they accidentally
have the same message and are emitted from the same line.

Fix duplicated warnings in the "finally" block.

(cherry picked from commit 279db6b)

serhiy-storchaka added a commit that referenced this pull request