GeezerVison is showing github.com — Instance attr error suggestions can execute `__getattr__` · Issue #132385 · python/cpython
Instance attr error suggestions can execute __getattr__ #132385DescriptionBug 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.
Linked PRs
Activitypablogsal commented on 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 similar skirpichev commented on Apr 11, 2025
We probably want to silence all errors.
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 commented on Apr 11, 2025
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
@pablogsal yes, I understand that and agree.
But, I think that error suggestions are a bit unique here. Because most of the time NameError happens when code is not working as intended. And at the same time perfectly valid __getattr__ can produce things like AttributeError, 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 handle Exception. Reacted by Pablo Galindo Salgado pablogsal commented on 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 commented on Apr 11, 2025
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
KeyboardInterrupt comes from the system, not from the object. millerdev commented on Apr 11, 2025 It's true, it was a very contrived example. SystemExit was used for dramatic effect, but is admittedly quite unrealistic.
A far more likely scenario would be a NameError resulting 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 commented on Apr 11, 2025 self.__dict__ assumes that we have __dict__ and working __getattribute__. So, let's keep hasattr in place, but just silence exceptions :)
pablogsal commented on Apr 11, 2025
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
KeyboardInterrupt comes from the system, not from the object.
Yeah, but this is not because someone would override stuff but because someone sent the signal no?
A far more likely scenario would be a NameError resulting 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.
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 commented on 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) added a commit that references this issue on May 2, 2025gh-132385: Fix instance error suggestions trigger potential exception…
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|