GeezerVison is showing github.com — The help function shows incorrect signature for subclass · Issue #105080 · python/cpython
The help function shows incorrect signature for subclass #105080DescriptionBug report
With the following class hierarchy:
class A0:
def __new__(cls, *args, **kw):
return super().__new__(cls)
def __init__(self, *args, **kw):
super().__init__()
class A1(A0):
def __init__(self, a, b):
super().__init__()
self.a = a
self.b = b
class A2(A1):
c = Nonehelp(A2) shows the wrong signature for instantiating A2 as A2(*args, **kw) instead of the expected A2(a, b), despite the fact that is shows the correct signature for __init__:
Help on class A2 in module __main__:
class A2(A1)
| A2(*args, **kw)
|
| Method resolution order:
| A2
| A1
| A0
| builtins.object
|
| Data and other attributes defined here:
|
| c = None
|
| ----------------------------------------------------------------------
| Methods inherited from A1:
|
| __init__(self, a, b)
| Initialize self. See help(type(self)) for accurate signature.
|
| ----------------------------------------------------------------------
| Static methods inherited from A0:
|
| __new__(cls, *args, **kw)
| Create and return a new object. See help(type) for accurate signature.
|
...
Note that help(A1) works correctly and shows the correct signature as A1(a, b):
Help on class A1 in module __main__:
class A1(A0)
| A1(a, b)
|
| Method resolution order:
| A1
| A0
| builtins.object
|
| Methods defined here:
|
| __init__(self, a, b)
| Initialize self. See help(type(self)) for accurate signature.
|
| ----------------------------------------------------------------------
| Static methods inherited from A0:
|
| __new__(cls, *args, **kw)
| Create and return a new object. See help(type) for accurate signature.
|
...
This doesn't seem to be an issue if __new__ is not defined on A0, or if A1 redefines __new__ with the same signature as __init__.
Your environment
- CPython versions tested on: 3.11.3+
- Operating system and architecture: Linux x86
Linked PRs
Activitygaogaotiantian commented on May 30, 2023 This is due to the behavior of inspect.signature(). To be more specific, _signature_from_callable() in inspect.py.
The current behavior to get a signature for a type:
- Check if the type has its own
__new__
- Check if the type has its own
__init__
- Find the user-defined
__new__ in __mro__
- Find the user-defined
__init__ in __mro__
- Non-related searches to this issue
As A1 has a direct defined __init__, it is shown as the signature. For A2, no __init__ or __new__ is defined, but there's a __new__ method defined in its __mro__, so that method was used.
This feels a bit weird to me - like inheritance, the derived class should take the bahavior of closer bases, not further ones. If we can agree this is a bug, I can fix it by going through __mro__ and search for user-defined __new__ and __init__. This would break the old behavior though(obviously).
We can also make it safer - only change this in main and not backport. I guess it's a decision call.
@AlexWaygood danpascu commented on May 30, 2023 From the description this seems to happen because it favors __new__ over __init__ when looking for the signature.
In my experience it's usually __init__ that has the more specific signature, while __new__ has a generic signature, only because it has to accept whatever __init__ accepts, but it doesn't usually care about the actual arguments when building the instance, while __init__ cares about the arguments because it has to use them to initialize the instance.
Wouldn't it make more practical sense to prefer __init__ over __new__, given that it usually has the more specific signature? As I understand the search algorithm mentioned above, it means any class that defines both a generic __new__ and a specific __init__ will still have the same problem, even after the proposed fix and even in the absence of any inheritance. gaogaotiantian commented on May 30, 2023
From the description this seems to happen because it favors __new__ over __init__ when looking for the signature.
In my experience it's usually __init__ that has the more specific signature, while __new__ has a generic signature, only because it has to accept whatever __init__ accepts, but it doesn't usually care about the actual arguments when building the instance, while __init__ cares about the arguments because it has to use them to initialize the instance.
Wouldn't it make more practical sense to prefer __init__ over __new__, given that it usually has the more specific signature? As I understand the search algorithm mentioned above, it means any class that defines both a generic __new__ and a specific __init__ will still have the same problem, even after the proposed fix and even in the absence of any inheritance.
What do you mean by "generic" __new__? You should not define a __new__ method for a class unless you need to do something very specific when creating an instance. The __new__ method in your example code should not be defined(I took it as a proof of concept). For most use cases, __new__ is not defined and __init__ is the only method defined. For the cases where __new__ is defined, it should be critical(for example, singleton class) and should take precedence before __init__. danpascu commented on May 30, 2023 The __new__ method in my example was there as part of a minimal test case that shows the problem.
Consider this example for a "generic" __new__ that has a purpose:
class XMLSimpleElement:
_xml_value_type = None # To be defined in subclass
def __new__(cls, *args, **kw):
if cls._xml_value_type is None:
raise TypeError(f"The {cls.__name__} class cannot be instantiated because it doesn't define the _xml_value_type attribute")
return super().__new__(cls)
def __init__(self, value):
super().__init__()
self.value = value
...
class XMLStringElement(XMLSimpleElement):
_xml_value_type = str
In this case the __new__ method is meant to prevent instantiation for classes that do not define their value type, since they are a kind of an abstract base class that cannot work without knowing the value type, but at the same time it doesn't care about the arguments it receives, nor does it want to match its signature with that of __init__ since a subclass may have an __init__ with a slightly different signature, like:
class XMLSimpleElementWithAttributes(XMLSimpleElement):
def __init__(self, value, *attributes):
super().__init__(value)
self.attributes = attributes
Similarly there are many examples out there of base classes that have a test in their __new__ method that if the class being instantiated is the base class itself, it will raise a TypeError because the base class is not supposed to be instantiated.
In the example above both help(XMLSimpleElement) and help(XMLStringElement) show the wrong signature, while help(XMLSimpleElementWithAttributes) shows the correct signature. gaogaotiantian commented on May 30, 2023 I don't think the example code is the correct way to go.
Your XMLSimpleElement is an abstract class. It feels a bit weird to have an __init__ method on an abstract base class that defines a specific way(that can be overwritten) to initialize.
I think the correct way to do it is to have a pure abstract base class
class XMLBaseElement:
_xml_value_type = None # To be defined in subclass
def __new__(cls, *args, **kw):
if cls._xml_value_type is None:
raise TypeError(f"The {cls.__name__} class cannot be instantiated because it doesn't define the _xml_value_type attribute")
return super().__new__(cls)Then a simple based on that:
class XMLSimpleElement:
def __init__(self, value):
super().__init__()
self.value = valueHaving a __new__ method that is very generic and an __init__ that is very specific just do not add together to me. Maybe others have different opinions.
If the implementation is like this, then all the signatures would be correct with my proposal. danpascu commented on May 30, 2023 Again, it was just an example to show that a generic __new__ and a specific __init__ can coexist. I'm sure it can be refactored to work around the problem. My point is that I'd like something that works without me having to work around the problem.
In the ideal case, the signature lookup should traverse the __mro__ looking for the first method between __new__ and __init__ that has the most specific signature. However it's unclear to me if we can properly define what "the most specific" signature is and how to identify it, let alone the fact that this would probably add a great deal of complexity to the solution.
Anyway, my follow up comments were more about stating my opinion that from my experience preferring __init__ over __new__ when picking the signature seems to work better in more cases and I find the current choice of preferring __new__ over __init__ less practical and more likely to run into issues.
That being said your proposal would definitely improve things and I'd be thankful for it. As for the cases that it would not cover, I guess I can always define __signature__ at the class level to override the choice for signature if I'm not happy with it. gaogaotiantian commented on May 31, 2023 I guess my point is - you can't prove a point with problematic code. Not saying your code is wrong, but if it's not preferable, then changing existing behavior based on that would be a bad idea. That's being said - do you know any real code in a popular repo that has similar issues?
In the docs it mentioned that:
__new__() is intended mainly to allow subclasses of immutable types (like int, str, or tuple) to customize instance creation. It is also commonly overridden in custom metaclasses in order to customize class creation.
I think if you can do something in __init__, you should do it in __init__, not __new__. In your example, you can check the class member in __init__ and it works perfectly fine - so no __new__ is needed. You'll probably say that's a "workaround", but that's probably - probably the correct way to achieve the feature.
I guess one of the reason to favor __new__ over __init__ is that - __new__ is guaranteed to execute, but __init__ is not. I can easily create some meaningful code to create instances of different classes based on the argument then the __init__ method would be from the class of these instances.
class XMLStrElement:
def __init__(self, val):
self.val = val
class XMLIntElement:
def __init__(self, val):
self.val = val
class XMLMultiElement:
def __init__(self, *args):
self.vals = args
class XMLElement:
def __new__(cls, *args):
if len(args) == 1:
if isinstance(args[0], str):
return XMLStrElement(args[0])
elif isinstance(args[0], int):
return XMLIntElement(args[0])
return super().__new__(cls)
else:
return XMLMultiElement(args)
def __init__(self, val):
self.val = val
i = XMLElement(1)
s = XMLElement("s")
m = XMLElement(1, "s")
e = XMLElement(None)
print(i, s, m, e)Instead of personal experience, __new__ is favored over __init__ on a language level.
I think overall, I feel weird about having a generic __new__ and a specific __init__ on the same class - it does not quite make sense to me. Reacted by sunmy2019 and Erlend E. Aasland sunmy2019 commented on May 31, 2023
Wouldn't it make more practical sense to prefer __init__ over __new__, given that it usually has the more specific signature?
No. __new__ can change the object type. You even cannot tell the object type until __new__ is executed, so you cannot decide which __init__ to call. gaogaotiantian commented on May 31, 2023 I do think the original request is valid - if a derived class D defined __init__ method but not __new__, we should use that for signature. And for all the derived classes based on D too.
The existing behavior feels wrong to me - at least we should not have different behavior on class D and D1(D). sunmy2019 commented on May 31, 2023
I do think the original request is valid - if a derived class D defined __init__ method but not __new__, we should use that for signature. And for all the derived classes based on D too.
The existing behavior feels wrong to me - at least we should not have different behavior on class D and D1(D).
I cannot follow. What is D? Can you provide a code snippet? gaogaotiantian commented on May 31, 2023
I do think the original request is valid - if a derived class D defined __init__ method but not __new__, we should use that for signature. And for all the derived classes based on D too.
The existing behavior feels wrong to me - at least we should not have different behavior on class D and D1(D).
I cannot follow. What is D? Can you provide a code snippet?
It's the original code snippet.
class A0:
def __new__(cls, *args, **kw):
return super().__new__(cls)
def __init__(self, *args, **kw):
super().__init__()
class A1(A0):
def __init__(self, a, b):
super().__init__()
self.a = a
self.b = b
class A2(A1):
c = NoneIt does not make sense that A2 and A1 have different signatures. I think the signature for both A1 and A2 should be (a, b). Maybe someone would argue that they should both be (*args, **kw) - we can discuss that. However, I don't think in any case they should be different, that's just wrong. sunmy2019 commented on May 31, 2023
I think the signature for both A1 and A2 should be (a, b).
Not exactly.
With customized metaclass, there are use cases like
class A0:
def __new__(cls, *args, **kw):
class D:
def __init__(self):
super().__init__()
return D()
class A1(A0):
def __init__(self, a, b):
super().__init__()
self.a = a
self.b = b
class A2(A1):
c = None
A2(...) is an instance of D. (You can call with any args/kwargs).
they should both be (*args, **kw)
I think so. danpascu commented on Jun 3, 2023 Thank you. I really appreciate the expedite handling.
May I ask what is the intention with this bug fix? Will it be backported, or is it just going to exist in 3.12 going forward? gaogaotiantian commented on Jun 3, 2023 This is considered a bug fix I believe so it was fixed in main and 3.12(notice that 3.12 is already a backport as we are on 3.13 alpha now). Not sure why it was not merged back to 3.11, that's a question for @carljm . 3.11 is still taking bug fixes right?
We won't port it back further because 3.10 only takes security fix now. So the only version that might be affected at this point is 3.11. danpascu commented on Jun 3, 2023 That's fine. 3.11 is what I'm interested in. If this could land there it would be great. carljm commented on Jun 3, 2023 Sorry, that was my oversight; it should be backported to 3.11. Kicked that off now. carljm commented on Jun 3, 2023 Looks like it does not backport cleanly. I will try to get to the manual backport soon but it may be a few days. @gaogaotiantian if you want to prepare the backport PR sooner (using the cherry_picker tool) I will be happy to review and merge it. gaogaotiantian commented on Jun 4, 2023
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|