Title: Allow _PyType_Lookup() to raise exceptions
Type: behavior Stage: patch review
Components: Interpreter Core Versions: Python 3.7
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: scoder, serhiy.storchaka, vstinner
Priority: normal Keywords: patch

Created on 2017-09-14 08:07 by scoder, last changed 2017-10-01 08:44 by serhiy.storchaka.

Pull Requests
URL Status Linked Edit
PR 3616 open scoder, 2017-09-16 11:55
Messages (6)
msg302156 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2017-09-14 08:07
Follow-up to issue 31336:
The fact that _PyType_Lookup() does not propagate exceptions leaves some space for ambiguity. If, in a chain of MRO lookups, one would fail and a later one would succeed, is it correct to keep trying? What if the first failure actually failed to see an otherwise existing attribute in that class? We simply can't know because the lookup failed for *some* reason.

I think, the only way to really avoid that ambiguity would be to allow _PyType_Lookup() to raise exceptions, and to make it fail on the first error.
msg302211 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2017-09-14 21:22
I'm working on a PR for this, but after changing all usages and fixing up some error handling here and there, it results in an interpreter crash for me. I'll try to debug it during the next days.
msg302336 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2017-09-16 11:59
Test suite passes now. The crash was due to an uninitialised error flag in one case, which lead the C compiler to do incorrect optimisations on undefined behaviour.
msg302413 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2017-09-18 07:19
Question: Do you think it's ok to change the signature of _PyType_Lookup() in this way by adding an error flag, or should I add a new function instead?

There is no performance difference to PR 3279 since gcc should optimise this flag properly away in most cases, so it's not a question about performance but about backwards compatibility. It's an internal, private function, which suggests that it should be ok, but it's not my decision. Cython calls it in a couple of cases, which I will obviously adapt in the next release, if the signature change is accepted.

Should the "no MRO after readying" error case be kept? It's the only non-exception error case, setting the error flag to 1. It seems ok to ignore this case, so it's not really an error, somehow. I would change it to return 0 (instead of 1), and thus change the error flag to simply "-1 on error (with exception set), 0 on success". Opinions?
msg303009 - (view) Author: Stefan Behnel (scoder) * (Python committer) Date: 2017-09-26 05:58
Any comments on this?
msg303456 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-10-01 08:44
I'm positive to this change in general, but this is too complex and delicate code. In needs careful reviewing.
Date User Action Args
2017-10-01 08:44:44serhiy.storchakasetpull_requests: - pull_request3632
2017-10-01 08:44:38serhiy.storchakasetmessages: + msg303456
2017-09-28 17:07:33pitrousetnosy: - pitrou
2017-09-28 16:32:33scodersetnosy: + vstinner
2017-09-26 05:58:57scodersetmessages: + msg303009
2017-09-18 07:28:41scodersetpull_requests: + pull_request3632
2017-09-18 07:19:37scodersetmessages: + msg302413
2017-09-16 11:59:19scodersetmessages: + msg302336
2017-09-16 11:55:02scodersetkeywords: + patch
stage: patch review
pull_requests: + pull_request3607
2017-09-14 21:22:33scodersetnosy: + pitrou, serhiy.storchaka
messages: + msg302211
2017-09-14 08:07:19scodercreate