Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Python 3.11.9 catches all exceptions raised in Generic.__setattr__("__orig_class__", type_parameter) #117744

Closed
nanne-aben opened this issue Apr 11, 2024 · 8 comments
Labels
3.11 only security fixes 3.12 bugs and security fixes 3.13 bugs and security fixes topic-typing type-bug An unexpected behavior, bug, or error

Comments

@nanne-aben
Copy link

nanne-aben commented Apr 11, 2024

Bug report

Bug description:

In Python 3.11.9, the following code does not raise an Exception:

from typing import Any, Generic, TypeVar

T = TypeVar("T")


class DataSet(Generic[T]):
    def __setattr__(self, name: str, value: Any) -> None:
        object.__setattr__(self, name, value)

        if name == "__orig_class__":
            raise Exception("Just randomly raising an exception for illustrative purposes.")


class Schema:
    pass


# initializing an object with type parameter `Schema` will call 
# `__setattr__("__orig_class__", (Schema,))`
df = DataSet[Schema]()

In other versions of Python (e.g. 3.11.8 or 3.12.2) this raises an Exception, but in Python 3.11.9 it appears this Exception is caught.

I use something similar to the above logic in typedspark to check whether the DataSet follows the schema specified in Schema, and I raise an Exception if it doesn't. As of Python 3.11.9, these exceptions are caught, hence breaking my logic.

Thank you for looking into this!

CPython versions tested on:

3.9, 3.10, 3.11, 3.12

Operating systems tested on:

Linux, macOS, Windows

@aisk
Copy link
Member

aisk commented Apr 11, 2024

I'm using Windows, and found that Python 3.12.3 and current main branch (02f1385) doesn't raise the exception too.

@aisk aisk added 3.12 bugs and security fixes 3.13 bugs and security fixes 3.11 only security fixes labels Apr 11, 2024
@nanne-aben
Copy link
Author

nanne-aben commented Apr 11, 2024

It looks like this behavior was introduced in this PR, corresponding to this ticket.

@hmc-cs-mdrissi
Copy link
Contributor

hmc-cs-mdrissi commented Apr 11, 2024

__orig_class__ itself is semi-documented state. It's documented in PEP but as internal typing implementation detail. A public api to retrieve them did get added here, but I think setting/modifying them remains internal behavior/undocumented on whether it should work.

Also from motivation of that PR I don't see a reasonable way to revert that pr without breaking original issue it was intended for. So I'd discourage relying on modifying __orig_class__ or if you do accepting that it's unstable and may change rules.

edit: I misremembered PEP 560. __orig_bases__ is mentioned in PEP with typing internal motivation. __orig_class__ I'm unsure if it is mentioned at all in any public documentation/pep.

@nanne-aben
Copy link
Author

Thank you for your reply!

orig_class itself is semi-documented state. It's documented in PEP but as internal typing implementation detail. A public api to retrieve them did get added #101688, but I think setting/modifying them remains internal behavior/undocumented on whether it should work.

RIght, I appreciate that __orig_class__ is an internal typing implementation detail and hence I cannot trust its interface to remain stable. That said...

Also from motivation of that PR I don't see a reasonable way to revert that pr without breaking original issue it was intended for.

Catching all Exceptions seems a bit extreme to me. Why not catch TypeError and AttributeError specifically? That would solve the original issue, right?

@JelleZijlstra
Copy link
Member

Tweaking the set of exceptions caught is bound to cause more bugs in the future, if someone has a buggy __setattr__ that raises an unexpected exception.

My inclination is to close this as "won't fix", and decide that your use case is unsupported.

@AlexWaygood
Copy link
Member

AlexWaygood commented Apr 12, 2024

Why not catch TypeError and AttributeError specifically?

Honestly, I think the only reasonable exception for people to raise in __setattr__ methods, property setters and similar is AttributeError. Which would imply that we should only catch AttributeError when setting __orig_class__ (the behaviour we had prior to #115213). But in the "real world", there's lots of "unreasonable" code out there -- it's sadly pretty common for people to raise all sorts of exceptions from __setattr__ methods.

Note that although #115165 was only reported regarding typing.Annotated, the actual bug there affected all generic aliases, such that code like this would raise RuntimeError:

class Foo[T]:
    def __setattr__(self, attr):
        raise RuntimeError("NO!")

Foo[int]()

That feels like really surprising behaviour to me; I think it would be unacceptable to go back to that.

I agree with @JelleZijlstra here; I think we have to close this as "won't fix".

@AlexWaygood AlexWaygood closed this as not planned Won't fix, can't repro, duplicate, stale Apr 12, 2024
@nanne-aben
Copy link
Author

Alright, got it! Think I might have found a way to resolve this without relying on __orig_class__ too, which is probably for the best.

Thanks for you time and feedback!

@AlexWaygood
Copy link
Member

No problem -- thanks for the well-written bug report @nanne-aben!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.11 only security fixes 3.12 bugs and security fixes 3.13 bugs and security fixes topic-typing type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

5 participants