Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.7k
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Lib/_pyrepl/console.py Outdated
| incomplete_input=False, | ||
| ) | ||
| withwarnings.catch_warnings(record=True)asall_warnings: | ||
| warnings.simplefilter("always") |
iritkatrielApr 1, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
https://docs.python.org/3/library/warnings.html#the-warnings-filter
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 callsPyErr_WarnExplicitObject and sets theregistry (last argument) toNULL. Theregistry 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:
importwarningsdef_compile():compile('1 is 1','input','eval')withwarnings.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?
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?
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 ofcompile. I'll check if there's a nice way to do that.
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 insetup_context:
Lines 911 to 916 in37bc386
| staticint | |
| setup_context(Py_ssize_tstack_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 toNULL):
Lines 1909 to 1910 in37bc386
| 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 toPyErr_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, ®istry))+ 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?
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 callPyErr_WarnExplicitObject with it?
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,intlineno){PyObject*unused_filename,*module,*registry;intunused_lineno;intstack_level=1;if (!setup_context(stack_level,NULL,&unused_filename,&unused_lineno,&module,®istry))return-1;intrc=PyErr_WarnExplicitObject(category,message,filename,lineno,module,registry);Py_DECREF(module);returnrc;}
I'll update the PR to see if everything still works with this change.
Uh oh!
There was an error while loading.Please reload this page.
iritkatriel commentedApr 5, 2025
tests are failing. |
tomasr8 commentedApr 5, 2025
Yep, I suspect it's because |
All commit authors signed the Contributor License Agreement. |
iritkatriel commentedApr 7, 2025
LGTM.@serhiy-storchaka what do you think? |
tomasr8 commentedApr 7, 2025
Ok it seems some other test modified the warning filters so I just needed to add I have another question though, regarding the |
Uh oh!
There was an error while loading.Please reload this page.
| warnings.simplefilter("default") | ||
| console.runsource(code) | ||
| count=sum("'return' in a 'finally' block"instr(w.message) |
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, likeassert(1, 'message') or1 is 1? BTW, they are currently emitted 3 times, not 2.
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 totest_compile for it.
| """) | ||
| withwarnings.catch_warnings(record=True)ascaught: | ||
| warnings.simplefilter("default") |
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"?
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 totest_compile for it
serhiy-storchaka left a comment
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. 👍
tomasr8 commentedApr 12, 2025
I'll take care of the backport |
…the REPL (pythonGH-131993)(cherry picked from commit3d08c8a)Co-authored-by: Tomas R. <tomas.roun8@gmail.com>
…the REPL (pythonGH-131993)(cherry picked from commit3d08c8a)Co-authored-by: Tomas R. <tomas.roun8@gmail.com>
…the REPL (pythonGH-131993)(cherry picked from commit3d08c8a)Co-authored-by: Tomas R. <tomas.roun8@gmail.com>
GH-132463 is a backport of this pull request to the3.13 branch. |
GH-132463 is a backport of this pull request to the3.13 branch. |
1 similar comment
GH-132463 is a backport of this pull request to the3.13 branch. |
RevertpythonGH-131993.Fix swallowing some syntax warnings in different modules if they accidentallyhave the same message and are emitted from the same line.ast.parse() no longer emits syntax warnings forreturn/break/continue in finally (see PEP-765) -- they are onlyemitted during compilation.
RevertpythonGH-131993.Fix swallowing some syntax warnings in different modules if they accidentallyhave the same message and are emitted from the same line.ast.parse() no longer emits syntax warnings forreturn/break/continue in finally (see PEP-765) -- they are onlyemitted during compilation.
RevertpythonGH-131993. Explicitly silence the duplicated PEP 765 syntax warningsin the REPL.Fix swallowing some syntax warnings in different modules if they accidentallyhave the same message and are emitted from the same line.Co-authored-by: Tomas R. <tomas.roun8@gmail.com>
RevertpythonGH-131993.Fix swallowing some syntax warnings in different modules if they accidentallyhave the same message and are emitted from the same line.
RevertpythonGH-131993.Fix swallowing some syntax warnings in different modules if they accidentallyhave the same message and are emitted from the same line.
…ythonGH-139755)RevertpythonGH-131993.Fix swallowing some syntax warnings in different modules if they accidentallyhave the same message and are emitted from the same line.(cherry picked from commit279db6b)Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
…odules (pythonGH-139755)RevertpythonGH-131993.Fix swallowing some syntax warnings in different modules if they accidentallyhave the same message and are emitted from the same line.(cherry picked from commit279db6b)Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
…odules (pythonGH-139755)RevertpythonGH-131993.Fix swallowing some syntax warnings in different modules if they accidentallyhave the same message and are emitted from the same line.Fix duplicated warnings in the "finally" block.(cherry picked from commit279db6b)Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
…GH-139755) (GH-140119)RevertGH-131993.Fix swallowing some syntax warnings in different modules if they accidentallyhave the same message and are emitted from the same line.Fix duplicated warnings in the "finally" block.(cherry picked from commit279db6b)
…GH-139755) (GH-140117)RevertGH-131993.Fix swallowing some syntax warnings in different modules if they accidentallyhave the same message and are emitted from the same line.Fix duplicated warnings in the "finally" block.(cherry picked from commit279db6b)Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>* Update 2025-10-06-10-03-37.gh-issue-139640.gY5oTb.rst---------Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
…ythonGH-139755)RevertpythonGH-131993.Fix swallowing some syntax warnings in different modules if they accidentallyhave the same message and are emitted from the same line.
Uh oh!
There was an error while loading.Please reload this page.
Fix based on the idea from@iritkatriel:#131927 (comment)
PEP 765 is new in 3.14 so I think we can skip a news entry?