-
-
Notifications
You must be signed in to change notification settings - Fork 30k
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-122595: Add more error checks in the compiler #122596
gh-122595: Add more error checks in the compiler #122596
Conversation
serhiy-storchaka
commented
Aug 2, 2024
•
edited by bedevere-app
bot
Loading
edited by bedevere-app
bot
- Issue: Add error checks in the compiler #122595
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
Python/symtable.c
Outdated
assert(PyLong_Check(v)); | ||
return PyLong_AS_LONG(v); | ||
if (!v) { | ||
return PyErr_Occurred() ? -1 : 0; |
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.
Generally the symtable functions return 0 for success and 1 for failure (but I think not always). It's probably worth changing that before adding these error checks, it's becoming confusing.
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.
The current pattern in symtable is 0 on failure (usually with an exception set -- though clearly, as in this case, it's not always consistent), and 1 on success.
There are some functions that don't follow this pattern. _PySymtable_LookupOptional
is a wrapper around PyDict_GetItemRef
, so follows its 1/0/-1 return value.
symtable_visit_params
and symtable_visit_argannotations
return -1 if given a null args
; this looks like just a mistake, but the case is never hit because every call site of these functions null-checks the argument first.
I would support a PR to switch symtable to match the preferred return value conventions.
In this particular case, why not switch to PyDict_GetItemRef
rather than introducing PyErr_Occurred
?
We could make _PyST_GetSymbol
another special case that returns -1
for error instead of 0
(even without making that change throughout the file.) It's clearer that -1
is an invalid symbol representation, though technically 0
is just as invalid, so it's not necessary to use -1
.
If the symbol is missing but no error occurs, I think that always represents a bug in the symbol table implementation, and we should set an exception and return the same exception failure code. Silently returning 0 to be interpreted by the caller as "no flags" is not expected and will just lead to bugs.
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 initially make it returning 0 on error, but then every caller was needed to check PyErr_Occurred()
. In particularly RETURN_IF_ERROR()
could not be used.
symtable_visit_params
andsymtable_visit_argannotations
return -1 if given a nullargs
; this looks like just a mistake, but the case is never hit because every call site of these functions null-checks the argument first.
Not only this. You can remove both checks and all will work. asdl_seq_LEN()
supports NULL, so in these cases they will be just empty loops.
I would support a PR to switch symtable to match the preferred return value conventions.
This is more painful, because every if (!symtable_...)
should be replaced with if (symtable_... < 0)
or like. A lot of code churning.
In this particular case, why not switch to
PyDict_GetItemRef
rather than introducingPyErr_Occurred
?
Done. When I switched to PyDict_GetItemRef
in the rest of the code, I omitted this file because it would be large code churning without visible benefits (at that moment).
If the symbol is missing but no error occurs, I think that always represents a bug in the symbol table implementation, and we should set an exception and return the same exception failure code. Silently returning 0 to be interpreted by the caller as "no flags" is not expected and will just lead to bugs.
Currently the code crashes if set an exception and return the failure code.
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.
Currently the code crashes if set an exception and return the failure code.
Ugh, OK. At some point I'm curious to look at which cases rely on this. But if this is the situation, then tri-value makes sense here. It's just too bad that we make the return values even more confusing in this file. But the only solution to that is to fully adopt "-1 for error" throughout the file, and as you say, this is a lot of code churn.
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
@@ -2650,9 +2712,6 @@ symtable_visit_argannotations(struct symtable *st, asdl_arg_seq *args) | |||
{ | |||
Py_ssize_t i; | |||
|
|||
if (!args) |
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 keep the check as an assertion? assert(args != NULL)? Same remark in symtable_visit_params().
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.
This looks good to me; thanks for the cleanup!