-
-
Notifications
You must be signed in to change notification settings - Fork 31k
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
Annotation scopes containing nested scopes #109118
Comments
Thanks, this is very helpful! Minimized version: class name_2[name_3]:
name_3 = name_4
class name_4[name_5]((name_3 for name_5 in name_0), name_3):
pass The runtime crash when removing the |
The first thing to figure out with the compiler crash is how the code should behave, as class scopes are weird. We have a genexp within the type parameters of a generic class nested inside another generic class. Here's the test I wrote, demonstrating how this works if the inner class is not generic:
In short, the genexp should not see the definition in the outer class body, only the one in the generic parameters of the outer class. However, the other bases should see the definition in the class body. This is how it works now if the inner class is not generic, and this is correct. If the inner class is generic, the behavior should be analogous, but evidently there's something wrong in the bookkeeping. |
…function (python#109123) (cherry picked from commit 17f9941)
…EP 695 function (pythonGH-109123). (cherry picked from commit 17f9941) Co-authored-by: Jelle Zijlstra <[email protected]>
Thinking about it more, I recommend that we should disallow nested scopes (comprehensions, generator expressions, and lambdas) within PEP 695 scopes (bases of generic classes, annotations of generic functions, values of type aliases) within classes. The normal scoping rules would produce very weird outcomes and would I think be difficult to implement. Also, disallowing them is a safe option this late in the Python 3.12 release cycle: if we can implement it correctly, we can always allow these constructs later for 3.13. We would disallow the following constructs: class C:
type T = lambda: T # SyntaxError
type T = [T for T in T] # SyntaxError
type T[U]: (T for T in U)] = int # SyntaxError
def f[T](x: lambda: T): ... # SyntaxError
class I[T](T for T in T): ... # SyntaxError All of these constructs would still be allowed by the interpreter outside of class scopes. Why do I say the outcomes are weird? Consider this code: def make_base(arg):
class Base:
__arg__ = arg
return Base
class C[T]:
T = "class"
U = "class"
class Inner[U](make_base((T, U)), make_base((T, U) for _ in (1,))):
pass There are four occurrences of T and U in the bases of Inner. What should they mean (referring to https://peps.python.org/pep-0695/#scoping-behavior)?
Why is it OK to disallow comprehensions/lambdas/genexps in PEP 695 scopes within classes?
I will submit a PR implementing this restriction. |
…classes Fixes python#109118. Fixes python#109194.
I am OK with the "disallow" approach for now. It seems like the simplest option for 3.12, and is unlikely to cause serious practical problems. Longer-term, I think it is awkward and confusing to disallow these constructs in annotation scope only within classes. Among your four bullet points illustrating the awkwardness of the scoping, the third point I don't agree with:
The special case here is the annotation scope, which (unlike other nested scopes) can see the enclosing class scope. IMO, this means that it is consistent and correct (by the same transitive rule of scope visibility that you discuss in the fourth bullet point) for a comprehension (or any other nested scope) within the annotation scope to also see the enclosing class scope. Therefore, I think that If we change this conclusion, that also eliminates the weirdness in the fourth bullet point, and I think results in a reasonable and consistent set of rules. |
@carljm your approach would also have a confusing effect. Given this code:
The annotation for the But now consider this:
Where we made |
Agreed. Of course, it's also counterintuitive if making the method generic renders the previously-working comprehension a SyntaxError :/ It's hard to introduce a new intervening scope with new visibility rules, and also make everything nested inside that new scope behave the same as if it weren't there. If the priority is to avoid adding generics changing the meaning of existing code (which does make sense), then it may be that the best (longer-term) option is to bite your fourth bullet and accept that annotation scopes inside class scopes have non-transitive visibility for class-scoped names. |
PEP 649 will also change the landscape here, since it will mean that all annotations (not just those on generics) run in annotation scopes... |
Another possibility is to make it so that comprehensions in annotation scopes in classes cannot see the annotation scope at all, so in this code: T = "global"
class X:
T = "class"
def meth[T](x: [T for _ in (1,)]): pass
Agree that PEP 649 makes it more important to fix this in a non-SyntaxError way for 3.13. |
I think if we did this, we might need to make it the general rule, not just "in classes." Otherwise it's too surprising that the visibility of But we can't make that the general rule after 3.12 is released, if we are only making nested scopes in annotation scopes within classes a SyntaxError right now. |
Not necessarily. Right now, this code: T = 1
[T for _ in (1,)] Would produce |
Yes, this is what I meant by "Unless I guess you consider..." in my previous comment. I am not sure those two cases are equivalent (in terms of whether it's intuitive for their context to matter), because the one involves an explicit assignment in a separate statement. But I agree that it could be argued they are. |
#109196) Fixes #109118. Fixes #109194. Co-authored-by: Carl Meyer <[email protected]>
…classes (pythonGH-109196) Fixes pythonGH-109118. Fixes pythonGH-109194. (cherry picked from commit b88d9e7) Co-authored-by: Jelle Zijlstra <[email protected]> Co-authored-by: Carl Meyer <[email protected]>
Reopening until 3.12 backport is in |
… function (GH-109123) (#109173) * gh-109118: Fix runtime crash when NameError happens in PEP 695 function (#109123) (cherry picked from commit 17f9941) * [3.12] gh-109118: Fix runtime crash when NameError happens in PEP 695 function (GH-109123). (cherry picked from commit 17f9941) Co-authored-by: Jelle Zijlstra <[email protected]>
… classes (GH-109196) (#109297) gh-109118: Disallow nested scopes within PEP 695 scopes within classes (GH-109196) Fixes GH-109118. Fixes GH-109194. (cherry picked from commit b88d9e7) Co-authored-by: Jelle Zijlstra <[email protected]> Co-authored-by: Carl Meyer <[email protected]>
…classes (python#109196) Fixes python#109118. Fixes python#109194. Co-authored-by: Carl Meyer <[email protected]>
Reopening this to consider changing some of the behavior for Python 3.13, where actual annotations will also be implemented using annotation scopes (PEP-649; cc @larryhastings). I now think the right behavior is that if a scope is nested in an annotation scope that is itself in a class scope, it can see names defined in the annotation scope, but not names defined in the enclosing class scope. After all, that's how generic methods currently work: T = "global T"
U = "global U"
class C:
T = "class T"
U = "class U"
def meth[T](self):
print(T, U)
C().meth() This currently picks up the type variable for T and the global for U. This is also the natural behavior of the current implementation, at least for lambdas. I'll send a PR first to change the behavior for lambdas, then work on comprehensions. |
…hout inlining (#118160) Co-authored-by: Carl Meyer <[email protected]>
Bug report
Bug description:
The following code causes a SystemError during compilation.
output (Python 3.12.0rc2+):
removing the named-expression makes it worse and causes a crash
output (Python 3.12.0rc2+):
side note:
The reason for the strange looking language constructs and variable names is that I generated this code. I found it while working on my pysource-codegen library.
CPython versions tested on:
3.12
Operating systems tested on:
Linux
Linked PRs
The text was updated successfully, but these errors were encountered: