-
-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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
Improve super()
error message and document zero-arg super()
usage inside nested functions and generator expressions.
#113212
Comments
cc @carljm |
After digging into source code I found out that super takes its object argument from fist local variable which is Lines 10454 to 10472 in f26bfe4
So I can trigger unpredicted behaviour with the following code: class Base:
def foo(self):
return self.val
class Derived(Base):
def __init__(self, val):
self.val = val
def foo(self):
def inner(other: Derived):
return super().foo() + other.val
return inner
b1 = Derived(1)
b10 = Derived(10)
print(b1.foo()(b10)) Expected output is print(d1.foo()(1)) This will throw: TypeError: super(type, obj): obj must be an instance or subtype of type which could be extremely confusing to user. |
Hi! Thanks for the report. This report covers a lot of territory, and it will be useful to clarify a few things. Most of the behavior you describe here has been true for the entire life of Python 3, since zero-argument So the only recent change in behavior here is in your "list comprehension" example, where code that raised a TypeError in all previous versions of Python 3 now works in Python 3.12, because the comprehension no longer implicitly creates a nested function. All of your other examples behave the same in all versions of Python 3; none of them worked in 3.11 and stopped working in 3.12. The first and most important question: is the change in Python 3.12 a regression, and should we attempt to restore the Python 3.11 behavior? I don't think so: there is no value to the previous TypeError, and the new behavior is both more intuitive and more useful. So I consider this a beneficial side effect of PEP 709 that doesn't require any further action. Next question: should any of the other longstanding behaviors of zero-argument Last question: are there any improvements to documentation or error messages warranted here? I think so: the current documentation for zero-argument And I also think the error message "super(type, obj): obj must be an instance or subtype of type" could be clearer. The biggest improvement IMO would be for the error to name the specific |
super()
not working inside generatorssuper()
to work inside generator expressions
Thanks for commenting on this. I do agree that making it work inside nested functions/generators and generator expressions is not straightforward and would require careful design, maybe even worth a separate PEP 🤔 ? Currently Lines 1314 to 1323 in d91e43e
and copied every time from closure to nested function/generator, generator expression by: cpython/Python/generated_cases.c.h Lines 2110 to 2125 in d91e43e
so maybe implementing something similar with
I also agree with:
But this is true when you explicitly define function/generator. When you have generator expression (which is converted to generator implicitly), it's first argument internally would be class Base:
def foo(self):
return 1
class Derived(Base):
def __init__(self, val=10):
self.it = iter(range(val))
def __iter__(self):
return self
def __next__(self):
return next(self.it)
def foo(self):
return list(super().foo() for _ in Derived(10))
b1 = Derived()
print(b1.foo()) now first argument is in fact instance of enclosing class and everything starts working. But user has no idea how generator expression is implemented under the hood and that This would be very helpful:
Also agree with you:
So the only concern is generator expression as it is not obvious to user where that “first argument” comes from. But it can be deducted from potential improved error message revealing types of |
zero-arg
So, instead of explain how it works in detail, would it be better to just ask users to only use it (or not use it) in certain circumstances? We might get some flexibility in the future, and it probably is easier to understand to a lot of users. How we do that, is implementation detail. |
There's no backward-compatibility problem with giving a different exception message when zero-arg super() is used inside a generator expression and throws an error. We should not throw an error in the unusual case where it happens to work today; that would be a backwards-compatibility problem. Also for runtime-overhead reasons we would not want to do the "is this a generator expression?" check unless we were already in the error path. But on the whole it's not clear to me that accidental use of zero-arg
I think there's a continuum of options here, not just a binary choice. E.g. I definitely agree that we should simply say "enclosing class definition" and not go into details of how the |
Yeah, this is what I meant.
Yes, this makes sense. But my point was that generator expressions are still implicit generators under the hood and “first argument of immediately enclosing function” does not apply to them. So there could be some additional info in error if current frame is generator.
It's not accidental. User assumes it would work or at least tell him something meaningful in case it fails.
This is not right IMHO. You cant let something work in an unpredicted way just because there is not enough people complaining about it. It’s like having a segfault in a module, but wait - that module is not used a lot so let's leave it like that. While my example can be a bit exaggerated, it makes a point. And I don’t think there is few people who encountered this. I did myself and I did not know what was wrong, so I filled in parameters explicitly and it was fixed. This is probably how it is done by users who encounter it. They just try to fill in parameters, it starts working and they just move on. |
I'm not necessarily opposed to adding extra info to the error message when zero-arg super() fails inside a genexp; I'd like to see how much complexity the PR for it would add. I definitely think the documentation update, whatever form it takes, should include specific mention of the fact that genexps create implicit nested functions and thus zero-arg Also, one more point about this:
If we were to change zero-arg |
…and generator expressions
…and generator expressions
super()
to work inside generator expressionssuper()
error message and document zero-arg super()
usage inside nested functions and generator expressions.
Changing issue title to reflect what is going to be done. Adding special treatment for generator expressions would introduce unnecessary complexity to PR so documenting it + improving error message on super check fail sounds like a good improvement. |
…and generator expressions
…and generator expressions
…and generator expressions
…and generator expressions
…and generator expressions
…and generator expressions
…and generator expressions
…ted functions and generator expressions (GH-113307)
…de nested functions and generator expressions (pythonGH-113307)
…de nested functions and generator expressions (pythonGH-113307)
…de nested functions and generator expressions (pythonGH-113307)
…de nested functions and generator expressions (pythonGH-113307)
Bug report
Bug description:
I tried searching for something related but I could not find exactly this case with
super
. Maybe I missed… so posting it here.Following code works fine:
But if you modify it to be like this:
you will get following error:
Before #101441 ,
[super().foo() for _ in range(10)]
would cause the same error but it was fixed implicitly by PEP709.You can fix this yourself by specifying type and instance in
super()
:If you decide that fixing this makes sense, can I work on fixing it? To me it makes sense (if you decide to leave it as it is) at least to give some kind of warning or error that in this case you should explicitly provide type and instance to
super
, because it is quite confusing why user gets this error as semantically this is valid and should workCPython versions tested on:
CPython main branch
Operating systems tested on:
Linux, macOS
Linked PRs
The text was updated successfully, but these errors were encountered: