Repository navigation
Instance attr error suggestions can execute __getattr__ #132385
Description
Bug report
>>> class A:
... def __getattr__(self, key):
... if key == 'foo': raise SystemExit('bye')
... def bar(self):
... foo
...
>>> A().bar()
bye
# interperter exits :(Originally found by @millerdev in #99140 (comment)
I think that this is not ideal. We probably want to silence all errors. I have a PR ready.
Activity
pythongh-132385: Fix instance error suggestions side effects in `trac…
pablogsal commentedon Apr 11, 2025
I am ok being more defensive but this is a known limitation in general and was discussed plenty of times. If users create an object with known unwanted effects on things like
__getattr__or__dir__there are basically on their own since the VM can call those at any point. The same if some user adds side effects to__repr__or similarskirpichev commentedon Apr 11, 2025
You meant except AttributeError? Hmm, why? Accordingly to the docs, the dunder method in example is broken. But I doubt we should silently hide this from the end user.
The SystemExit is a special snowflake: no traceback will be printed. But I think that the example is slightly artificial in using that specific exception. With a different you will got something more meaningful. Well, when issues like #129605 will be fixed in the new REPL;-)
sobolevn commentedon Apr 11, 2025
@pablogsal yes, I understand that and agree.
But, I think that error suggestions are a bit unique here. Because most of the time
NameErrorhappens when code is not working as intended. And at the same time perfectly valid__getattr__can produce things likeAttributeError,TypeError, etc.Here's the demo of the difference between a regular
NameError:2025-04-11.13.36.02.mov
And a side-effect which terminates the whole REPL:
2025-04-11.13.35.29.mov
I propose to fix the second behavior.
@iritkatriel suggested that we should not catch
BaseException, I agree. I will change by PR to only handleException.pablogsal commentedon Apr 11, 2025
Yeah I am not against fixing the second behaviour I am just being cautious of not trying to go crazy and promise things that would complicate or restrict everything
iritkatriel commentedon Apr 11, 2025
KeyboardInterrupt comes from the system, not from the object.
millerdev commentedon Apr 11, 2025
It's true, it was a very contrived example.
SystemExitwas used for dramatic effect, but is admittedly quite unrealistic.A far more likely scenario would be a
NameErrorresulting in attribute lookup that causes a unintended side effect like hitting a database or loading a cache, that then further complicates debugging attempts by distracting from the real problem.Would it make sense to use an attribute lookup strategy more like
attr in self.__dict__or something along those lines that does not trigger property accessors or special methods like__getattr__?sobolevn commentedon Apr 11, 2025
self.__dict__assumes that we have__dict__and working__getattribute__. So, let's keephasattrin place, but just silence exceptions :)pablogsal commentedon Apr 11, 2025
Yeah, but this is not because someone would override stuff but because someone sent the signal no?
This is precisely what I meant: if the user is implementing that behavior then is up to them, we should not try to protect anything here or we will be drowning in complexity.
picnixz commentedon Apr 11, 2025
(sorry, I thought that the error was at the interpreter's level but it's more in
traceback; thus I added the triaged label to be sure I'm not passing over that issue again)gh-132385: Fix instance error suggestions trigger potential exception…
pythongh-132385: Fix instance error suggestions trigger potential exc…
[3.13] gh-132385: Fix instance error suggestions trigger potential ex…
pythongh-132385: Fix instance error suggestions trigger potential exc…