gh-122595: Add more error checks in the compiler

[serhiy-storchaka]

• Issue: Add error checks in the compiler #122595

[iritkatriel]

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.

[carljm]

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.

[serhiy-storchaka]

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 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.

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 introducing PyErr_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.

[carljm]

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.

[vstinner]

LGTM

[vstinner]

Maybe keep the check as an assertion? assert(args != NULL)? Same remark in symtable_visit_params().

[carljm]

This looks good to me; thanks for the cleanup!