Should `decimal.Context.__delattr__` be in the stub?

[AlexWaygood]

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

In 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 deleted

Maybe we could represent this by adding Context.__delattr__ back to the stub and giving it the signature def __delattr__(self, __name: str) -> NoReturn?

[AlexWaygood]

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

	# __setattr__() only allows to set a specific set of attributes,
	# already defined above.

[AlexWaygood]

_threading_local.local.__delattr__ was also deleted in #8483.

Unlike _decimal.Context.__delattr__, this actually didn't have the same signature as object.__delattr__: while the name argument is positional-only in object.__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.local be 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]

Sure, if there are semantic differences, we should add them back.

[AlexWaygood]

@erictraut, if we added decimal.Context.__delattr__ back to the stub and gave it the signature def __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]

Pyright doesn't pay attention to the return type of __delattr__, so returning NoReturn will 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]

Thanks @erictraut! Let's leave __delattr__ off decimal.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.