Repository navigation
Should decimal.Context.__delattr__ be in the stub? #8504
Description
#8483 by @sobolevn deleted decimal.Context.__delattr__ from the stub, on the grounds that it had the exact same signature as builtins.object.__delattr__.
I think the rationale for this deletion was incorrect, as overriding __delattr__ on a subclass can cause a type checker to give that class special semantics, even if the signature of __delattr__ is identical to object.__delattr__. For example:
class Foo: ...
class Bar:
def __delattr__(self, __name: str) -> None: ...
f = Foo()
b = Bar()
del f.spam # mypy and pyright both emit an error here
# mypy emits an error here (it does not special-case __delattr__)
# pyright does *not* emit an error here (it special-cases __delattr__ to allow arbitrary deletions if __delattr__ is overridden)
del b.spamIn the specific case of Context.__delattr__, however, I'm not sure whether it should be present in the stub or not. This is the behaviour you get at runtime:
>>> import decimal
>>> c = decimal.Context()
>>> c.prec = 54
>>> del c.prec
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: context attributes cannot be deletedMaybe we could represent this by adding Context.__delattr__ back to the stub and giving it the signature def __delattr__(self, __name: str) -> NoReturn?
Activity
AlexWaygood commentedon Aug 8, 2022
Whatever the case, we should probably delete this orphaned comment, which was there to explain why
__delattr__was present in the stub but__setattr__was not:typeshed/stdlib/_decimal.pyi
Lines 201 to 202 in d7a5a14
AlexWaygood commentedon Aug 8, 2022
_threading_local.local.__delattr__was also deleted in #8483.Unlike
_decimal.Context.__delattr__, this actually didn't have the same signature asobject.__delattr__: while thenameargument is positional-only inobject.__delattr__, it is positional-or-keyword on_threading_local.local.__delattr__.Also unlike
_decimal.Context.__delattr__, you can also make a clear case for why it should be in the stub in terms of the special semantics of__delattr__. The docs make clear that the whole point of this class is that you should be able to dynamically set and delete attributes, so it makes sense that type-checkers should special-case the class to allow that kind of thing. It's also important that our stub for_threading_local.localbe kept in sync with the C version of the class,threading.local(which at runtime is actually_thread._local).I propose that we add
_threading_local.local.__delattr__back to the stub, and add a test case for_threading_local.local/threading.local.srittau commentedon Aug 8, 2022
Sure, if there are semantic differences, we should add them back.
AlexWaygood commentedon Aug 8, 2022
@erictraut, if we added
decimal.Context.__delattr__back to the stub and gave it the signaturedef __delattr__(self, __name: str) -> NoReturn(to indicate that trying to delete any attributes on instances of the class fails at runtime), how would that interact with pyright's special-casing of__delattr__?erictraut commentedon Aug 8, 2022
Pyright doesn't pay attention to the return type of
__delattr__, so returningNoReturnwill have no effect.If you want to reflect that
__delattr__will generate an exception at runtime, I would recommend simply omitting it from the stub, like you have already done. That way, pyright and mypy (and presumably the other type checkers) will generate an error during type checking.If a class is designed to support arbitrary additions and deletions of attributes, then adding an explicit
__delattr__to that class will tell pyright that it should not generate an error if someone attempts to delete an attribute.AlexWaygood commentedon Aug 8, 2022
Thanks @erictraut! Let's leave
__delattr__offdecimal.Context, in that case (but improve the comment about why it's not there), and add it back to_threading_local.local, with a test case.