bpo-41060: Avoid SEGFAULT when calling GET_INVALID_TARGET in the grammar

[lysnikolaou]

GET_INVALID_TARGET might unexpectedly return NULL, which if not
caught will cause a SEGFAULT. Therefore, this PR introduces a new
inline function RAISE_SYNTAX_ERROR_INVALID_TARGET that always
checks for GET_INVALID_TARGET returning NULL and can be used in
the grammar, replacing the long C ternary operation used till now.

https://bugs.python.org/issue41060

[pablogsal]

Seems that there is something missing still:

test_assign_call (test.test_syntax.SyntaxTestCase) ... ok
test_assign_del (test.test_syntax.SyntaxTestCase) ... python: Parser/pegen.h:164: CHECK_CALL: Assertion `PyErr_Occurred()' failed.
Fatal Python error: Aborted

[pablogsal]

I have to say that I am a bit scared about how we are dealing with this part, it seems that is super easy to forget some combination that segfaults :(

[lysnikolaou]

I have to say that I am a bit scared about how we are dealing with this part, it seems that is super easy to forget some combination that segfaults :(

I agree that this approach makes it fairly easy to miss corner cases. Still, I'm having a hard time figuring out what other approaches we could think of that don't.

[pablogsal]

I agree that this approach makes it fairly easy to miss corner cases. Still, I'm having a hard time figuring out what other approaches we could think of that don't.

A lot of the time the problem is that we pass NULL to PyPegen_get_expr_name so maybe we should add code to this function that returns something generic so the we do not crash (maybe something that makes the error generic like 'Cannot assign to invalid target').

[lysnikolaou]

I agree that this approach makes it fairly easy to miss corner cases. Still, I'm having a hard time figuring out what other approaches we could think of that don't.

A lot of the time the problem is that we pass NULL to PyPegen_get_expr_name so maybe we should add code to this function that returns something generic so the we do not crash (maybe something that makes the error generic like 'Cannot assign to invalid target').

That is what this PR does, no? The return value of GET_INVALID_TARGET gets checked, before passing it to _PyPegen_get_expr_name. If it's NULL, RAISE_SYNTAX_ERROR("invalid_syntax") gets called instead.

[pablogsal]

If we backport this PR, are these compatible with the old parser?

[lysnikolaou]

Yup yup, they should be.

[pablogsal]

That is what this PR does, no? The return value of GET_INVALID_TARGET gets checked, before passing it to _PyPegen_get_expr_name. If it's NULL, RAISE_SYNTAX_ERROR("invalid_syntax") gets called instead.

Yep yep, I wrote that before checking 😅 What I had in mind is some in-between message not that generic as invalid syntax but a bit more specialized ("Cannot assign/delete invalid target" or something like that). But I am fine with invalid syntax though.

[lysnikolaou]

"Cannot assign/delete invalid target" or something like that

This error message would be misleading in some cases though. For the input with a as b for example, the target is fine. It is the missing colon afterwards that's wrong.

[miss-islington]

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

[miss-islington]

Sorry, @lysnikolaou and @pablogsal, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 6c4e0bd974f2895d42b63d9d004587e74b286c88 3.9

[pablogsal]

Seems that we need to do a manual backport

[lysnikolaou]

Seems that we need to do a manual backport

No worries, I'll do it.

[bedevere-bot]

GH-21024 is a backport of this pull request to the 3.9 branch.