-
-
Notifications
You must be signed in to change notification settings - Fork 30.8k
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
The help function shows incorrect signature for subclass #105080
Comments
This is due to the behavior of The current behavior to get a signature for a
As 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 We can also make it safer - only change this in main and not backport. I guess it's a decision call. |
From the description this seems to happen because it favors In my experience it's usually Wouldn't it make more practical sense to prefer |
What do you mean by "generic" |
The Consider this example for a "generic"
In this case the
Similarly there are many examples out there of base classes that have a test in their In the example above both |
I don't think the example code is the correct way to go. Your 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 = value Having a If the implementation is like this, then all the signatures would be correct with my proposal. |
Again, it was just an example to show that a generic In the ideal case, the signature lookup should traverse the Anyway, my follow up comments were more about stating my opinion that from my experience preferring 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 |
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:
I think if you can do something in I guess one of the reason to favor 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, I think overall, I feel weird about having a generic |
No. |
I do think the original request is valid - if a derived class The existing behavior feels wrong to me - at least we should not have different behavior on class |
I cannot follow. What is |
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 = None It does not make sense that |
Not exactly. With customized metaclass, there are use cases like
I think so. |
The code does not make sense - yes it's theoretically possible, but it's invalid code. Could you find any real code that has a similar code structure? The existing behavior of |
I'm not sure why we ended up dissecting the test code and analyzing it as if it's some real code. All of it was typed on the spot as a minimal test case to highlight the problem. Saying it's problematic code just sidesteps the issue. We can discuss it at the concept level if you prefer. I find it problematic that a class that defines both Now you say that is not "preferred" code to have a generic
A quick search in the base library shows this example in unittest/mock:
which will show the wrong signature in
I disagree. The reason to put it in
That is a good point that I forgot about. But I would argue that it happens in cases like unpickling or returning instances of other types from
There are many things that I consider wrong with this example, but what
It doesn't matter how you feel about it. It is allowed by the language and other people will use it in cases similar to what my previous examples showed and then While one can argue that with
which is both wrong (since it's actually the |
Agree.
I hope not. That would be worse than now. A very strong -1 from me on this idea. |
This is a very interesting example because that was explicitly fixed in #100252. Should we take it as a hint that this is wrong(or at least not preferred)?
The reason we discussed The reason a case where At this point, we can't even confidently say that "specific" should be prioritized before "generic" - it is true for some cases, but not always. That's why I'm asking for real examples - we'll have a better idea about how people are actually writing code and try to provide a better(not always "correct") signature for the classes. |
I don't see why we should make that assumption. The commit title is "3.8x speed improvement in (Async)Mock instantiation". Maybe the change is related to that. Or maybe they noticed that the signature in |
Of course it's possible(or likely) that the fix is for the speed up - I'm simply stating that having an inconsistent signature for I'm not picking on you, because this is not my decision. You want favoring A good evidence to convince core devs to change the current behavior is a couple of real code examples (it would be great that they are in CPython repo, but other popupar repos work too). I'm pretty confident that my proposal earlier can get through - that's an enhancement to existing behavior (or a bug fix). If you want something more, you probably need more. For now, it seems like this is not a super interesting topic to core devs - we can try to ask someone for their opinion, but it would be nice if we prepared it well. And of course, there would be no guarantee that this will be fixed (or addressed) because CPython is still basically driven by volunteers :) |
We'll have to agree to disagree on this. As far as I'm concerned I see no issue with a generic From my point of view, if a method doesn't use some arguments, there is no point in specifying them in the signature, just to make the signature match with the other method. In fact many IDEs will flag such a usage with a warning that the arguments specified in the signature are not used in the body of the function, yet they do not flag a signature like Anyway, I don't think we'll convince one another to change our view on this so let's leave it at that.
I found a couple more but they seemed irrelevant to our discussion:
Class
I do not feel picked on. I just understand we have different points of view.
I do not "want" it that way, I just stated that based on my experience that seems to work better, but I'm open to arguments why the other way around is preferable, though so far I have not seen anything to make me change my mind. That being said, I also understand that this would require a backward incompatible change and that is a very big can of worms that probably none wants to touch, so and it's very unlikely to happen.
I agree. The original proposal is already an improvement over what we have now, even if it doesn't fix everything. Truth being told, I don't think a general solution that works in every case can be found for this anyway.
Let's leave the change in behavior out for now. I'm perfectly fine with the original bug fix proposal. While I believe that the other way around would be better, I also understand perfectly well that such a backward incompatible change in behavior is very unlikely to happen and even if it does, it would probably take years of discussions.
No, I think it would be easier and more expedient to leave the "more" out for now and just go for the bug fix. |
…honGH-105217) (cherry picked from commit 9ad199b) Co-authored-by: Tian Gao <gaogaotiantian@hotmail.com>
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? |
This is considered a bug fix I believe so it was fixed in We won't port it back further because |
That's fine. 3.11 is what I'm interested in. If this could land there it would be great. |
Sorry, that was my oversight; it should be backported to 3.11. Kicked that off now. |
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. |
…es (pythonGH-105217). (cherry picked from commit 9ad199b) Co-authored-by: Tian Gao <gaogaotiantian@hotmail.com>
…es (pythonGH-105217). (cherry picked from commit 9ad199b) Co-authored-by: Tian Gao <gaogaotiantian@hotmail.com>
Bug report
With the following class hierarchy:
help(A2)
shows the wrong signature for instantiating A2 asA2(*args, **kw)
instead of the expectedA2(a, b)
, despite the fact that is shows the correct signature for__init__
:Note that
help(A1)
works correctly and shows the correct signature asA1(a, b)
: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
Linked PRs
The text was updated successfully, but these errors were encountered: