gh-121804: always show error location for SyntaxError's in basic repl

[skirpichev]

• Issue: The new repl sometimes does not show the error location when SyntaxError is raised #121804

[pablogsal]

Sorry @skirpichev but I think this is probably too much complexity (even if the code it's not too difficult to follow) for the fallback that it's not going to be triggered most of the time. Let me check with more core devs to get their input first

@ambv@lysnikolaou what do toy think?

[pablogsal]

If nobody answers by Wednesday next week, ping me and we can merge it. After looking at it I think it may be fine 👍

Ensure to run a bunch of ref leaks manual checks by hand, please. Unfortunately running them in the REPL it's not trivial, but yo can manually compare the output of sys.gettotalrefcount before and after raising.

[skirpichev]

but yo can manually compare the output of sys.gettotalrefcount before and after raising.

@pablogsal, I think that there is no reference leaks, but I can't justify this by sys.gettotalrefcount() output:

$ PYTHON_BASIC_REPL=1 ./python -q >>> import gc, sys >>> sys.gettotalrefcount() 61809 >>> gc.collect() 106 >>> sys.gettotalrefcount() 60837 >>> deff(x, x): ... ... File "<stdin>-4", line 1 def f(x, x): ... ^ SyntaxError: duplicate argument 'x' in function definition >>> gc.collect() 0 >>> sys.gettotalrefcount() 60871 >>> gc.collect() 0 >>> sys.gettotalrefcount() 60883

[lysnikolaou]

@lysnikolaou what do toy think?

Sorry for not replying earlier. I was out on vacation.

I don't have a strong opinion either way, but I agree with the sentiment that might be a bit too complex. It improves a subset of SyntaxErrors for a subset of users that will continue to use the old REPL after they've switched to 3.13 (I don't think this could be backported to earlier versions) with a fairly complicated approach.

[picnixz]

Personally, I think it's better for the user's experience to show syntax errors correctly, but I don't know whether the basic REPL will be used a lot or not and whether those kind of errors are shown a lot or not. So I'm +0.5.

[miss-islington-app]

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

[bedevere-app]

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

[pablogsal]

Thanks for the pr @skirpichev and thanks @picnixz for your comments 🚀

[hugovk]

@skirpichev For next time, please could you upgrade to blurb 1.2+ so we don't get spaces in the news file path?

https://discuss.python.org/t/new-blurb-1-2-please-upgrade/59159

No need to change this news file. Thanks!

[skirpichev]

On Wed, Sep 04, 2024 at 04:15:07AM -0700, Hugo van Kemenade wrote: ***@***.*** For next time, please could you upgrade to blurb 1.2+ so we don't get spaces in the news file path?

Thanks for remind this. I did upgrade of my toolchain. But, perhaps, we should update devguide to specify minimal version of blurb or enforce upgrade to latest stable version?

[hugovk]

I plan on enforcing it via CI at some point, maybe after 3.13.0 or 3.14.0a1?

Something like hugovk@e4cc2f7 -> https://github.com/hugovk/cpython/actions/runs/10576194271/job/29301498185

We can't enforce a minimal blurb version as such, people can also use Blurb It (already updated to use underscores) or write them by hand.